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>
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>
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
Did not reproduce on TOT. Can someone else try it?
(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.
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.
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.
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
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.
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.
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.
*** Bug 28807 has been marked as a duplicate of this bug. ***
Chromium bug tracker URL: http://crbug.com/20891
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.
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.
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?
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.
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.
+ 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.
Created attachment 39286 [details] fixes the bug but requires a lot of rebaselines
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.
(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.
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.
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.
It seems like Niwa's change does too much, and Bono's change does too little. :(
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.
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.
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?
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. :)
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.
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.
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
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.
False rejection was caused by bug 30098.
Comment on attachment 40616 [details] The second quick fix Clearing flags on attachment: 40616 Committed r49129: <http://trac.webkit.org/changeset/49129>
All reviewed patches have been landed. Closing bug.