"ASSERTION: killing mutation events" in nsMenuFrame::UpdateMenuType

RESOLVED FIXED

Status

()

Core
XUL
RESOLVED FIXED
7 years ago
7 years ago

People

(Reporter: Jesse Ruderman, Assigned: smaug)

Tracking

(Blocks: 2 bugs, {assertion, testcase})

Trunk
x86
Mac OS X
assertion, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking1.9.2 .11+, status1.9.2 .11-fixed, blocking1.9.1 .14+, status1.9.1 .14-fixed)

Details

(Whiteboard: [sg:critical?][critsmash:patch])

Attachments

(3 attachments)

423 bytes, text/html
Details
17.56 KB, text/plain
Details
3.48 KB, patch
Neil Deakin (not available until Aug 9)
: review+
neil@parkwaycc.co.uk
: superreview+
dbaron
: approval2.0+
Details | Diff | Splinter Review
(Reporter)

Description

7 years ago
Created attachment 462305 [details]
testcase

###!!! 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.
(Reporter)

Comment 1

7 years ago
Created attachment 462306 [details]
assertion stack traces
(Assignee)

Updated

7 years ago
Assignee: nobody → Olli.Pettay
(Assignee)

Comment 2

7 years ago
Um, nsMenuFrame::AttributeChanged is quite wrong.
I blame hyatt.
(Assignee)

Comment 3

7 years ago
Created attachment 462387 [details] [diff] [review]
patch

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.
(Assignee)

Updated

7 years ago
Attachment #462387 - Flags: superreview?(neil)
Attachment #462387 - Flags: review?(enndeakin)

Updated

7 years ago
Attachment #462387 - Flags: superreview?(neil) → superreview+
(Reporter)

Updated

7 years ago
Whiteboard: [sg:critical?]

Updated

7 years ago
Whiteboard: [sg:critical?] → [sg:critical?][critsmash:investigating]

Updated

7 years ago
Whiteboard: [sg:critical?][critsmash:investigating] → [sg:critical?][critsmash:patch]
(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.
(Assignee)

Comment 5

7 years ago
it has
if (mType != eMenuType_Radio) return
and
if (mType != eMenuType_Radio || ...) return
(Assignee)

Comment 6

7 years ago
In case of eMenuType_Normal, mChecked is probably changed,
but mChecked isn't really used with eMenuType_Normal.
(Assignee)

Comment 7

7 years ago
But if really wanted I could add the pretty useless mType != eMenuType_Normal back.
Neil needs to respond.
Whiteboard: [sg:critical?][critsmash:patch] → [sg:critical?][critsmash:patch][needs review]
Attachment #462387 - Flags: review?(enndeakin) → review+
(Assignee)

Updated

7 years ago
blocking1.9.1: --- → ?
blocking1.9.2: --- → ?
(Assignee)

Updated

7 years ago
Attachment #462387 - Flags: approval2.0?

Updated

7 years ago
blocking1.9.1: ? → needed
blocking1.9.2: ? → needed
status1.9.1: --- → wanted
status1.9.2: --- → wanted
Attachment #462387 - Flags: approval2.0? → approval2.0+
(Assignee)

Updated

7 years ago
Whiteboard: [sg:critical?][critsmash:patch][needs review] → [sg:critical?][critsmash:patch]
(Assignee)

Comment 9

7 years ago
http://hg.mozilla.org/mozilla-central/rev/e4e653196488
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED
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.
(Assignee)

Comment 11

7 years ago
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.
(Assignee)

Comment 12

7 years ago
I need to test whether the patch applies to branches.
blocking1.9.1: needed → .13+
blocking1.9.2: needed → .10+
(Assignee)

Updated

7 years ago
Attachment #462387 - Flags: approval1.9.2.10?
Attachment #462387 - Flags: approval1.9.1.13?
Comment on attachment 462387 [details] [diff] [review]
patch

Approved for 1.9.2.11 and 1.9.1.14, a=dveditz for release-drivers
Attachment #462387 - Flags: approval1.9.2.11?
Attachment #462387 - Flags: approval1.9.2.11+
Attachment #462387 - Flags: approval1.9.1.14?
Attachment #462387 - Flags: approval1.9.1.14+
(Assignee)

Comment 14

7 years ago
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/6a2ae85c5dbc
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/528a466700d8
status1.9.1: wanted → .14-fixed
status1.9.2: wanted → .11-fixed
Group: core-security
(Reporter)

Comment 15

7 years ago
Crashtest: http://hg.mozilla.org/mozilla-central/rev/ea9ed8b0fe1b
Flags: in-testsuite+
Blocks: 592424
No longer blocks: 592424
You need to log in before you can comment on or make changes to this bug.