Last Comment Bug 507775 - 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 (* s...
Status: RESOLVED FIXED
[sg:critical] [qa-needs-str]
: crash, verified1.9.1, verified1.9.2
Product: Core
Classification: Components
Component: Layout: Block and Inline (show other bugs)
: 1.9.2 Branch
: x86 All
: -- critical (vote)
: mozilla1.9.3a5
Assigned To: Mats Palmgren (:mats)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-07-31 21:53 PDT by [retired]
Modified: 2015-10-16 11:38 PDT (History)
15 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.5-fixed
.11-fixed


Attachments
backtrace for this bug (19.33 KB, text/plain)
2009-08-02 04:56 PDT, [retired]
no flags Details
Example of XUL file I performed the crash with. (3.65 KB, application/vnd.mozilla.xul+xml; charset=iso-8859-1)
2009-08-10 06:12 PDT, [retired]
no flags Details
frame dump + stacks (43.56 KB, text/html)
2010-04-09 15:47 PDT, Mats Palmgren (:mats)
no flags Details
Patch rev. 1, for branches. (1.16 KB, patch)
2010-04-09 15:59 PDT, Mats Palmgren (:mats)
bzbarsky: review+
roc: superreview+
mbeltzner: approval1.9.2.5+
mbeltzner: approval1.9.1.11+
Details | Diff | Review
Patch rev. 1, for trunk. (1.11 KB, patch)
2010-04-09 16:01 PDT, Mats Palmgren (:mats)
bzbarsky: review+
roc: superreview+
Details | Diff | Review
non-working mochitest (2.43 KB, patch)
2010-04-14 11:08 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Review

Description [retired] 2009-07-31 21:53:52 PDT
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 [retired] 2009-07-31 22:23:19 PDT
Another crash report associated with this crash: http://crash-stats.mozilla.com/report/index/e8919ee4-eb58-49ec-afe4-ebeb82090731 (@ @0x0).
Comment 2 [retired] 2009-08-01 04:42:21 PDT
Another person I asked (jaymac) crashed with [@@0x0|@0x323c7ff]. http://crash-stats.mozilla.com/report/index/5169ccb1-eada-4830-8b3d-7e4942090801?p=1
Comment 4 [retired] 2009-08-02 04:56:12 PDT
Created attachment 392142 [details]
backtrace for this bug

Stacktrace for this crash in plain text
Comment 5 Daniel Veditz [:dveditz] 2009-08-07 11:50:08 PDT
Rob: can someone on your team look into this?
Comment 6 Rob Campbell [:rc] (:robcee) 2009-08-10 05:41:40 PDT
yeah, taking a look.
Comment 7 Rob Campbell [:rc] (:robcee) 2009-08-10 05:59:19 PDT
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) 2009-08-10 06:00:37 PDT
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) 2009-08-10 06:06:46 PDT
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 [retired] 2009-08-10 06:12:16 PDT
Created attachment 393500 [details]
Example of XUL file I performed the crash with.
Comment 11 [retired] 2009-08-10 06:12:55 PDT
(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) 2009-08-10 06:21:09 PDT
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: ⌚️UTC-7 (review requests must explain patch) 2009-08-10 07:43:31 PDT
(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: ⌚️UTC-7 (review requests must explain patch) 2009-08-10 07:49:33 PDT
(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: ⌚️UTC-7 (review requests must explain patch) 2009-08-10 08:13:40 PDT
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: ⌚️UTC-7 (review requests must explain patch) 2009-08-10 08:51:56 PDT
(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) 2009-08-10 09:20:17 PDT
Firebug 1.5 is compatible with trunk.

http://getfirebug.com/releases/firebug/1.5X/
Comment 18 Rob Campbell [:rc] (:robcee) 2009-08-18 09:08:13 PDT
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 [retired] 2009-08-18 09:49:45 PDT
(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) 2009-08-21 09:55:18 PDT
(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) (Exited; email my personal email if necessary) 2010-02-16 13:05:32 PST
We don't have a standalone testcase here? Can someone try to make one?
Comment 22 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2010-03-16 14:43:18 PDT
Mats, any chance you can work on this soon?
Comment 23 Mats Palmgren (:mats) 2010-04-07 14:24:27 PDT
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 (:mats) 2010-04-09 15:47:09 PDT
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.
Comment 25 Mats Palmgren (:mats) 2010-04-09 15:59:54 PDT
Created attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.
Comment 26 Mats Palmgren (:mats) 2010-04-09 16:01:00 PDT
Created attachment 438196 [details] [diff] [review]
Patch rev. 1, for trunk.
Comment 27 Boris Zbarsky [:bz] (Out June 25-July 6) 2010-04-09 21:14:52 PDT
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) (Exited; email my personal email if necessary) 2010-04-10 22:56:46 PDT
Thanks Mats!!! Can you attach a testcase here that when can land when it's safe to do so?
Comment 29 Reed Loden [:reed] (use needinfo?) 2010-04-13 13:37:42 PDT
As a security bug, this needs SR before landing, no?
Comment 30 Brendan Eich [:brendan] 2010-04-13 14:02:50 PDT
(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] (use needinfo?) 2010-04-13 14:08:35 PDT
(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 2010-04-13 17:06:04 PDT
looks good for reviews. Mats, please check this in ASAP.
Comment 33 Mats Palmgren (:mats) 2010-04-13 17:23:00 PDT
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 (:mats) 2010-04-14 11:08:19 PDT
Created attachment 439031 [details] [diff] [review]
non-working mochitest

Attempt to make a mochitest.  I can't make it crash!
Comment 35 Mats Palmgren (:mats) 2010-04-14 11:09:35 PDT
http://hg.mozilla.org/mozilla-central/rev/1b4a2702d2dd
Comment 36 Mike Beltzner [:beltzner, not reading bugmail] 2010-05-21 13:21:01 PDT
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] 2010-05-21 13:22:20 PDT
Comment on attachment 438195 [details] [diff] [review]
Patch rev. 1, for branches.

(oops, wrong flag!)
Comment 39 Mike Beltzner [:beltzner, not reading bugmail] 2010-06-01 05:00:29 PDT
Was this expected to cause a 2.2% SunSpider regression? 'cause it did:

http://mzl.la/9XcV5k
Comment 40 Mats Palmgren (:mats) 2010-06-01 11:21:40 PDT
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 2010-06-01 14:31:47 PDT
I think you should back out and see if the perf changes revert, just to be safe.
Comment 42 Mats Palmgren (:mats) 2010-06-01 15:10:30 PDT
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 (:mats) 2010-06-01 19:15:51 PDT
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] 2010-07-01 12:08:52 PDT
(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] 2010-07-01 12:20:28 PDT
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 [retired] 2010-07-01 12:33:55 PDT
(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] 2010-07-01 12:51:38 PDT
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] 2010-07-01 13:18:19 PDT
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 [retired] 2010-07-01 13:45:37 PDT
(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] 2010-07-01 13:49:58 PDT
We use "Verified" for trunk verification. Please use verified1.9.1 and verified1.9.2 keywords for branch verifications.

Thanks for the help!

Note You need to log in before you can comment on or make changes to this bug.