1091229 - When nsBulletFrame loads images, it should block onload

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

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

nsBulletFrame needs to block onload when it loads images. Not doing so makes any test that uses list items a potential intermittent orange (see bug 850197 for an example that has plagued us for years). Bug 1089046 makes changes to the timing of ImageLib notifications that make such intermittent oranges much more likely, as well, so now is the time to take this on.

Comment 1

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

In the long term what we really want to do is make nsBulletFrame use ImageLoader for loading; I've filed bug 1091236 as a followup.

Comment 2

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

I suspect with this fixed, we can enable the |list-simple-1.html| test on B2G. Making this block bug 773482 speculatively. (I'll remove the block if that turns out not to be true. I haven't tested this theory on try yet.)

Comment 3

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

Attachment: Make nsBulletFrame block onload when it loads images

Here's the patch. It's pretty simple, though it got a bit more verbose just because I wanted to make this a zero-overhead patch but I also wanted to assert for double blocks/unblocks, which meant I needed to pack the new boolean flag I added, which meant I needed some additional code to avoid taking the address of a bitfield. I think things came out pretty clean, though.

I verified that the use of GetComposedDoc() in GetOurCurrentDoc() is correct in person with wchen.

Try job here:

https://tbpl.mozilla.org/?tree=Try&rev=43e898114422

Comment 4

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8513838 [details] [diff] [review]
Make nsBulletFrame block onload when it loads images

>@@ -686,24 +666,76 @@ nsBulletFrame::Notify(imgIRequest *aRequ
>+  if (aType == imgINotificationObserver::LOAD_COMPLETE) {
>+    // Unconditionally start decoding for now.
>+    // XXX(seth): We eventually want to decide whether to do this based on
>+    // visibility. We should get that for free from bug 1091236.
>+    if (aRequest == mImageRequest) {
>+      mImageRequest->StartDecoding();
>     }
>   }

I think RequestDecode would be better here, do you agree?

Comment 5

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

(In reply to Timothy Nikkel (:tn) from comment #4)
> I think RequestDecode would be better here, do you agree?

Thanks for the quick review! I think you're right that RequestDecode would be better. I had just imitated what nsImageLoadingContent did, but actually I wonder if it might be better off with RequestDecode too.

I'm a bit puzzled at these try results, though. How could this patch have caused these failures?! I've pushed a couple of variants to investigate.

Comment 6

[Timothy Nikkel (:tnikkel)]

(In reply to Seth Fowler [:seth] from comment #5)
> (In reply to Timothy Nikkel (:tn) from comment #4)
> > I think RequestDecode would be better here, do you agree?
> 
> Thanks for the quick review! I think you're right that RequestDecode would
> be better. I had just imitated what nsImageLoadingContent did, but actually
> I wonder if it might be better off with RequestDecode too.

I've wanted to change that too, but haven't gotten to it.

> I'm a bit puzzled at these try results, though. How could this patch have
> caused these failures?! I've pushed a couple of variants to investigate.

Odd, I'm curious what the answer is too.

Comment 7

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

Attachment: Make nsBulletFrame block onload when it loads images

This tweaked version of the patch invalidates the frame on LOAD_COMPLETE, just like nsImageFrame does.

Comment 8

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

Attachment: Make nsBulletFrame block onload when it loads images

Also, B2G needs a little bit of fuzz.

Comment 9

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

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=d41ae163ae79

Comment 10

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

Hmm. Here's an additional try push that tries some more aggressive techniques:

https://tbpl.mozilla.org/?tree=Try&rev=6e39bb271d25

Comment 11

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

Doh. Needed to repush that one:

https://tbpl.mozilla.org/?tree=Try&rev=6383f8d0ba37

Comment 12

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

Sigh. Forgot a null-check.

https://tbpl.mozilla.org/?tree=Try&rev=1b9e32321838

Comment 13

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

Attachment: Make nsBulletFrame block onload when it loads images

Alright, the remaining failures look real. It looks like we're failing to unblock onload in those cases. I've added code to unblock onload in DeregisterAndCancelImageRequest, which should hopefully take care of it. I've also added an NS_ASSERTION about it in the destructor.

Here's a new try job:

https://tbpl.mozilla.org/?tree=Try&rev=c00dedcc3bde

Comment 14

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

Looks like that took care of the problem. Nice! So this patch is ready to go.

Comment 15

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

Pushed:

https://hg.mozilla.org/integration/mozilla-inbound/rev/4fe0150f01d4

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/4fe0150f01d4