841579 - nsImageLoadingContent shouldn't depend on load happening after decode

Status: RESOLVED FIXED

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

Description

[Joe Drew (not getting mail)]

Attachment: add a status bit for decode started, and test for it in nsImageLoadingContent

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.

Comment 1

[Joe Drew (not getting mail)]

Attachment: test that removing images from the DOM midway through loading doesn't inhibit proper loads

Comment 2

[Seth Fowler [:seth] [:s2h]]

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.

Comment 3

[Joe Drew (not getting mail)]

hm, though this does seem to break a couple of crashtests: https://tbpl.mozilla.org/?tree=Try&rev=a530385e3a30

Comment 4

[Joe Drew (not getting mail)]

Attachment: v2

Another try that should probably fix some of that orange: https://tbpl.mozilla.org/?tree=Try&rev=3b4713931db0

Comment 5

[Joe Drew (not getting mail)]

Attachment: v3, fix typo

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

Comment 6

[Robert Lickenbrock [:rclick]]

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.

Comment 7

[Joe Drew (not getting mail)]

Onload is unblocked at the earliest of OnStopRequest or OnDecodingFinished, IIRC, so it won't work, I think?

Comment 8

[Joe Drew (not getting mail)]

Attachment: v4, check for errors

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

Comment 9

[Joe Drew (not getting mail)]

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 10

[Joe Drew (not getting mail)]

Comment on attachment 714139 [details] [diff] [review]
test that removing images from the DOM midway through loading doesn't inhibit proper loads

rs=khuey

Comment 11

[Joe Drew (not getting mail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0222cb048028
https://hg.mozilla.org/integration/mozilla-inbound/rev/d21d144e3db9

Comment 12

[Seth Fowler [:seth] [:s2h]]

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.

Comment 13

[Joe Drew (not getting mail)]

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.

Comment 14

[Joe Drew (not getting mail)]

Try run was successful: https://tbpl.mozilla.org/?tree=Try&rev=303b6b3e03c7

Landed again, hopefully to stick this time:

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/7fb88d0bf0ec
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/66006561df23

Comment 15

[Joe Drew (not getting mail)]

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

Comment 16

[Joe Drew (not getting mail)]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d37fdbac89f2