Bug 28710 – Copy some forms of text causes Webkit crash in CSSStyleDeclaration::copyPropertiesInSet

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

Went here
http://classicalmusic.about.com/od/romanticperiodsymphonies/qt/Beethovenjoytxt.htm

selected the translation text "O friends....  above the stars must he dwell."

I did a command-C to copy the text.  As the mouse pointer was going to the text editor window, Webkit crashed.

repeated several times until deciding to report this bug. 

Note: Could copy text off of the Webkit startup "nightly build" web page without crashing

Please attach a crash log <http://webkit.org/quality/crashlogs.html>

[Alexey Proskuryakov]

I can reproduce this with a local debug build of r47661, stack trace:

#0	0x03a6102f in WebCore::CSSStyleDeclaration::copyPropertiesInSet at CSSStyleDeclaration.cpp:153
#1	0x0396b118 in WebCore::editingStyleAtPosition at ApplyStyleCommand.cpp:386
#2	0x03fb0ed1 in WebCore::createMarkup at markup.cpp:1007
#3	0x03f8f44b in WebCore::LegacyWebArchive::createFromSelection at LegacyWebArchive.cpp:567
#4	0x04007cae in WebCore::Pasteboard::writeSelection at PasteboardMac.mm:166
#5	0x04008180 in WebCore::Pasteboard::writeSelection at PasteboardMac.mm:203
#6	0x03bd911b in WebCore::Editor::copy at Editor.cpp:1035
#7	0x03bde8c7 in WebCore::executeCopy at EditorCommand.cpp:284
#8	0x03bdc3a3 in WebCore::Editor::Command::execute at EditorCommand.cpp:1504
#9	0x00e05ed6 in -[WebHTMLView executeCoreCommandBySelector:] at WebHTMLView.mm:2360
#10	0x00df6f8b in -[WebHTMLView copy:] at WebHTMLView.mm:2382
<..>

<rdar://problem/7170666>

[Ryosuke Niwa]

Crash report.

Version:         r47686 (47686)
Code Type:       X86 (Native)
Date/Time:       2009-08-26 09:51:48.256 -0700
OS Version:      Mac OS X 10.5.8 (9L31a)
Report Version:  6
Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000000

0   com.apple.WebCore             	0x00fa1d50 WebCore::CSSStyleDeclaration::copyPropertiesInSet(int const*, unsigned int) const + 144
1   com.apple.WebCore             	0x00f088a0 WebCore::editingStyleAtPosition(WebCore::Position, WebCore::ShouldIncludeTypingStyle) + 80
2   com.apple.WebCore             	0x014a21bf WebCore::createMarkup(WebCore::Range const*, WTF::Vector<WebCore::Node*, 0ul>*, WebCore::EAnnotateForInterchange, bool) + 4687
3   com.apple.WebCore             	0x01485e14 WebCore::LegacyWebArchive::createFromSelection(WebCore::Frame*) + 132
4   com.apple.WebCore             	0x014d9e02 WebCore::Pasteboard::writeSelection(NSPasteboard*, WebCore::Range*, bool, WebCore::Frame*) + 1410
5   com.apple.WebCore             	0x010c107d WebCore::Editor::copy() + 269
6   com.apple.WebCore             	0x010ca469 __ZN7WebCoreL11executeCopyEPNS_5FrameEPNS_5EventENS_19EditorCommandSourceERKNS_6StringE + 25
7   com.apple.WebCore             	0x010c8dfe WebCore::Editor::Command::execute(WebCore::String const&, WebCore::Event*) const + 110
8   com.apple.WebKit              	0x0032a4a7 -[WebHTMLView executeCoreCommandBySelector:] + 135

[Ryosuke Niwa]

Did not reproduce on TOT.  Can someone else try it?

[Ryosuke Niwa]

(In reply to comment #5)
> Did not reproduce on TOT.  Can someone else try it?

Finally managed to reproduce.
385    RefPtr<CSSComputedStyleDeclaration> computedStyleAtPosition = pos.computedStyle();
is returning 0.

[Ryosuke Niwa]

Created attachment 38655 [details]
reduction, copy "first paragraph" and crashes WebKit.

This crash is caused by a bug that the computed style is null if text-decoration is set to inherit at the html node.

[Ryosuke Niwa]

I did some investigation with the test case I posted.

http://trac.webkit.org/browser/trunk/WebCore/dom/Position.cpp#L199
On the line 211, anchorNode() is returning the html node, and n->isElementNode() is returning false so that n->parentNode() is called on n=html element.

editingStyleAtPosition is called from http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L1005

parentOfLastClosed is also html element.

When text-decoration: inherit is removed from the html element, the position points to div, and everything works fine.

[Ryosuke Niwa]

In createMarkup, specialCommonAncestor is null normally but it's set to the html element with text-decoration: inherit property on html element.

The problem is caused by the line 950 of markup.cpp where we find the highest enclosing node, which is presentational.

On the line 712, we check whether text decoration exist or not by checking whether it's equal to none or inherit.  But we should also ignore inherit because inherit = none for the purpose of this function.
return !propertyMissingOrEqualToNone(style.get(), CSSPropertyTextDecoration);

http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L948
http://trac.webkit.org/browser/trunk/WebCore/editing/markup.cpp#L704

[Ryosuke Niwa]

Created attachment 38845 [details]
attempt to fix the bug but needs DRT test

This patch modifies propertyMissingOrEqualToNone to propertyMissingOrEqualToNoneOrInherit.  Need a DRT test and changelog.

[Ryosuke Niwa]

Created attachment 38846 [details]
another test

Even if my attempt was accepted, WebKit still crashes when html element has text-decoration.  I feel like this problem will be solved if isElement() was true for html element.  I don't really understand why this is not the case.

[Alexey Proskuryakov]

isElementNode() should be true for the HTML element, but not for the Document node (which is its parent). Looks like the problem here is that a position's anchor node is the document node, which makes Position::element() return null.

[Alexey Proskuryakov]

*** Bug 28807 has been marked as a duplicate of this bug. ***

[Jeremy Moskovich]

Chromium bug tracker URL: http://crbug.com/20891

[Ryosuke Niwa]

Here are some options to fix this bug.

1. We make sure document is not passed as node to editingStyleAtPosition in createMakrup

2. We allow position.computedStyle to use a document node.

3. We bail out in editingStyleAtPosition if the position returned a null pointer.

There are several ways to implement 1, and I can't really think of a good one.  But I don't think why any element with text-decoration must be a special ancestor.  I feel like we can get away without making it special ancestor since we're adding computedStyle in a wrapping span later anyways.

[Ryosuke Niwa]

I tried #2 but this method turned out to be insufficient.  When allowing html elemenet to be a special ancestor causes the style attribute of the html element in inside which the fragment is pasted to be modififed.

To solve this problem, we should disallow elements with text-decoration property to be a special ancestor, and should use StyleChange class from ApplyStyleCommand.

[Ryosuke Niwa]

Created attachment 39020 [details]
2nd attempt to fix the bug, requires rebaseline & DRT test

As I described above, we need to export StyleChange from ApplyStyleCommand and use it in createMarkup.  But that means we might end up wrapping a bunch of block elements with b, s, etc...  But this requires substential architectual change of the editing code (takes at least several months).  So for time being, we can ignore StyleWithCSS=false, and just fix the crash.

This patch tries to modify Position::computedStyle to allow document node to be passed to computedStyle.  In addition, it modifies isElementPresentational not to recognize html elements with text-decoration property.  Instead of treating them as special ancestors, we'll preserve text decorations from the computed style when we wrap the content with style span if necessary.

This patch somehow caused a whole bunch of tests to fail because WebKit spits out a bunch of annoymous nodes and empty text nodes.  Does anyone know why this happens?

[Ryosuke Niwa]

Comment on attachment 39020 [details]
2nd attempt to fix the bug, requires rebaseline & DRT test

Make my patch r? even though I don't intend to land this patch at all.  I just need some feedback on my approach here.

[Eric Seidel (no email)]

Comment on attachment 39020 [details]
2nd attempt to fix the bug, requires rebaseline & DRT test

This will need a layout test in the end.  Not sure what about this approach you want feedback on.  Probably best to just post a "final" patch that can be actually reviewed.

[Justin Garcia]

+    ContainerNode* elem = element();
+    // Allow document node to be passed if there

Comment is unnecessary since it just says what the code does.

> This patch somehow caused a whole bunch of tests to fail 
> because WebKit spits out a bunch of annoymous nodes and 
> empty text nodes.  Does anyone know why this happens?

Could you attach those diffs?

     // All text-decoration-related elements should have been treated as special ancestors
     // If we ever hit this ASSERT, we should export StyleChange in ApplyStyleCommand and use it here
-    ASSERT(propertyMissingOrEqualToNone(style, CSSPropertyTextDecoration) && propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));
+    ASSERT(propertyMissingOrEqualToNone(style, CSSPropertyWebkitTextDecorationsInEffect));

I think that the comment needs to be updated.

[Ryosuke Niwa]

Created attachment 39286 [details]
fixes the bug but requires a lot of rebaselines

[Hironori Bono]

Created attachment 39409 [details]
A super-quick fix written in an hour

This is a super-quick workaround for this bug written this morning. (I haven't noticed you have been working for this bug.)
Even though it is nothing but a better-than-crash fix, I wish this helps you.

[Ryosuke Niwa]

(In reply to comment #22)
> Created an attachment (id=39409) [details]
> A super-quick fix written in an hour
> 
> This is a super-quick workaround for this bug written this morning. (I haven't
> noticed you have been working for this bug.)
> Even though it is nothing but a better-than-crash fix, I wish this helps you.

Your patch fixes the crash but doesn't fix the problem caused this crash: passing a null pointer to editingStyleAtPosition.  We should never be passing a null pointer to editingStyleAtPosition because it doesn't make sense to obtain the editing style at null position.

[Justin Garcia]

Ryosuke, did you scope out the DOM changes that are going on with some of these layout tests?  I'd be interested to see what's going on...I think in most cases it's just some unrendered whitespace from the original HTML that's no longer cleared away by editing.

[Eric Seidel (no email)]

Sigh.  So this is the top WebKit crasher for Chromium.  We need to get a fix in. I'm not sure reading Ryosuke's patch if the rebaselining is correct.  I'll see if I can't catch him over IRC or GChat and ask him.

[Eric Seidel (no email)]

It seems like Niwa's change does too much, and Bono's change does too little. :(

[Eric Seidel (no email)]

Comment on attachment 39286 [details]
fixes the bug but requires a lot of rebaselines

The anonymous nodes are wrong. :(  We chatted some about this over IRC.  I think we need to try for a more surgical fix.

[Ryosuke Niwa]

Talked with Eric S. on IRC, and concluded that we should use Bono's patch for now since this is a major crash bug even though his patch does not fix the actual behavior of WebKit that it attemps to copy html element only if it has text-decoration style. (this results in the modification of the html element of the document into which the content.)  We should file a separate bug to fix these issues, and assert that position.node() is not NULL in editingStyleAtPosition in later patch.

[Ryosuke Niwa]

Investigated LayoutTests/editing/pasteboard/display-block-on-spans.html.  It turned out that there were no change in the DOM generated.  Anonymous node seems to be created in renderer.  Can somene explain to me when/how anonymous nodes are made?  I tried to follow find it on XCode debugger, but I couldn't locate where the nodes are generated. (No difference in showTreeForThis)

Meanwhile, it seems like we could convert the tests that require rebaseline in my patch to dumpAsText tests first because we can separte it (that WebKit generates anonymous node in this case) into another bug if the resultant DOMs are same.

If people don't mind, I'd like to use my patch as is, or after converting tests to dumpAsText tests that spits out HTML instead of render tree.  

Any opinions?

[Hironori Bono]

Created attachment 40616 [details]
The second quick fix

I would like to upload another better-than-crash patch as requested by Eric.
I agree with the comment of Niwa-san that this patch isn't a good fix. I wish someone writes a better fix. :)

[Eric Seidel (no email)]

Comment on attachment 40616 [details]
The second quick fix

Looks fine to me.  Ryosuke wanted to do a follow-up fix, but given how common this crasher is (most common WebKit-related crasher in Chrome), I think we should get this minimal fix in first.

[Eric Seidel (no email)]

Comment on attachment 40616 [details]
The second quick fix

Bono-san is not a committer according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/modules/committers.py, so I'm adding cq+ to get this landed.

[WebKit Commit Bot]

Comment on attachment 40616 [details]
The second quick fix

Rejecting patch 40616 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11380 test cases.
fast/dom/prototype-inheritance.html -> failed

Exiting early after 1 failures. 5489 tests run.
134.88s total testing time

5488 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
1 test case (<1%) had stderr output

[Eric Seidel (no email)]

Comment on attachment 40616 [details]
The second quick fix

The buildbots were behind.  bugzilla-tool checked them, saw they were green, but they shouldn't have been.  This was not your fault.

[Eric Seidel (no email)]

False rejection was caused by bug 30098.

[WebKit Commit Bot]

Comment on attachment 40616 [details]
The second quick fix

Clearing flags on attachment: 40616

Committed r49129: <http://trac.webkit.org/changeset/49129>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.