When nsBulletFrame loads images, it should block onload

RESOLVED FIXED in mozilla36

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: seth, Assigned: seth)

Tracking

(Blocks: 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Blocks: 1091236
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
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: 773482
(Assignee)

Comment 3

4 years ago
Created attachment 8513838 [details] [diff] [review]
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
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+
(Assignee)

Comment 5

4 years ago
(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.
(Assignee)

Comment 7

4 years ago
Created attachment 8514484 [details] [diff] [review]
Make nsBulletFrame block onload when it loads images

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

Updated

4 years ago
Attachment #8513838 - Attachment is obsolete: true
(Assignee)

Comment 8

4 years ago
Created attachment 8514486 [details] [diff] [review]
Make nsBulletFrame block onload when it loads images

Also, B2G needs a little bit of fuzz.
(Assignee)

Updated

4 years ago
Attachment #8514484 - Attachment is obsolete: true
(Assignee)

Comment 10

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

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

Comment 11

4 years ago
Doh. Needed to repush that one:

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

Comment 13

4 years ago
Created attachment 8519361 [details] [diff] [review]
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
(Assignee)

Updated

4 years ago
Attachment #8514486 - Attachment is obsolete: true
(Assignee)

Comment 14

4 years ago
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
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in before you can comment on or make changes to this bug.