872870 - Assertions and crash with undoManager, removing document.documentElement

Status: RESOLVED FIXED

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

Description

[Jesse Ruderman]

Attachment: 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]

Comment 1

[Jesse Ruderman]

Attachment: stacks (gdb)

Comment 2

[Jesse Ruderman]

Related to bug 840877?

Comment 3

[Scoobidiver (away)]

On Windows: bp-4158e848-e25d-47b5-9aea-b6a952130516.

Comment 4

[Andrew McCreight [:mccr8]]

Could you look at this, William?  Thanks.

Comment 5

[Andrew McCreight [:mccr8]]

Marking sec-high because the assertions sound kind of bad.

Comment 6

[William Chen [:wchen]]

Attachment: Use GetParentNode() instead of GetParent() in UndoManager.

Comment 7

[William Chen [:wchen]]

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.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

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.

Comment 9

[William Chen [:wchen]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb7bb0efab58

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cb7bb0efab58