Last Comment Bug 529087 - Crash [@ nsXBLBinding::AllowScripts]
: Crash [@ nsXBLBinding::AllowScripts]
Status: RESOLVED FIXED
[sg:critical?][ccbr][critsmash:patch]...
: crash
Product: Core
Classification: Components
Component: XBL (show other bugs)
: unspecified
: All All
-- critical (vote)
: ---
Assigned To: Jonas Sicking (:sicking) No longer reading bugmail consistently
:
: Andrew Overholt [:overholt]
Mentors:
: 570880 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-11-16 12:18 PST by Mats Palmgren (:mats)
Modified: 2011-06-13 10:01 PDT (History)
13 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1+
.7+
.7-fixed
.11+
.11-fixed


Attachments
Patch to fix (1.92 KB, patch)
2010-04-26 18:08 PDT, Jonas Sicking (:sicking) No longer reading bugmail consistently
bugs: review+
dveditz: approval1.9.2.7+
dveditz: approval1.9.1.11+
Details | Diff | Splinter Review

Description User image Mats Palmgren (:mats) 2009-11-16 12:18:39 PST
It's ranked #276 in the 3.5.5 (past 7 days) top crash list:
http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.5.5/7
with 389 crashes, 380 on Win, 9 on OSX.

stack:
nsXBLBinding::AllowScripts               content/xbl/src/nsXBLBinding.cpp:1380
nsXBLBinding::ExecuteAttachedHandler     content/xbl/src/nsXBLBinding.cpp:977
nsXBLBinding::ExecuteAttachedHandler     content/xbl/src/nsXBLBinding.cpp:975
nsRunnableMethod<nsFileUploadContentStream>::Run   xpcom/nsThreadUtils.h:278
nsContentUtils::RemoveScriptBlocker   content/base/src/nsContentUtils.cpp:4344
PresShell::DoFlushPendingNotifications	layout/base/nsPresShell.cpp:4842
PresShell::FlushPendingNotifications	layout/base/nsPresShell.cpp:4798
nsDocument::FlushPendingNotifications	content/base/src/nsDocument.cpp:6375
nsQueryReferent::operator	nsWeakReference.cpp:88
nsComputedDOMStyle::GetPropertyCSSValue layout/style/nsComputedDOMStyle.cpp:349
xul.dll@0x8c4807	

There are also 2 crashes on trunk within the past week, so it appears
the bug hasn't been fixed yet.
Comment 1 User image Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 12:40:53 PST
Line 1380 is:

1380   nsIDocument* doc = mBoundElement->GetOwnerDoc();

right?
Comment 2 User image Mats Palmgren (:mats) 2009-11-16 12:49:25 PST
Yes.

This crash might be what bug 307562 was about?
Comment 3 User image Boris Zbarsky [:bz] (still a bit busy) 2009-11-16 12:56:14 PST
Hmm.  mBoundElement is a weak ref, and is never unset, as far as I can tell.  How's that supposed to work?
Comment 4 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-13 15:20:08 PDT
Taking this. ETA end of next week.
Comment 5 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-26 18:08:41 PDT
Created attachment 441671 [details] [diff] [review]
Patch to fix

I think this should do it.

Usually we're protected by the fact that nodes are always UnbindFromTree'ed from their document before they die. And unbinding removes any xbl bindings.

However nodes with NODE_FORCE_XBL_BINDINGS set can have xbl bindings without being in a document, and thus can die while still having bindings.

The change in this patch to nsNodeUtils takes care of this. This should ensure that mBoundElement never becomes dangling. This should be enough to take care of the security issues here.

However we also need to deal with that we can get a null mBoundElement while running nested constructors. Otherwise we would crash with a nullpointer deref and still have the topcrash reported in comment 0. The change to nsXBLBinding should take care of that.
Comment 6 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-04-26 18:09:11 PDT
I think I can come up with tests for this. Asking for review in the meantime though.
Comment 7 User image Johnny Stenback (:jst, jst@mozilla.com) 2010-05-11 13:19:57 PDT
Olli, can you review this security bug fix?
Comment 8 User image Olli Pettay [:smaug] 2010-05-11 14:50:14 PDT
Yes, I will later this week. Sorry for the delay.
Comment 9 User image Olli Pettay [:smaug] 2010-05-11 15:01:01 PDT
Comment on attachment 441671 [details] [diff] [review]
Patch to fix

Ok, makes sense.

Though, if BindingManager() may return null, we should rename it to GetBindingManager().
In theory it may return null, but I think we need to change ownership back to what it used to be (so that we can have strong parent pointer - which is a different bug.).
Comment 10 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-11 15:04:37 PDT
I think it can only return null during cycle collection. But yes, ideally we should improve the situation there.
Comment 11 User image Damon Sicore (:damons) 2010-05-18 13:19:52 PDT
Can we get this landed?
Comment 12 User image Olli Pettay [:smaug] 2010-05-19 01:17:40 PDT
Security bugs need also sr.
Comment 13 User image Olli Pettay [:smaug] 2010-05-19 01:27:41 PDT
Comment on attachment 441671 [details] [diff] [review]
Patch to fix

Or if peterv is away for some time, perhaps jst could sr.
Comment 14 User image Mats Palmgren (:mats) 2010-05-19 02:45:33 PDT
> Security bugs need also sr.

Not anymore. That rule was removed from the policy.
http://www.mozilla.org/hacking/reviewers.html
(the policy change was announced in mozilla.governance)
Comment 15 User image Olli Pettay [:smaug] 2010-05-19 03:00:09 PDT
Comment on attachment 441671 [details] [diff] [review]
Patch to fix

Huh, we're changing rules faster than people learn them.
Comment 16 User image Damon Sicore (:damons) 2010-05-25 13:16:32 PDT
Can we land this?
Comment 17 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-26 15:41:01 PDT
Checked in. Thanks for the review!

http://hg.mozilla.org/mozilla-central/rev/60fda31d4012
Comment 18 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-05-26 16:13:21 PDT
Hrm.. also checked in bustage fix due to merge.

http://hg.mozilla.org/mozilla-central/rev/adf3e43ffdd9
Comment 19 User image Johnny Stenback (:jst, jst@mozilla.com) 2010-06-01 12:58:18 PDT
This should be marked fixed now, no?
Comment 20 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-06-01 13:33:23 PDT
Indeed! Somehow missed that in all the flag-setting.
Comment 21 User image Daniel Veditz [:dveditz] 2010-06-02 10:49:18 PDT
Comment on attachment 441671 [details] [diff] [review]
Patch to fix

Approved for 1.9.2.5 and 1.9.1.11, a=dveditz for release-drivers
Comment 22 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-06-08 21:52:43 PDT
*** Bug 570880 has been marked as a duplicate of this bug. ***
Comment 23 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2010-06-24 21:08:13 PDT
Fixed on 1.9.2 and 1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/446e17503f87
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/15dccae78d7e
Comment 24 User image Al Billings [:abillings] 2010-07-01 16:21:05 PDT
Are there STR that can be used for verification of this fix on 1.9.1 and 1.9.2?
Comment 25 User image Al Billings [:abillings] 2010-07-15 16:24:47 PDT
Jonas? Mats? Any reply?

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