Bug 56407 - REGRESSION (r81165): Assert running editing/style/iframe-onload-crash.html with non-Mac editing behavior
Summary: REGRESSION (r81165): Assert running editing/style/iframe-onload-crash.html wi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Levi Weintraub
URL:
Keywords: LayoutTestFailure, Regression
Depends on:
Blocks:
 
Reported: 2011-03-15 13:29 PDT by David Levin
Modified: 2011-03-16 12:23 PDT (History)
5 users (show)

See Also:


Attachments
Patch (3.64 KB, patch)
2011-03-15 18:55 PDT, Levi Weintraub
no flags Details | Formatted Diff | Diff
Patch (13.35 KB, patch)
2011-03-16 12:16 PDT, Levi Weintraub
rniwa: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Levin 2011-03-15 13:29:21 PDT
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.
Comment 1 David Levin 2011-03-15 13:32:45 PDT
Started happening after http://trac.webkit.org/changeset/81165 landed.
Comment 3 Levi Weintraub 2011-03-15 17:46:09 PDT
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.
Comment 4 Levi Weintraub 2011-03-15 18:55:06 PDT
Created attachment 85895 [details]
Patch
Comment 5 Ryosuke Niwa 2011-03-15 19:02:09 PDT
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.
Comment 6 Ryosuke Niwa 2011-03-15 19:42:48 PDT
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();
Comment 7 David Levin 2011-03-15 19:45:54 PDT
Can we get something in to fix this or rollout the original while it is being addressed?
Comment 8 Ryosuke Niwa 2011-03-15 20:29:17 PDT
(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.
Comment 9 Ryosuke Niwa 2011-03-15 21:38:13 PDT
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
Comment 10 Adam Roben (:aroben) 2011-03-15 23:58:33 PDT
I added this test to the Windows Skipped file. Please remove it when fixing this bug.
Comment 11 Philippe Normand 2011-03-16 08:41:37 PDT
Also skipped in GTK: http://trac.webkit.org/changeset/81246
Please unskip when landing the fix!
Comment 12 David Levin 2011-03-16 08:51:36 PDT
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.
Comment 13 Levi Weintraub 2011-03-16 12:16:47 PDT
Created attachment 85953 [details]
Patch
Comment 14 Levi Weintraub 2011-03-16 12:18:39 PDT
Committed r81266: <http://trac.webkit.org/changeset/81266>
Comment 15 David Levin 2011-03-16 12:23:15 PDT
Yeah!