Closed Bug 841579 Opened 7 years ago Closed 7 years ago

nsImageLoadingContent shouldn't depend on load happening after decode

Categories

(Core :: DOM: Core & HTML, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: joe, Assigned: joe)

References

Details

Attachments

(2 files, 3 obsolete files)

Currently, nsImageLoadingContent seems to depend on load happening after decode, in that errors that get noticed after the size decode happens are ignored. This is relatively OK, except that in bug 716140, all decode notifications are made asynchronously, and further, all images must go through a size decode, even ones we're decoding fully right away.

As an alternative to this, I propose this patch, which will make nsImageLoadingContent wait for the DECCODE_COMPLETE notification if, when LOAD_COMPLETE fires, one of (a) we are still waiting for a decode; or (b) we have asked for a decode.
Attachment #714136 - Flags: review?(khuey)
Is this green on try right now? I believe we could get rid of the "delay firing OnStopRequest" stuff from bug 704059 if this works.
hm, though this does seem to break a couple of crashtests: https://tbpl.mozilla.org/?tree=Try&rev=a530385e3a30
Attached patch v2 (obsolete) — Splinter Review
Another try that should probably fix some of that orange: https://tbpl.mozilla.org/?tree=Try&rev=3b4713931db0
Attachment #714136 - Attachment is obsolete: true
Attachment #714136 - Flags: review?(khuey)
Attachment #714158 - Flags: review?(khuey)
Depends on: 841661
Attached patch v3, fix typo (obsolete) — Splinter Review
Argh, typo + not running tests locally = sad Try run. New one, including a fix for dependent bug 841661 that caused my tests to fail without cause: https://tbpl.mozilla.org/?tree=Try&rev=e334074e7154
Attachment #714158 - Attachment is obsolete: true
Attachment #714158 - Flags: review?(khuey)
Attachment #714251 - Flags: review?(khuey)
Is there any reason why this couldn't utilize the existing onload blocking notifications? It seems like we could check if the REQUEST_BLOCKS_ONLOAD flag is set, and if it is, set another flag indicating that we deferred the load events. Then, in UnblockOnload, check that flag and fire the events if needed.
Onload is unblocked at the earliest of OnStopRequest or OnDecodingFinished, IIRC, so it won't work, I think?
Since what we're looking for is actually errors that might come after OnStopRequest, we need to check for errors in OnStopRequest too.

This version is very happy on try: https://tbpl.mozilla.org/?tree=Try&rev=2a5e6da4fc4b
Attachment #714251 - Attachment is obsolete: true
Attachment #714251 - Flags: review?(khuey)
Attachment #714554 - Flags: review?(khuey)
One change is needed to this patch to make it entirely correct:

-  if (NS_SUCCEEDED(aStatus) &&
+  if (NS_SUCCEEDED(aStatus) && !(reqStatus & imgIRequest::STATUS_ERROR) &&

When reviewing, please pretend that's the code you're reviewing.
Comment on attachment 714139 [details] [diff] [review]
test that removing images from the DOM midway through loading doesn't inhibit proper loads

rs=khuey
Attachment #714139 - Flags: review?(khuey) → review+
Glad to see this getting pushed in. This is super great stuff, Joe. I've filed bug 847630 to take advantage of this to simplify the code in VectorImage a bit.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/644043c3de95 - it broke chrome://mochitests/content/chrome/image/test/mochitest/test_removal_ondecode.html and /tests/image/test/mochitest/test_bug512435.html.
Target Milestone: mozilla22 → ---
Depends on: 848516
And out the code comes again, despite the green runs on Try, because it causes a frequent intermittent failure in image/test/mochitest/test_bug512435.html | img_{a,b} should be decoded before onload fires. 

http://hg.mozilla.org/integration/mozilla-inbound/rev/d4a0f20ca152
Thanks to bug 848624, our build scripts didn't know that imgIRequest.idl had changed, so it put the old version of imgIRequest.xpt into the distributed interfaces.xpt, meaning that it was a crapshoot as to whether our build machines were even testing the new interface, depending on how recently the machine had been clobbered.

This also explains why I could never reproduce the bug on my computer or on try.

Anyways, repushed with a UUID change, which should fix that problem and let it stick.

https://hg.mozilla.org/integration/mozilla-inbound/rev/d37fdbac89f2
https://hg.mozilla.org/mozilla-central/rev/d37fdbac89f2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.