Closed Bug 872870 Opened 6 years ago Closed 6 years ago

Assertions and crash with undoManager, removing document.documentElement

Categories

(Core :: DOM: Core & HTML, defect, critical)

defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla24
Tracking Status
firefox21 --- disabled
firefox22 --- disabled
firefox23 --- disabled
firefox24 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: jruderman, Assigned: wchen)

References

Details

(4 keywords, Whiteboard: [adv-main24-])

Crash Data

Attachments

(3 files)

Attached file testcase
With:
  user_pref("dom.undo_manager.enabled", true);

###!!! ASSERTION: Inserting node that already has parent: '!aKid->GetParentNode()', file content/base/src/nsINode.cpp, line 1326

###!!! ASSERTION: aChild with prev sibling?: '!aChild->GetPreviousSibling()', file content/base/src/nsAttrAndChildArray.cpp, line 822

###!!! ASSERTION: Already have a document.  Unbind first!: '!GetCurrentDoc()', file content/base/src/Element.cpp, line 1016

###!!! ASSERTION: Shouldn't be here!: '!(IsNodeOfType(eCONTENT) && IsInDoc())', file nsINode.h, line 1495

###!!! ASSERTION: Bound to wrong document: 'aDocument == GetCurrentDoc()', file content/base/src/Element.cpp, line 1200

###!!! ASSERTION: No style context found!: 'mStyleContext', file nsStyleStructList.h, line 61

Crash [@ nsGfxScrollFrameInner::IsLTR() const]
Attached file stacks (gdb)
Related to bug 840877?
On Windows: bp-4158e848-e25d-47b5-9aea-b6a952130516.
Crash Signature: [@ NS_DebugBreak | nsINode::doInsertChildAt(nsIContent*, unsigned int, bool, nsAttrAndChildArray&) ] [@ nsCachedStyleData::GetStyleVisibility() ]
OS: Mac OS X → All
Hardware: x86_64 → All
Could you look at this, William?  Thanks.
Assignee: nobody → wchen
Marking sec-high because the assertions sound kind of bad.
Keywords: sec-high
Attachment #761022 - Flags: review?(ehsan) → review+
Comment on attachment 761022 [details] [diff] [review]
Use GetParentNode() instead of GetParent() in UndoManager.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?
Test case included could be used to reproduce the bug.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?
No.

Which older supported branches are affected by this flaw?
20+

If not all supported branches, which bug introduced the flaw?
617532

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?
No backports. The patch will likely apply cleanly on to previous branches.

How likely is this patch to cause regressions; how much testing does it need?
Not likely to cause regression. Existing tests is likely sufficient.

It should also be noted that this feature is hidden behind a pref and disabled by default.
Attachment #761022 - Flags: sec-approval?
Comment on attachment 761022 [details] [diff] [review]
Use GetParentNode() instead of GetParent() in UndoManager.

sec-approval+ since this is pref'd off but the test needs to be in a separate patch and only checked in after we ship the fix.
Attachment #761022 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/cb7bb0efab58
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [adv-main24-]
Group: core-security
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.