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

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
Layout: Block and Inline
--
critical
RESOLVED FIXED
8 years ago
a year ago

People

(Reporter: Tobbi, Assigned: mats)

Tracking

({crash, verified1.9.1, verified1.9.2})

1.9.2 Branch
mozilla1.9.3a5
x86
All
crash, verified1.9.1, verified1.9.2
Points:
---

Firefox Tracking Flags

(status1.9.2 .5-fixed, status1.9.1 .11-fixed)

Details

([sg:critical] [qa-needs-str], crash signature)

Attachments

(6 attachments)

(Reporter)

Description

8 years ago
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'.
(Reporter)

Updated

8 years ago
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]
(Reporter)

Comment 1

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

Comment 2

8 years ago
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

Comment 3

8 years ago
https://developer.mozilla.org/en/How_to_get_a_stacktrace_with_WinDbg
(Reporter)

Comment 4

8 years ago
Created attachment 392142 [details]
backtrace for this bug

Stacktrace for this crash in plain text
(Reporter)

Updated

8 years ago
Component: General → XUL
Product: Firefox → Core
QA Contact: general → xptoolkit.widgets
Version: 3.5 Branch → 1.9.1 Branch

Updated

8 years ago
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]
(Reporter)

Updated

8 years ago
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: ? → ---
status1.9.1: --- → wanted
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?]
(Reporter)

Comment 10

8 years ago
Created attachment 393500 [details]
Example of XUL file I performed the crash with.
(Reporter)

Comment 11

8 years ago
(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.
(Reporter)

Comment 19

8 years ago
(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.

Updated

8 years ago
Blocks: 518287
No longer blocks: 518287
Keywords: testcase-wanted

Updated

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

Comment 23

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

Comment 24

7 years ago
Created attachment 438192 [details]
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.
(Assignee)

Comment 25

7 years ago
Created attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.
Attachment #438195 - Flags: review?(bzbarsky)
(Assignee)

Comment 26

7 years ago
Created attachment 438196 [details] [diff] [review]
Patch rev. 1, for trunk.
Attachment #438196 - Flags: review?(bzbarsky)
(Assignee)

Updated

7 years ago
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?

Updated

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

Updated

7 years ago
Attachment #438195 - Flags: superreview?(roc)
(Assignee)

Updated

7 years ago
Attachment #438196 - Flags: superreview?(roc)
Attachment #438195 - Flags: superreview?(roc) → superreview+
Attachment #438196 - Flags: superreview?(roc) → superreview+

Comment 32

7 years ago
looks good for reviews. Mats, please check this in ASAP.
Whiteboard: [sg:critical][needs super-review] → [sg:critical]
(Assignee)

Comment 33

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

Comment 34

7 years ago
Created attachment 439031 [details] [diff] [review]
non-working mochitest

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

Comment 35

7 years ago
http://hg.mozilla.org/mozilla-central/rev/1b4a2702d2dd
Status: NEW → RESOLVED
Last Resolved: 7 years ago
status1.9.2: --- → ?
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Updated

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

Comment 38

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/acc99e6c1c1c
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/36188af29584
status1.9.1: wanted → .11-fixed
status1.9.2: ? → .5-fixed
Was this expected to cause a 2.2% SunSpider regression? 'cause it did:

http://mzl.la/9XcV5k
(Assignee)

Comment 40

7 years ago
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

7 years ago
I think you should back out and see if the perf changes revert, just to be safe.
(Assignee)

Comment 42

7 years ago
Temporarily backed out on 1.9.1 to investigate perf regression:
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5edc3e8585de
status1.9.1: .11-fixed → ?
(Assignee)

Comment 43

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

Updated

7 years ago
status1.9.1: ? → .11-fixed
(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]
(Reporter)

Comment 46

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

Comment 49

7 years ago
(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
Last Resolved: 7 years ago7 years ago
Keywords: verified1.9.1, verified1.9.2
Group: core-security
Crash Signature: [@ @0x0 | @0x349d7ff] [@ @0x0 - nsLineLayout::ReflowFrame]
Keywords: testcase-wanted
You need to log in before you can comment on or make changes to this bug.