Last Comment Bug 764591 - crash in nsStyleSet::ReparentStyleContext when clicking / hovering NoScript icon in Add-on Toolbar
: crash in nsStyleSet::ReparentStyleContext when clicking / hovering NoScript i...
Status: RESOLVED FIXED
[workaround: View|Toolbars|Customize,...
: crash, regression, topcrash
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 16 Branch
: All All
: -- critical with 2 votes (vote)
: mozilla16
Assigned To: Boris Zbarsky [:bz] (still a bit busy)
:
:
Mentors:
: 765502 (view as bug list)
Depends on:
Blocks: 724235 736695
  Show dependency treegraph
 
Reported: 2012-06-13 14:30 PDT by Daniel Holbert [:dholbert]
Modified: 2012-06-19 20:32 PDT (History)
23 users (show)
bzbarsky: in‑testsuite?
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-


Attachments
Handle removals of kids of document fragments being inserted under a separate scriptblocker, just like we do with the single-node insert case, so their XBL binding teardown happens before insert. (11.40 KB, patch)
2012-06-15 12:25 PDT, Boris Zbarsky [:bz] (still a bit busy)
bugs: review+
Details | Diff | Splinter Review

Description Daniel Holbert [:dholbert] 2012-06-13 14:30:12 PDT
STR:
 1. Install NoScript from http://noscript.net/getit.
       (Restart Firefox to complete installation.)

 2. "View | Toolbars | Add-on Bar" to make Add-on Bar visible.
 3. "View | Toolbars | Customize"
 4. Drag NoScript icon from URLbar area to Add-on Bar; then, close "Customize" dialog.
 5. Click NoScript icon in Add-on Bar.

Actual results: CRASH

This is new in today's m-c nightly.

m-c mozregression range:
http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2012-06-12&enddate=2012-06-13

m-i mozregression range:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f08886a8cf22&tochange=4a4519b018e9

This bug was filed from the Socorro interface and is 
report bp-6114342e-2eb8-4591-a0f9-850542120613 .
=============================================================
Comment 1 Daniel Holbert [:dholbert] 2012-06-13 14:36:05 PDT
At first glance, this cset for bug 763424 looks like the most related-looking layout change in the m-i regression range:
  http://hg.mozilla.org/integration/mozilla-inbound/rev/f4ff27e63666
(touches nsFrameManager::ReResolveStyleContext, which occurs multiple times in the crash stack)

Tentatively blaming that, to start with.
Comment 2 Daniel Holbert [:dholbert] 2012-06-13 14:42:21 PDT
Quoting Scoobidiver from bug 724235 comment #1:
> There's a spike in crashes (about 50 crashes per hour!) starting in
> 16.0a1/20120613.
Comment 3 Daniel Holbert [:dholbert] 2012-06-13 15:00:03 PDT
OK, I think bug 763424 is innocent -- I backed it out locally, and I'm still crashing.
Comment 4 Daniel Holbert [:dholbert] 2012-06-13 15:14:36 PDT
In fact, it's a regression from another bug in that same push -- bug 736695.

I backed out its 3 csets (the top 3 in http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=f4ff27e63666 ) and that fixes this crash.
Comment 5 Daniel Holbert [:dholbert] 2012-06-13 15:17:17 PDT
(I backed them out locally, I mean)
Comment 6 Daniel Holbert [:dholbert] 2012-06-13 15:27:13 PDT
To be more specific: this is fixed by a local backout of the final cset from bug 736695:
 http://hg.mozilla.org/integration/mozilla-inbound/rev/ca716959ca2b
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-06-13 16:50:52 PDT
Yeah, the final cset is the dangerous one.  The previous ones were basically unobservable (until the final cset) refactoring.

I'll look into this.
Comment 8 Giorgio Maone [:mao] 2012-06-14 05:02:26 PDT
Side note for NoScript users coming here after a crash: as a work-around you can just use the contextual "Customize" menu on the add-on bar and move the NoScript icon to the navigation bar.
Comment 9 matthias koplenig 2012-06-14 06:25:00 PDT
(In reply to Giorgio Maone from comment #8)
> Side note for NoScript users coming here after a crash: as a work-around you
> can just use the contextual "Customize" menu on the add-on bar and move the
> NoScript icon to the navigation bar.

hmm. somehow i got this crash today while looking at noscript's context-menu.
crashreport: b76a4e04-a32f-4596-93bb-a37f92120614
Comment 10 malte.schuetze 2012-06-14 09:40:49 PDT
A crash also occurs when using the right-click menu (Right click -> NoScript).
Comment 11 Scoobidiver (away) 2012-06-15 01:11:39 PDT
(In reply to Daniel Holbert [:dholbert] from comment #4)
> In fact, it's a regression from another bug in that same push -- bug 736695.
This bug is ten times crashier than bug 736695. Can someone backout the culprit changeset if nobody has a quick fix?
Comment 12 Spencer Selander [greenknight] 2012-06-15 04:59:57 PDT
I triggered this several times hovering the NoScript add-on bar icon, but I had the same crash twice without going close to the add-on bar tonight, trying to load the Facebook app My Calendar - Birthdays:

http://crash-stats.mozilla.com/report/index/bp-7b83c4cf-0484-4b91-829d-572b42120615
http://crash-stats.mozilla.com/report/index/bp-2430093d-c330-4603-8dcf-32c842120615

Thanks for the workaround, Giorgio.
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 09:11:38 PDT
For what it's worth, I haven't gotten this to crash yet....  I do see some assertions, but no crash.  I guess I'll aim to fix the assertions and hope that fixes the crash for people.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 09:19:15 PDT
So the first thing that goes wrong is that a timer fires ["chrome://noscript/content/noscriptOverlay.js":98] and calls some script ["chrome://noscript/content/noscriptOverlay.js":38] which calls openPopup on a popup boxobject.  This fires a popupshowing event, which runs more script ["chrome://noscript/content/noscriptOverlay.js":76] which inserts a <xul:menuitem> ["chrome://noscript/content/noscriptOverlay.js":912].

We go to construct frames for this <xul:menuitem>, find an XBL-bound kid under it, but that kid thinks it's not in a document.  That's the first assertion...
Comment 15 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 09:22:30 PDT
Then there are other asserts about primary frames being confused, etc....
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 10:30:53 PDT
Fwiw, I would dearly love a smaller testcase that still shows the problem.  That would make this a heck of a lot easier to debug...  Right now I'm getting dozens of calls to the code involved every time I hover over the button.  :(
Comment 17 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 11:14:09 PDT
Aha!  Looking at the noscript code links from comment 14, there's a document fragment involved!
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 12:06:48 PDT
So I bet that's it.  The removals from the document fragment happen under the insertion scriptblocker, so the binding teardowns happen async _after_ all those nodes are inserted into the document, which is bad.
Comment 19 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 12:25:22 PDT
Created attachment 633625 [details] [diff] [review]
Handle removals of kids of document fragments being inserted under a separate scriptblocker, just like we do with the single-node insert case, so their XBL binding teardown happens before insert.

Well, this fixes the assertions for me.  Can someone who is seeing crashes give this a shot?  There should be try builds at https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/bzbarsky@mozilla.com-cced24ceafe2/ at some point later today...
Comment 20 Daniel Holbert [:dholbert] 2012-06-15 12:36:04 PDT
(In reply to Boris Zbarsky (:bz) from comment #19)
> Well, this fixes the assertions for me.  Can someone who is seeing crashes
> give this a shot?

Nice! This fixes all assertions & crashes in my local debug build.  Tested w/ icon in NavBar &  Add-on bar, and also tested the NoScript submenu in page-rightclick-contextmenu.  No assertions or crashes.
Comment 21 Olli Pettay [:smaug] (reviewing overload) 2012-06-15 14:18:58 PDT
Comment on attachment 633625 [details] [diff] [review]
Handle removals of kids of document fragments being inserted under a separate scriptblocker, just like we do with the single-node insert case, so their XBL binding teardown happens before insert.


>@@ -4348,63 +4450,42 @@ nsINode::ReplaceOrInsertBefore(bool aRep
>   }
> 
>   /*
>    * Check if we're inserting a document fragment. If we are, we need
>    * to remove the children of the document fragment and add them
>    * individually (i.e. we don't add the actual document fragment).
Fix this comment, since the children have been removed already.
Comment 22 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 14:59:52 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/d8bc9c5cdc4a with that comment fixed.
Comment 23 Robert Kaiser 2012-06-15 15:40:01 PDT
Just for your amusement, this is roughly 50% of all crashes on Nightly yesterday.
Comment 24 Boris Zbarsky [:bz] (still a bit busy) 2012-06-15 15:45:01 PDT
Which just goes to show that we should remove support for XBL destructors, preferably yesterday...
Comment 25 Gabriela [:gaby2300] 2012-06-15 16:51:27 PDT
I can reproduce today using Windows 7 and Nightly. The workaround didn't work for me.

Crash report:
https://crash-stats.mozilla.com/report/index/bp-01343147-3d5d-4601-8230-1d9492120615
Comment 26 Ryan VanderMeulen [:RyanVM] 2012-06-16 06:50:45 PDT
https://hg.mozilla.org/mozilla-central/rev/d8bc9c5cdc4a
Comment 27 Daniel Holbert [:dholbert] 2012-06-16 11:07:25 PDT
*** Bug 765502 has been marked as a duplicate of this bug. ***
Comment 28 Alice0775 White 2012-06-16 11:12:39 PDT
*** Bug 765502 has been marked as a duplicate of this bug. ***

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