764591 - crash in nsStyleSet::ReparentStyleContext when clicking / hovering NoScript icon in Add-on Toolbar

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

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]]

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]]

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]]

OK, I think bug 763424 is innocent -- I backed it out locally, and I'm still crashing.

Comment 4

[Daniel Holbert [:dholbert]]

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]]

(I backed them out locally, I mean)

Comment 6

[Daniel Holbert [:dholbert]]

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 [:bzbarsky]]

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 [:ma1]]

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 [:metasieben]]

(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]

A crash also occurs when using the right-click menu (Right click -> NoScript).

Comment 11

[Scoobidiver (away)]

(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]]

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 [:bzbarsky]]

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 [:bzbarsky]]

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 [:bzbarsky]]

Then there are other asserts about primary frames being confused, etc....

Comment 16

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Aha!  Looking at the noscript code links from comment 14, there's a document fragment involved!

Comment 18

[Boris Zbarsky [:bzbarsky]]

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 [:bzbarsky]]

Attachment: 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/[e-mail]-cced24ceafe2/ at some point later today...

Comment 20

[Daniel Holbert [:dholbert]]

(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][[e-mail]]]

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 [:bzbarsky]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/d8bc9c5cdc4a with that comment fixed.

Comment 23

[Robert Kaiser]

Just for your amusement, this is roughly 50% of all crashes on Nightly yesterday.

Comment 24

[Boris Zbarsky [:bzbarsky]]

Which just goes to show that we should remove support for XBL destructors, preferably yesterday...

Comment 25

[Gabriela [:gaby2300]]

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]]

https://hg.mozilla.org/mozilla-central/rev/d8bc9c5cdc4a