"ASSERTION: aDocument must be current doc of aParent" with lots of execCommand

RESOLVED FIXED in mozilla2.0b1

Status

()

Core
Editor
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Jesse Ruderman, Assigned: Ehsan)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla2.0b1
x86
Mac OS X
assertion, testcase
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 1 obsolete attachment)

(Reporter)

Description

8 years ago
Created attachment 451797 [details]
testcase

Loading and unloading the testcase trigger lots of assertions, including:

###!!! ASSERTION: transaction did not execute properly: '(NS_SUCCEEDED(result))', file /builds/slave/mozilla-central-macosx-debug/build/editor/libeditor/base/nsEditor.cpp, line 654

###!!! ASSERTION: anonymous nodes should not be in child lists: '!aOldChild->IsRootOfAnonymousSubtree()', file /builds/slave/mozilla-central-macosx-debug/build/layout/base/nsCSSFrameConstructor.cpp, line 11517

###!!! ASSERTION: aDocument must be current doc of aParent: '!aParent || aDocument == aParent->GetCurrentDoc()', file /builds/slave/mozilla-central-macosx-debug/build/content/base/src/nsGenericElement.cpp, line 2799

The last one isn't covered by any other bugs.
(Assignee)

Comment 1

8 years ago
Created attachment 451829 [details] [diff] [review]
Part 1: Don't attempt to inject nodes for type-in state where the container can't accept child nodes

The root cause of the problem here is that when we have a type-in state (which is set by the forecolor command, among others), we don't correctly check to see if the active node in the document can accept children on the next editor transaction.  Therefore, when the next command (insertunorderedlist, among others) attempts to set the type-in state, we would end up with the following content tree:

      input@0x204c0620 id="i" intrinsicstate=[00020040] flags=[02200383] primaryframe=0x14d8640 refcount=27<
        span@0x14eb74c0 _moz_dirty="" style="color: green;" intrinsicstate=[00020000] flags=[02200901] primaryframe=0x0 refcount=6<
          Text@0x14ee0670 intrinsicstate=[00020000] flags=[00000101] primaryframe=0x0 refcount=9<>
        >
      >

which is invalid, and breaks tons of assumptions elsewhere in the code.

This patch adds the check in question.  With this patch, the only assertion when loading the test case is:

###!!! ASSERTION: transaction did not execute properly: '(NS_SUCCEEDED(result))', file /Users/ehsanakhgari/moz/src/editor/libeditor/base/nsEditor.cpp, line 654

More patches to follow...
Assignee: nobody → ehsan
Status: NEW → ASSIGNED
Attachment #451829 - Flags: review?(roc)
(Assignee)

Comment 2

8 years ago
Created attachment 452331 [details] [diff] [review]
Part 2: Make sure not to exceed the editor's root element when promoting ranges

Another problem I discovered while working on this is that the HTML editor could exceed the editor's root element when promoting ranges, because it does an equality check against the root element, but the element at hand might be higher in the content hierarchy already (e.g. when the selection starts and ends on the <html> element, for example.
Attachment #452331 - Flags: review?(roc)
(Assignee)

Updated

8 years ago
Attachment #452331 - Attachment description: Make sure not to exceed the editor's root element when promoting ranges → Part 2: Make sure not to exceed the editor's root element when promoting ranges
(Assignee)

Comment 3

8 years ago
Created attachment 452340 [details] [diff] [review]
Part 3: Improve error checking in nsEditor::GetStartNodeAndOffset using NS_ENSURE macros
Attachment #452340 - Flags: review?(roc)
(Assignee)

Comment 4

8 years ago
Created attachment 452353 [details] [diff] [review]
Part 4: Don't attempt to check empty selections, after all, they're empty

TypeInState's selection listener tries to get ranges for non-collapsed selections, but doesn't consider empty selections, which casuses it to fail.
Attachment #452353 - Flags: review?(roc)
(Assignee)

Comment 5

8 years ago
Created attachment 452583 [details] [diff] [review]
Part 5: Remove a bogus NS_ENSURE_TRUE check, and add a couple more
Attachment #452583 - Flags: review?(roc)
(Assignee)

Comment 6

8 years ago
Created attachment 452584 [details] [diff] [review]
Part 6: Don't try to split a node which can't contain a list when inserting one

This is the core problem causing the last assertion.  The code to inject a list into the document did not change that the parent (in this case, the input element) can contain a list element (in this case, a ul element), and attempted to split this into two nodes.

In addition to the assertion, this caused the input element to be split into two input elements!  Therefore, I'll add a reftest which makes sure that the assertion doesn't happen, and that the input element is not split into two!
Attachment #452584 - Flags: review?(roc)
(Assignee)

Comment 7

8 years ago
Created attachment 452589 [details] [diff] [review]
Part 7: reftest
Attachment #452589 - Flags: review?(roc)
(Assignee)

Comment 8

8 years ago
Created attachment 452625 [details] [diff] [review]
Part 7: reftest

Actually, we need to remove the selection from the document, otherwise the rendering of 572598-1.html can't match that of 572598-ref.html.
Attachment #452589 - Attachment is obsolete: true
You need to log in before you can comment on or make changes to this bug.