Bugzilla@Mozilla – Bug 529087
Crash [@ nsXBLBinding::AllowScripts]
Last modified: 2010-09-27 18:04:59 PDT
Summon comment box
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.
Line 1380 is: 1380 nsIDocument* doc = mBoundElement->GetOwnerDoc(); right?
Yes. This crash might be what bug 307562 was about?
Hmm. mBoundElement is a weak ref, and is never unset, as far as I can tell. How's that supposed to work?
Taking this. ETA end of next week.
Created attachment 441671 [details] [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.
I think I can come up with tests for this. Asking for review in the meantime though.
Olli, can you review this security bug fix?
Yes, I will later this week. Sorry for the delay.
Comment on attachment 441671 [details] [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.).
I think it can only return null during cycle collection. But yes, ideally we should improve the situation there.
Can we get this landed?
Security bugs need also sr.
Comment on attachment 441671 [details] [review] Patch to fix Or if peterv is away for some time, perhaps jst could sr.
> 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 on attachment 441671 [details] [review] Patch to fix Huh, we're changing rules faster than people learn them.
Can we land this?
Checked in. Thanks for the review! http://hg.mozilla.org/mozilla-central/rev/60fda31d4012
Hrm.. also checked in bustage fix due to merge. http://hg.mozilla.org/mozilla-central/rev/adf3e43ffdd9
This should be marked fixed now, no?
Indeed! Somehow missed that in all the flag-setting.
Comment on attachment 441671 [details] [review] Patch to fix Approved for 1.9.2.5 and 1.9.1.11, a=dveditz for release-drivers
*** Bug 570880 has been marked as a duplicate of this bug. ***
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
Are there STR that can be used for verification of this fix on 1.9.1 and 1.9.2?
Jonas? Mats? Any reply?