Closed Bug 1091229 Opened 6 years ago Closed 6 years ago

When nsBulletFrame loads images, it should block onload

Categories

(Core :: ImageLib, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: seth, Assigned: seth)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 3 obsolete files)

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.
Blocks: 1091236
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.
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.)
Blocks: b2g-reftest
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
Attachment #8513838 - Flags: review?(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?
Attachment #8513838 - Flags: review?(tnikkel) → review+
(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.
(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.
This tweaked version of the patch invalidates the frame on LOAD_COMPLETE, just like nsImageFrame does.
Attachment #8513838 - Attachment is obsolete: true
Also, B2G needs a little bit of fuzz.
Attachment #8514484 - Attachment is obsolete: true
Hmm. Here's an additional try push that tries some more aggressive techniques:

https://tbpl.mozilla.org/?tree=Try&rev=6e39bb271d25
Doh. Needed to repush that one:

https://tbpl.mozilla.org/?tree=Try&rev=6383f8d0ba37
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
Attachment #8514486 - Attachment is obsolete: true
Looks like that took care of the problem. Nice! So this patch is ready to go.
https://hg.mozilla.org/mozilla-central/rev/4fe0150f01d4
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.