Closed
Bug 1218195
Opened 9 years ago
Closed 9 years ago
crash in nsMutationReceiver::NativeAnonymousChildListChange
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
VERIFIED
FIXED
mozilla45
| Tracking | Status | |
|---|---|---|
| firefox43 | --- | unaffected |
| firefox44 | --- | verified |
| firefox45 | --- | verified |
| b2g-v2.5 | --- | fixed |
People
(Reporter: Pike, Assigned: smaug)
References
()
Details
(Keywords: crash)
Crash Data
Attachments
(1 file)
|
(deleted),
patch
|
bzbarsky
:
review+
bgrins
:
feedback+
abillings
:
approval-mozilla-aurora+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is
report bp-582fffc2-73a5-4dfd-997f-b89b42151025.
=============================================================
At least one of the crashes I have in devtools these days.
|
Assignee | |
Updated•9 years ago
|
Flags: needinfo?(bgrinstead)
|
||
Comment 1•9 years ago
|
||
(In reply to Axel Hecht [:Pike] from comment #0)
> This bug was filed from the Socorro interface and is
> report bp-582fffc2-73a5-4dfd-997f-b89b42151025.
> =============================================================
>
> At least one of the crashes I have in devtools these days.
Any chance you can reproduce this crash and have STR?
Flags: needinfo?(l10n)
|
Reporter | |
Comment 3•9 years ago
|
||
I'm usually debugging my local install of c9, https://cloud9-sdk.readme.io/. I'm using that with my own plugin, but that's rather tedious to set up so far.
I'm mostly using the DOM inspector and $0 js for the panels on the left side.
Not really STR, but that's the context.
Flags: needinfo?(l10n)
|
|
||
Comment 4•9 years ago
|
||
From https://bugzilla.mozilla.org/show_bug.cgi?id=1213270#c4 I came up with STR:
1) Open data:text/html,<style>body::before{ content:'foo'; } body::after{ content:'baz'; }</style><body> bar </body>
2) Open inspector
3) Edit as HTML on body and add <div></div> to the end of it
Wait a second. If it doesn't crash then try again (add <div></div> again to the end, or remove <div></div> from the end)
|
|
||
Comment 5•9 years ago
|
||
Still debugging, but I'm seeing a bunch of assertions when following STR in a debug build:
[96343] ###!!! ASSERTION: Still generating MutationRecords?: 'mCurrentMutations.IsEmpty()', file fx-team-debug/dom/base/nsDOMMutationObserver.cpp, line 812
Then get a lot of spew on this one when hovering nodes in the markup view afterwords:
[96343] ###!!! ASSERTION: Unexpected MutationRecord type!: 'mCurrentMutations[last]->mType == aType', file fx-team-debug/dom/base/nsDOMMutationObserver.cpp, line 932
[96343] ###!!! ASSERTION: Unexpected MutationRecord type!: 'mCurrentMutations[last]->mType == aType', file fx-team-debug/dom/base/nsDOMMutationObserver.cpp, line 932
[96343] ###!!! ASSERTION: Unexpected MutationRecord type!: 'mCurrentMutations[last]->mType == aType', file fx-team-debug/dom/base/nsDOMMutationObserver.cpp, line 932
Pausing inside nsDOMMutationObserver::CurrentRecord, aType is "nativeAnonymousChildList" but can't read mCurrentMutations[last]->mType.
|
|
||
Comment 6•9 years ago
|
||
What's weird is that the crash / assertions don't reproduce for me with this page (even with devtools open and navigating / clicking around the markup view while it's running):
data:text/html,<style>.el::before{ content:'foo'; } .el::after{ content:'baz'; }</style><body><div class="el"> bar </div><script>setInterval(function() {document.querySelector(".el").innerHTML = document.querySelector(".el").innerHTML + "<div></div>";}, 1000)</script></body>
Same if outerHTML is edited instead of innerHTML. So something different is happening when the edit happens from the toolbox.
|
|
||
Comment 7•9 years ago
|
||
I've given up for the day. Good news is it's easy to reproduce Comment 5. It seems that LeaveMutationHandling is the place where mCurrentMutations is supposed to be emptied out (besides Disconnect). This function is called by IMPL_MUTATION_NOTIFICATION in nsNodeUtils, but not until after HandleMutation happens (at least in this case).
When I try inspect the contents of mCurrentMutations when this bad case happens (right after editing HTML and conditionally breaking on '!mCurrentMutations.IsEmpty()' in HandleMutation) I see:
> p mCurrentMutations
(nsAutoTArray<nsDOMMutationRecord *, 4>) $9 = {
[0] = 0x0000000000000000
[1] = 0x0000000000000000
}
|
|
||
Updated•9 years ago
|
|
|
Assignee | |
Updated•9 years ago
|
Assignee: nobody → bugs
|
|
Assignee | |
Updated•9 years ago
|
Group: dom-core-security
|
|
Assignee | |
Comment 8•9 years ago
|
||
bgrins, could you still verify this works for you.
Attachment #8679790 -
Flags: feedback?(bgrinstead)
|
|
||
Comment 9•9 years ago
|
||
Comment on attachment 8679790 [details] [diff] [review]
patch
Review of attachment 8679790 [details] [diff] [review]:
-----------------------------------------------------------------
Checked on both debug and opt builds, assertions are gone in debug and can't reproduce the crash with this patch applied. Also just did a check to make sure anon mutations were observed on a normal page and they seem to work fine.
Attachment #8679790 -
Flags: feedback?(bgrinstead) → feedback+
|
|
Assignee | |
Comment 10•9 years ago
|
||
Comment on attachment 8679790 [details] [diff] [review]
patch
So the setup is a bit complicated because of nested DOM mutations, which normally, from MutationObserver point of view, doesn't ever happen (or when it happens in case of innerHTML and such, we care only about the outermost change),
but for devtools we added support for getting MutationRecords when native anonymous children of a node has been added or removed.
And nested mutations can happen for example when a node which has ::before is removed from document.
So the issue is that nsDOMMutationObserver::LeaveMutationHandling() expects all the MutationObservers which have observed a mutation at nested level X to have observed changes also in levels 1 - (X - 1) to clear nsDOMMutationObserver::mCurrentMutations properly.
So, in order to achieve that, just call AddCurrentlyHandlingObserver recursively with level - 1.
Note, level should in practice be 1 always when web pages are using MutationObserver, so no performance regressions there.
Attachment #8679790 -
Flags: review?(jonas)
|
||
Comment 11•9 years ago
|
||
Comment on attachment 8679790 [details] [diff] [review]
patch
r=me
Attachment #8679790 -
Flags: review?(jonas) → review+
|
|
Assignee | |
Comment 12•9 years ago
|
||
Comment on attachment 8679790 [details] [diff] [review]
patch
[Security approval request comment]
How easily could an exploit be constructed based on the patch? Not easily, and the feature which triggers this crash is chrome only
Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
Comment will be "Bug 1218195, mark MutationObserver as observing in all the nested DOM mutations, r=bz"
Which older supported branches are affected by this flaw?
So apparently aurora is already 44, and bug 1034110 revealed this, so aurora and trunk.
I haven't found any way to trigger this without bug 1034110.
If not all supported branches, which bug introduced the flaw?
The code is old, but bug 1034110 revealed the bug.
Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
The same patch should apply to aurora(44) and trunk.
How likely is this patch to cause regressions; how much testing does it need?
Should be rather safe
Attachment #8679790 -
Flags: sec-approval?
Attachment #8679790 -
Flags: approval-mozilla-aurora?
|
||
Comment 13•9 years ago
|
||
Comment on attachment 8679790 [details] [diff] [review]
patch
We need a security rating on this but this seems safe to approve for Aurora and Trunk. If you check it in before we branch, we won't need it on beta.
Attachment #8679790 -
Flags: sec-approval?
Attachment #8679790 -
Flags: sec-approval+
Attachment #8679790 -
Flags: approval-mozilla-aurora?
Attachment #8679790 -
Flags: approval-mozilla-aurora+
|
|
||
Comment 14•9 years ago
|
||
(In reply to Al Billings [:abillings] from comment #13)
> If you check it in before we branch, we won't need it on beta.
The branch happened early this time, so Aurora is already 44 as of yesterday
|
|
Assignee | |
Comment 15•9 years ago
|
||
|
||
Comment 16•9 years ago
|
||
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
|
||
Comment 17•9 years ago
|
||
|
||
Updated•9 years ago
|
Group: dom-core-security → core-security-release
|
|
||
Updated•9 years ago
|
Group: core-security-release
|
|
||
Comment 18•9 years ago
|
||
| bugherder uplift | ||
status-b2g-v2.5:
--- → fixed
|
|
||
Comment 19•9 years ago
|
||
| bugherder uplift | ||
|
||
Updated•9 years ago
|
Flags: qe-verify+
|
||
Comment 20•9 years ago
|
||
Reproduced with Nightly 44.0a1 under Mac OS X 10.11 → bp-6377a515-65a3-4e68-b060-e80042160104
Verified fixed with Firefox 44 beta 4 (Build ID: 20151228134903) and latest Developer Edition 45.0a2 (from 2016-01-03), under Mac OS X 10.11.
No new reports in Socorro for this signature after pushing this fix: https://crash-stats.mozilla.com/report/list?range_unit=days&range_value=28&signature=nsMutationReceiver%3A%3ANativeAnonymousChildListChange#tab-reports
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•