{{ message }}
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be nice to reduce this forced one-second frequency... what would you think about something like so?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll try that, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibson042 Now that I think about it... How do we know this
onerrorcallback will be fired as the last one? 404 responses may not be cached AFAIK so there's a risk that one will fire before the other ones are done.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well,
src="#"would guarantee that the invalid source is cached.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That will resolve to the current page, though, so it won't be a 404.
Also, currently all the tests succeed even if cache is disabled in the browser DevTools. I'd like to keep that working.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@gibson042 I'm thinking about ways to reduce this delay but if I don't find anything reliable that'd work with cache disabled, I think I'll just merge it, we can always apply improvements later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a 404, just need a response that isn't a successful image. And I don't see any cache concerns, but if you want to merge without the acceleration then I won't object.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point about not needing 404s. I tested your suggestion, though, and - if I understood it correctly - it looks like I was right about race conditions. See the code at:
mgol@7cd593f
I reverted the two security fixes locally & all assertions should fail but it's pretty random for me; on each refresh different ones are failing for me. I tried the
esmodulestests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An example test run below, I only run this single test. Note that the order of reported assertions is also pretty random.

There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, raciness confirmed. I don't think that's fatal to the testing, but it would require restructuring. No worries.