507775 - Firefox crashes upon closing when selecting display:block for everything (* selector) in CSS with Firebug in [@ @0x0 | @0x349d7ff][@ @0x0 - nsLineLayout::ReflowFrame]

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Tobias (:Tobbi) Markus]

I noticed that Firefox can crash upon closedown when you do the following:
1. Install Firebug from https://addons.mozilla.org/de/firefox/addon/1843 and restart.
2. Load a random XUL document in Firefox.
3. Start up Firebug for this XUL document.
4. Go to the Styles tab on the right and select the '*' selector.
5. Change the 'display' property to block.
6. Close Firefox. 
--> Crash with http://crash-stats.mozilla.com/report/index/226e45b8-3da0-4fee-90ca-164f62090731

I can't really say what causes that crash, thus component 'General'.

Comment 1

[Tobias (:Tobbi) Markus]

Another crash report associated with this crash: http://crash-stats.mozilla.com/report/index/e8919ee4-eb58-49ec-afe4-ebeb82090731 (@ @0x0).

Comment 2

[Tobias (:Tobbi) Markus]

Another person I asked (jaymac) crashed with [@@0x0|@0x323c7ff]. http://crash-stats.mozilla.com/report/index/5169ccb1-eada-4830-8b3d-7e4942090801?p=1

Comment 3

[timeless]

https://developer.mozilla.org/en/How_to_get_a_stacktrace_with_WinDbg

Comment 4

[Tobias (:Tobbi) Markus]

Attachment: backtrace for this bug

Stacktrace for this crash in plain text

Comment 5

[Daniel Veditz [:dveditz]]

Rob: can someone on your team look into this?

Comment 6

[Rob Campbell [:rc] (:robcee)]

yeah, taking a look.

Comment 7

[Rob Campbell [:rc] (:robcee)]

I was able to crash Firefox 3.5.1 pretty easily by loading chrome://browser/content/browser.xul and changing * to { display: block; } as the reporter describes. Still waiting for my stack-trace, but it should appear here momentarily:

http://crash-stats.mozilla.com/report/index/de597de2-a2e6-48b3-8aee-fe4982090810?p=1

One notable difference: I didn't have to close Firefox. I unfocused it by clicking on another window and then the crash occurred. Betting it happened on reflow.

Tobias: I'm curious what xul document you were inspecting when you triggered this crash?

Comment 8

[Rob Campbell [:rc] (:robcee)]

one other thing: I don't think this is necessarily being caused by Firebug. I expect it's a result of modifying the xul document's css in a way that would normally never happen. Might be possible to duplicate this by modifying userAgent.css to include a | * { display: block; } | rule.

Comment 9

[Rob Campbell [:rc] (:robcee)]

whups, meant userChrome.css.

this, made my browser pretty ugly, but didn't crash. Makes sense I guess, because this seems to be stemming from a later reflow.

any theories from the layout people?

Comment 10

[Tobias (:Tobbi) Markus]

Attachment: Example of XUL file I performed the crash with.

Comment 11

[Tobias (:Tobbi) Markus]

(In reply to comment #7)
> Tobias: I'm curious what xul document you were inspecting when you triggered
> this crash?

I was just testing a random XUL file, such as the one I attached above.

Comment 12

[Rob Campbell [:rc] (:robcee)]

happens on Mac too.

thank, Tobias. Guess it doesn't matter what the content is, just that it's xul.

based on the stack trace, looks like, nsLineLayout& aLineLayout parameter was null going in.

Not really sure how exploitable this is, Dan. Seems like it would be difficult to inject code from this. Would like to get some layout guys cc'd here to help debug.

Comment 13

[David Baron :dbaron:]

(In reply to comment #12)
> based on the stack trace, looks like, nsLineLayout& aLineLayout parameter was
> null going in.

Sounds like something that should be getting wrapped in a block (non-XUL content inside XUL) isn't getting wrapped.

Comment 14

[David Baron :dbaron:]

(In reply to comment #13)
> Sounds like something that should be getting wrapped in a block (non-XUL
> content inside XUL) isn't getting wrapped.

Er, except not if it's in nsBlockFrame::*.

Comment 15

[David Baron :dbaron:]

For what it's worth, in a 1.9.0 build, if I modify the testcase to add:

<?xml-stylesheet href='data:text/css,window[foo] *{display:block}' type='text/css'?>

and then enter this in the URL bar:

javascript:void(document.documentElement.setAttribute('foo', 'bar'))

I don't get a crash.

Comment 16

[David Baron :dbaron:]

(And, for what it's worth, being able to know whether this was fixed on trunk already is another reason it would be really great to have a Firebug that was compatible with trunk.)

Comment 17

[Rob Campbell [:rc] (:robcee)]

Firebug 1.5 is compatible with trunk.

http://getfirebug.com/releases/firebug/1.5X/

Comment 18

[Rob Campbell [:rc] (:robcee)]

just to loopback, I mentioned this in last weekend's meeting and we think we have a fix for it in Firebug.

we can get the origin of a rule and detect if that's coming from one of the userAgent.css files. If so, we can prevent modification of it. Should fix this.

Comment 19

[Tobias (:Tobbi) Markus]

(In reply to comment #18)
> we can get the origin of a rule and detect if that's coming from one of the
> userAgent.css files. If so, we can prevent modification of it. Should fix this.

A remark on my site: Wouldn't that only fix the bug in Firebug, so that it can't be triggered using Firebug anymore, wouldn't the actual bug remain?

Comment 20

[Rob Campbell [:rc] (:robcee)]

(In reply to comment #19)
> (In reply to comment #18)
> > we can get the origin of a rule and detect if that's coming from one of the
> > userAgent.css files. If so, we can prevent modification of it. Should fix this.
> 
> A remark on my site: Wouldn't that only fix the bug in Firebug, so that it
> can't be triggered using Firebug anymore, wouldn't the actual bug remain?

yes. You would no longer be able to trigger this bug using Firebug. Morphing a display rule on XUL page will still crash if you can do it some other way.

Comment 21

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

We don't have a standalone testcase here? Can someone try to make one?

Comment 22

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

Mats, any chance you can work on this soon?

Comment 23

[Mats Palmgren (inactive)]

In a 1.9.2 debug build on x86-64 Linux I get:

*** stack smashing detected ***: /OBJDIR/usr/moz/1.9.2/dist/bin/firefox-bin terminated

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff34a0c27 in ?? () from /lib/libgcc_s.so.1
(gdb) bt
#0  0x00007ffff34a0c27 in ?? () from /lib/libgcc_s.so.1
#1  0x00007ffff34a148b in _Unwind_Backtrace () from /lib/libgcc_s.so.1
#2  0x00007ffff321978e in *__GI___backtrace (array=<value optimized out>, size=64) at ../sysdeps/x86_64/../ia64/backtrace.c:85
#3  0x00007ffff318dcab in __libc_message (do_abort=<value optimized out>, fmt=<value optimized out>) at ../sysdeps/unix/sysv/linux/libc_fatal.c:168
#4  0x00007ffff3219647 in *__GI___fortify_fail (msg=0x7ffff3257d2e "stack smashing detected") at fortify_fail.c:32
#5  0x00007ffff3219610 in __stack_chk_fail () at stack_chk_fail.c:29
#6  0x00007ffff7191c30 in NS_CopyUnicodeToNative (input=<value optimized out>, output=...) at /usr/moz/1.9.2/xpcom/io/nsNativeCharsetUtils.cpp:868
#7  0xdadadadadadadada in ?? ()
#8  0xdadadadadadadada in ?? ()
#9  0xdadadadadadadada in ?? ()
#10 0x00ff69ca0000ed80 in ?? ()
#11 0x000042c000ff58c0 in ?? ()
#12 0x0000b9e1000011a0 in ?? ()
#13 0x000000000000ec00 in ?? ()
#14 0x0000eccb0000ec40 in ?? ()
#15 0xdadadadadadadada in ?? ()
#16 0xdadadadadadadada in ?? ()
#17 0xdadadadadadadada in ?? ()
#18 0x0000ecca0000ec80 in ?? ()
#19 0xdadadadadadadada in ?? ()
#20 0xdadadadadadadada in ?? ()
#21 0xdadadadadadadada in ?? ()
#22 0x0000ecc90000ecc0 in ?? ()
...

Comment 24

[Mats Palmgren (inactive)]

Attachment: frame dump + stacks

There's a restyle request for the root box's popupgroup (reframing it),
it destroys the PopupSetFrame (lime) which destroys a bunch of
OOF MenuPopup frames, leaving dangling placeholder frames all over...

This dump shows the original frame tree, the stack for the restyle
request, some assertions associated with deleting the OOFs, then
the crash stack (in a debug build), and the broken frame tree
at that point.

Comment 25

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1, for branches.

Comment 26

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1, for trunk.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.

So the issue is that a frame contains OOFs but not their placeholders?  <sigh>.

Comment 28

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

Thanks Mats!!! Can you attach a testcase here that when can land when it's safe to do so?

Comment 29

[Reed Loden [:reed]]

As a security bug, this needs SR before landing, no?

Comment 30

[Brendan Eich [:brendan]]

(In reply to comment #29)
> As a security bug, this needs SR before landing, no?

We got rid of that bogus rule (right?), but maybe this just needs regular SR.

/be

Comment 31

[Reed Loden [:reed]]

(In reply to comment #30)
> (In reply to comment #29)
> > As a security bug, this needs SR before landing, no?
> 
> We got rid of that bogus rule (right?), but maybe this just needs regular SR.

Nope. According to http://www.mozilla.org/hacking/reviewers.html ("What needs super-review?"),
"""
All changes involving security

For security bugs, and changes to security models or mechanisms, we want extra experienced eyes to be involved with those changes.
"""

Comment 32

[Robert Sayre]

looks good for reviews. Mats, please check this in ASAP.

Comment 33

[Mats Palmgren (inactive)]

Rob, there's lot's of unstarred orange on m-c and people on
#developers seems to think it's a real error(s).

Comment 34

[Mats Palmgren (inactive)]

Attachment: non-working mochitest

Attempt to make a mochitest.  I can't make it crash!

Comment 35

[Mats Palmgren (inactive)]

http://hg.mozilla.org/mozilla-central/rev/1b4a2702d2dd

Comment 36

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

Comment on attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.

a=beltzner for mozilla-1.9.1 and mozilla-1.9.2 default

Comment 37

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

Comment on attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.

(oops, wrong flag!)

Comment 38

[Mats Palmgren (inactive)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/acc99e6c1c1c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/36188af29584

Comment 39

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

Was this expected to cause a 2.2% SunSpider regression? 'cause it did:

http://mzl.la/9XcV5k

Comment 40

[Mats Palmgren (inactive)]

I responded to the 2.2% Dromaeo regression on mozilla.dev.tree-management.
There also a "Improvement: SunSpider decrease 5.11% on Win7 Firefox3.5"
on this changeset.  Both are unexpected and "impossible" to have been
caused by this patch.  I'm guessing something was changed in the test
and/or testing environment.
I'll be happy to backout/re-land to verify if you like.

Comment 41

[Robert Sayre]

I think you should back out and see if the perf changes revert, just to be safe.

Comment 42

[Mats Palmgren (inactive)]

Temporarily backed out on 1.9.1 to investigate perf regression:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5edc3e8585de

Comment 43

[Mats Palmgren (inactive)]

Dromaeo(V8) on WINNT 5.1 looked fine to me, so I re-landed:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c3e1d2040160

Comment 44

[Al Billings [:abillings - ex-MoCo]]

(In reply to comment #0)
> I noticed that Firefox can crash upon closedown when you do the following:
> 1. Install Firebug from https://addons.mozilla.org/de/firefox/addon/1843 and
> restart.
> 2. Load a random XUL document in Firefox.
> 3. Start up Firebug for this XUL document.
> 4. Go to the Styles tab on the right and select the '*' selector.
> 5. Change the 'display' property to block.
> 6. Close Firefox. 

 Did the UI change in Firebug? I see no '*' selector in the Styles tab on the right in current Firebug. What version were you using for this?

Comment 45

[Al Billings [:abillings - ex-MoCo]]

Looking at this in Firebug 1.4.1, the UI is the same so I'm just unclear on how to repro based on the above steps. I'm not a Firebug user normally.

Comment 46

[Tobias (:Tobbi) Markus]

(In reply to comment #45)
> Looking at this in Firebug 1.4.1, the UI is the same so I'm just unclear on how
> to repro based on the above steps. I'm not a Firebug user normally.

Load a XUL document, then open firebug. Click the styles tab. You should find a * selector.

Comment 47

[Al Billings [:abillings - ex-MoCo]]

If I load chrome://browser/content/browser.xul or the file attached here and open up Firebug, the right pane has tabs that say, "Style", "Computed Layout", and "DOM". Selecting style, the menu off of it says, "Show User Agent CSS", "Expand Shorthand Properties", ":active", and ":hover" with nothing else. Where is the * selector?

So, I'm obviously not getting something here. I assume it is personal failure rather than Firebug but, as I said, I don't use Firebug normally.

Comment 48

[Al Billings [:abillings - ex-MoCo]]

Ah, Tobbi made this clearer on IRC. I still can't reproduce the crash on either 1.9.1 and 1.9.2. He's going to look at this.

Comment 49

[Tobias (:Tobbi) Markus]

(In reply to comment #48)
> Ah, Tobbi made this clearer on IRC. I still can't reproduce the crash on either
> 1.9.1 and 1.9.2. He's going to look at this.

I can't reproduce the crash either with the latest candidate builds (3.6.7 and 3.5.11). 

Marking VERIFIED.

Comment 50

[Al Billings [:abillings - ex-MoCo]]

We use "Verified" for trunk verification. Please use verified1.9.1 and verified1.9.2 keywords for branch verifications.

Thanks for the help!