Closed Bug 507775 Opened 10 years ago Closed 9 years ago

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

Categories

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

1.9.2 Branch
x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5
Tracking Status
status1.9.2 --- .5-fixed
status1.9.1 --- .11-fixed

People

(Reporter: Tobbi, Assigned: mats)

Details

(Keywords: crash, verified1.9.1, verified1.9.2, Whiteboard: [sg:critical] [qa-needs-str])

Crash Data

Attachments

(6 files)

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'.
Summary: Firefox crashes upon closedown when selecting display:block for everything (* selector) in CSS with Firebug → Firefox crashes upon closedown when selecting display:block for everything (* selector) in CSS with Firebug in [ @@0x0 | @0x349d7ff]
Another crash report associated with this crash: http://crash-stats.mozilla.com/report/index/e8919ee4-eb58-49ec-afe4-ebeb82090731 (@ @0x0).
Another person I asked (jaymac) crashed with [@@0x0|@0x323c7ff]. http://crash-stats.mozilla.com/report/index/5169ccb1-eada-4830-8b3d-7e4942090801?p=1
Status: UNCONFIRMED → NEW
blocking1.9.1: --- → ?
Ever confirmed: true
Attached file backtrace for this bug
Stacktrace for this crash in plain text
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets
Version: 3.5 Branch → 1.9.1 Branch
Component: XUL → Layout: Block and Inline
Keywords: crash
QA Contact: xptoolkit.widgets → layout.block-and-inline
Summary: Firefox crashes upon closedown when selecting display:block for everything (* selector) in CSS with Firebug in [ @@0x0 | @0x349d7ff] → Firefox crashes upon closedown when selecting display:block for everything (* selector) in CSS with Firebug in [ @@0x0 | @0x349d7ff][@ 0x0 - nsLineLayout::ReflowFrame]
Summary: Firefox crashes upon closedown when selecting display:block for everything (* selector) in CSS with Firebug in [ @@0x0 | @0x349d7ff][@ 0x0 - nsLineLayout::ReflowFrame] → Firefox crashes upon closing when selecting display:block for everything (* selector) in CSS with Firebug in [ @@0x0 | @0x349d7ff][@ 0x0 - nsLineLayout::ReflowFrame]
Rob: can someone on your team look into this?
Group: core-security
blocking1.9.1: ? → ---
Whiteboard: [sg:critical?] firebug-related?
yeah, taking a look.
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?
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.
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?
Whiteboard: [sg:critical?] firebug-related? → [sg:critical?]
(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.
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.
OS: Windows Vista → All
(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.
Attachment #393500 - Attachment mime type: application/vnd.mozilla.xul+xml → application/vnd.mozilla.xul+xml; charset=iso-8859-1
(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::*.
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.
(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.)
Firebug 1.5 is compatible with trunk.

http://getfirebug.com/releases/firebug/1.5X/
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.
(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?
(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.
Blocks: 518287
No longer blocks: 518287
Keywords: testcase-wanted
Summary: Firefox crashes upon closing when selecting display:block for everything (* selector) in CSS with Firebug in [ @@0x0 | @0x349d7ff][@ 0x0 - nsLineLayout::ReflowFrame] → Firefox crashes upon closing when selecting display:block for everything (* selector) in CSS with Firebug in [@ @0x0 | @0x349d7ff][@ @0x0 - nsLineLayout::ReflowFrame]
We don't have a standalone testcase here? Can someone try to make one?
Assignee: nobody → matspal
Mats, any chance you can work on this soon?
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 ?? ()
...
Whiteboard: [sg:critical?] → [sg:critical]
Version: 1.9.1 Branch → 1.9.2 Branch
Attached file 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.
Attachment #438195 - Flags: review?(bzbarsky)
Attachment #438196 - Flags: review?(bzbarsky)
Whiteboard: [sg:critical] → [sg:critical][needs review]
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>.
Attachment #438195 - Flags: review?(bzbarsky) → review+
Attachment #438196 - Flags: review?(bzbarsky) → review+
Thanks Mats!!! Can you attach a testcase here that when can land when it's safe to do so?
Keywords: checkin-needed
As a security bug, this needs SR before landing, no?
Keywords: checkin-needed
Whiteboard: [sg:critical][needs review] → [sg:critical][needs super-review]
(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
(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.
"""
Attachment #438195 - Flags: superreview?(roc)
Attachment #438196 - Flags: superreview?(roc)
Attachment #438195 - Flags: superreview?(roc) → superreview+
Attachment #438196 - Flags: superreview?(roc) → superreview+
looks good for reviews. Mats, please check this in ASAP.
Whiteboard: [sg:critical][needs super-review] → [sg:critical]
Rob, there's lot's of unstarred orange on m-c and people on
#developers seems to think it's a real error(s).
Attempt to make a mochitest.  I can't make it crash!
http://hg.mozilla.org/mozilla-central/rev/1b4a2702d2dd
Status: NEW → RESOLVED
Closed: 10 years ago
status1.9.2: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
Attachment #438195 - Flags: approval1.9.2.4?
Attachment #438195 - Flags: approval1.9.1.10?
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
Attachment #438195 - Flags: approval1.9.2.5+
Attachment #438195 - Flags: approval1.9.2.4?
Attachment #438195 - Flags: approval1.9.1.10?
Attachment #438195 - Flags: approval1.9.1.10+
Comment on attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.

(oops, wrong flag!)
Attachment #438195 - Flags: approval1.9.1.10+ → approval1.9.1.11+
Was this expected to cause a 2.2% SunSpider regression? 'cause it did:

http://mzl.la/9XcV5k
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.
I think you should back out and see if the perf changes revert, just to be safe.
Temporarily backed out on 1.9.1 to investigate perf regression:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5edc3e8585de
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
(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?
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.
Whiteboard: [sg:critical] → [sg:critical] [qa-needs-str]
(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.
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.
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.
(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.
Status: RESOLVED → VERIFIED
We use "Verified" for trunk verification. Please use verified1.9.1 and verified1.9.2 keywords for branch verifications.

Thanks for the help!
Status: VERIFIED → RESOLVED
Closed: 10 years ago9 years ago
Group: core-security
Crash Signature: [@ @0x0 | @0x349d7ff] [@ @0x0 - nsLineLayout::ReflowFrame]
You need to log in before you can comment on or make changes to this bug.