Bug 56407 – REGRESSION (r81165): Assert running editing/style/iframe-onload-crash.html with non-Mac editing behavior

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[David Levin]

ASSERTION FAILED: destination.deepEquivalent().anchorNode()->inDocument()
third_party/WebKit/Source/WebCore/editing/CompositeEditCommand.cpp(987) : void WebCore::CompositeEditCommand::moveParagraphs(const WebCore::VisiblePosition&, const WebCore::VisiblePosition&, const WebCore::VisiblePosition&, bool, bool)
[28191:28191:1053796965558:ERROR:process_util_posix.cc(108)] Received signal 11

It may happen in other places but this is the first bot I see doing this.

[David Levin]

Started happening after http://trac.webkit.org/changeset/81165 landed.

[Adam Roben (:aroben)]

This is asserting on Windows, too: http://build.webkit.org/results/Windows%20XP%20Debug%20(Tests)/r81179%20(26384)/editing/style/iframe-onload-crash-crash-log.txt

[Levi Weintraub]

Mac editing behavior  masks this bug, but in the end it comes down to issues with canHaveChildrenForEditing, which failed to take into account whether the node actually had children. Any node with children should return true for canHaveChildrenForEditing.

I'm testing this change before uploading, but I've validated it fixes this crash.

[Levi Weintraub]

Created attachment 85895 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 85895 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=85895&action=review

> Source/WebCore/editing/htmlediting.cpp:92
> +        || node->hasChildNodes());

Now that I think about it, it's odd that a function named "canHaveChildrenForEditing" changes its return value depending on whether or not the node has children. Can we add this fix in editingIgnoresContent?  I started to think that these two functions are really wrong.

[Ryosuke Niwa]

Our note:

    return !node->hasTagName(hrTag) && // only if no children
           !node->hasTagName(brTag) && // always
           !node->hasTagName(imgTag) && // always
           !node->hasTagName(buttonTag) && // editable?
           !node->hasTagName(inputTag) && // shadow
           !node->hasTagName(textareaTag) && // shadow
           !node->hasTagName(objectTag) && // useFallbackContents returns false
           !node->hasTagName(iframeTag) && // always
           !node->hasTagName(embedTag) && // when render isn't RenderEmbeddedObject?
           !node->hasTagName(appletTag) && // when renderer isn't RenderApplet??
           !node->hasTagName(selectTag) && // shadow
           !node->hasTagName(datagridTag) &&
#if ENABLE(WML)
           !node->hasTagName(WMLNames::doTag) && // ??
#endif
           !node->isTextNode();

[David Levin]

Can we get something in to fix this or rollout the original while it is being addressed?

[Ryosuke Niwa]

(In reply to comment #7)
> Can we get something in to fix this or rollout the original while it is being addressed?

We're working on it.  We should be able to fix it by tomorrow.  r81165 was indeed a correct change it's just that it revealed another editing bug.  I'd rather not revert the patch given that the crash doesn't seem to reproduce on release build.

[Ryosuke Niwa]

The test is also failing on GTK debug build as expected:
http://build.webkit.org/results/GTK%20Linux%2064-bit%20Debug/r81213%20(20426)/editing/style/iframe-onload-crash-stderr.txt

[Adam Roben (:aroben)]

I added this test to the Windows Skipped file. Please remove it when fixing this bug.

[Philippe Normand]

Also skipped in GTK: http://trac.webkit.org/changeset/81246
Please unskip when landing the fix!

[David Levin]

Comment on attachment 85895 [details]
Patch

r- I think there are comments waiting to be addressed. Plus there are at least three skipped files that should be modified which aren't in this patch.

Please consider the cost imposed on lots of people when leaving stuff like this in the tree. (Several people had to investigate this failure, search bugs, modify skipped list, add comments here, etc.) In order words, it may seem good to leave an improvement in the tree but it imposes a cost on lots of other people.

In addition leaving asserts in the tree makes it harder for people to use debug builds to test things because they may hit this assert and think something it wrong etc.

If an assert will only take a short time to fix, then it will only take a short while to get in the original patch. If it will take along while to fix, then the patch shouldn't be left in anyway.

In short, please consider rolling out in the future.

[Levi Weintraub]

Created attachment 85953 [details]
Patch

[Levi Weintraub]

Committed r81266: <http://trac.webkit.org/changeset/81266>

[David Levin]

Yeah!