Last Comment Bug 583957 - "ASSERTION: killing mutation events" in nsMenuFrame::UpdateMenuType
: "ASSERTION: killing mutation events" in nsMenuFrame::UpdateMenuType
: assertion, testcase
Product: Core
Classification: Components
Component: XUL (show other bugs)
: Trunk
: x86 Mac OS X
: -- normal (vote)
: ---
Assigned To: Olli Pettay [:smaug]
: Neil Deakin
Depends on:
Blocks: 325861 344486
  Show dependency treegraph
Reported: 2010-08-02 19:25 PDT by Jesse Ruderman
Modified: 2011-02-09 09:08 PST (History)
10 users (show)
jruderman: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (423 bytes, text/html)
2010-08-02 19:25 PDT, Jesse Ruderman
no flags Details
assertion stack traces (17.56 KB, text/plain)
2010-08-02 19:26 PDT, Jesse Ruderman
no flags Details
patch (3.48 KB, patch)
2010-08-03 06:47 PDT, Olli Pettay [:smaug]
enndeakin: review+
neil: superreview+
dbaron: approval2.0+
dveditz: approval1.9.2.11+
dveditz: approval1.9.1.14+
Details | Diff | Splinter Review

Description Jesse Ruderman 2010-08-02 19:25:02 PDT
Created attachment 462305 [details]

###!!! ASSERTION: Want to fire mutation events, but it's not safe: '(aNode->IsNodeOfType(nsINode::eCONTENT) && static_cast<nsIContent*>(aNode)-> IsInNativeAnonymousSubtree()) || sScriptBlockerCount == sRemovableScriptBlockerCount', file content/base/src/nsContentUtils.cpp, line 3619

###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file content/base/src/nsContentUtils.cpp, line 6142

###!!! ASSERTION: This is unsafe! Fix the caller!: 'Error', file content/events/src/nsEventDispatcher.cpp, line 514

Security-sensitive because previous bugs with these assertions (bug 557398, bug 564461) were deemed to be likely-exploitable.
Comment 1 Jesse Ruderman 2010-08-02 19:26:10 PDT
Created attachment 462306 [details]
assertion stack traces
Comment 2 Olli Pettay [:smaug] 2010-08-03 03:10:10 PDT
Um, nsMenuFrame::AttributeChanged is quite wrong.
I blame hyatt.
Comment 3 Olli Pettay [:smaug] 2010-08-03 06:47:38 PDT
Created attachment 462387 [details] [diff] [review]

I think we need this. I'll test this some more.

if (mType != eMenuType_Normal) check shouldn't be needed since 
UpdateMenuSpecialState does that too.
Comment 4 Neil Deakin 2010-08-05 10:24:53 PDT
(In reply to comment #3)
> if (mType != eMenuType_Normal) check shouldn't be needed since 
> UpdateMenuSpecialState does that too.

I don't see UpdateMenuSpecialState checking mType.
Comment 5 Olli Pettay [:smaug] 2010-08-05 10:58:35 PDT
it has
if (mType != eMenuType_Radio) return
if (mType != eMenuType_Radio || ...) return
Comment 6 Olli Pettay [:smaug] 2010-08-05 11:02:25 PDT
In case of eMenuType_Normal, mChecked is probably changed,
but mChecked isn't really used with eMenuType_Normal.
Comment 7 Olli Pettay [:smaug] 2010-08-05 11:02:54 PDT
But if really wanted I could add the pretty useless mType != eMenuType_Normal back.
Comment 8 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-08-10 12:58:20 PDT
Neil needs to respond.
Comment 9 Olli Pettay [:smaug] 2010-08-12 05:21:22 PDT
Comment 10 Daniel Veditz [:dveditz] 2010-09-01 11:09:36 PDT
Is this bug a regression, or did we just not have those assertions on the branches?

In a 1.9.2 debug build I get
###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file /Users/daniel/dev/ff192/content/base/src/nsContentUtils.cpp, line 5245
###!!! ASSERTION: killing mutation events: 'nsContentUtils::IsSafeToRunScript()', file /Users/daniel/dev/ff192/content/base/src/nsContentUtils.cpp, line 5245

but not the scary first and third assertion from comment 0. On the other hand, if you're blaming hyatt (comment 2) I guess this is an old problem.
Comment 11 Olli Pettay [:smaug] 2010-09-01 12:29:00 PDT
We don't have all the same assertions on 1.9.2.
We should land this to branches too.
Sorry that I didn't ask approval, yet.
Comment 12 Olli Pettay [:smaug] 2010-09-01 12:29:28 PDT
I need to test whether the patch applies to branches.
Comment 13 Daniel Veditz [:dveditz] 2010-09-17 11:32:27 PDT
Comment on attachment 462387 [details] [diff] [review]

Approved for and, a=dveditz for release-drivers
Comment 15 Jesse Ruderman 2010-11-06 18:30:07 PDT

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