494590 - nsCSSValue::StartImageLoad does synchronous image load constructing frames

Status: NEW

(Core :: CSS Parsing and Computation, defect)

Description

[Olli Pettay [:smaug][[e-mail]]]

This causes the forum problem in bug 491063.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Yeah, see bug 491063 comment 40...

Comment 2

[Mike Beltzner [:beltzner, not reading bugmail]]

To add some more context to this bug:

The fix for bug 491063 (described in bug 491063 comment 40) seems to have caused a problem where a blank sheet is presented when visiting an https site, in this case, the Mozilla Intranet Forum.

from bug 491063 comment 49:
> I can reproduce the forum problem if I set 
> security.warn_viewing_mixed to true and security.warn_viewing_mixed.show_once
> to false. Investigating.

Blocking until this is better understood.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #2)
> The fix for bug 491063 (described in bug 491063 comment 40) seems to have
> caused a problem where a blank sheet is presented when visiting an https site,
> in this case, the Mozilla Intranet Forum.
This is not a regression from bug 491063, but the fix in that bug didn't
include the fix for nsCSSValue.

I don't feel well at all today, but I'll try to write the patch tomorrow
(unless someone takes this bug from me before that ;) ).

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: patch

..but I think I need to look at also other cases when nsContentUtils::LoadImage is used.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 379549 [details] [diff] [review]
patch

Bah, no, this breaks many reftests.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: idea 2

This doesn't work either. Reftest runs ok, but images in xul menus are painted
too late, or not at all.

Comment 7

[Olli Pettay [:smaug][[e-mail]]]

Attachment: allow chrome flushes

Jonas, Boris, would this be terrible?
I can't figure out anything simple to fix CSS image load.

Comment 8

[Boris Zbarsky [:bzbarsky]]

The xul images thing is weird, but that first patch was clearly wrong in the way you tried to address in the second one: it breaks all the consumers who expect null mRequest to mean the image just couldn't be loaded...

I'll have to think about the chrome flush thing a bit, but long-term we really do need to fix this bug the right way; the current code is totally unsafe.  So maybe the right thing to do is go through the mRequest consumers and see what they're up to.  Not so palatable for 1.9.1.  :(

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I was thinking the 2nd approach again, and perhaps that is still the
way to go. I need to figure out why icons in xul menus break.

Comment 10

[Boris Zbarsky [:bzbarsky]]

I'd rather examine (and fix) the callers than have the extra shim object, if possible, for what it's worth.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2.1

Comment 12

[Boris Zbarsky [:bzbarsky]]

For that last diff, the Clone() impl looks wrong (will start new loads instead of just glomming on to the existing one).

And in general, I worry about the memory and performance overhead here...

I was pretty serious about adjusting the consumers to not assume null request means load failed to start.  That almost seems safer to me than trying to get this shim correct _and_ performant.

Comment 13

[Boris Zbarsky [:bzbarsky]]

And just to be clear, just about any change we make here scares the bejeezus out of me.  :(

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #13)
> And just to be clear, just about any change we make here scares the bejeezus
> out of me.  :(
Yeah. Which is why I'm trying several different approaches.
It is very unfortunate that this bug was noticed so late.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

Attachment: v2.3 (without leaks and doesn't break generated content with images.)

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #15)
> Created an attachment (id=379688) [details]
This has still the original Clone behavior.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #16)
> This has still the original Clone behavior.
I mean the v2 patch behavior.

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #12)
> I was pretty serious about adjusting the consumers to not assume null request
> means load failed to start.  That almost seems safer to me than trying to get
> this shim correct _and_ performant.
So far I haven't figured out any easy way to handle null request.
There are cases like XUL image handling, generated content images, CSS stuff, etc

Comment 19

[Boris Zbarsky [:bzbarsky]]

Yes, they'd need to be handled separately...

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Boris mentioned on this morning's phone call that we might be able to spawn the dialog asynchronously, at least on branch, to work around the underlying issues. That makes sense to me.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

I'm writing that async patch ATM.

Comment 22

[Boris Zbarsky [:bzbarsky]]

It might be worth spinning that off into a separate bug, or cloning this one, or something.  We do still want to fix this bug as filed.

Comment 23

[Johnathan Nightingale [:johnath]]

(In reply to comment #22)
> It might be worth spinning that off into a separate bug, or cloning this one,
> or something.  We do still want to fix this bug as filed.

I agree - filed bug 494940 for the async approach in particular.  Olli, you say you're already writing that patch?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

Attachment: async alert

This is pretty much the minimal patch.
Not sure if someone else should sr this.

If some background tab requests alert, that tab is brought to foreground.

Comment 25

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 379736 [details] [diff] [review]
async alert

Even safer patch coming

Comment 26

[Olli Pettay [:smaug][[e-mail]]]

Attachment: async alert, v2

Comment 27

[Olli Pettay [:smaug][[e-mail]]]

Bah, I'll move the patch to the new bug

Comment 28

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Deblocking, since we're asyncing it up for 3.5.