Bug 35791

Summary: 2x execCommand ReadAV@NULL
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: eric, hamaji, morrita, tkent, tony
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
URL: http://code.google.com/p/chromium/issues/detail?id=34575
Attachments:
Description Flags
patch v0
none
v1; use inDocument() none

Berend-Jan Wever
Reported 2010-03-05 08:12:16 PST
Repro: <BODY></BODY> <SCRIPT> document.designMode = "on"; document.execCommand("selectall"); document.execCommand("InsertLineBreak"); document.execCommand("InsertLineBreak"); document.execCommand("forecolor", false, 1); // Optional, see note document.execCommand("InsertText", false, 1); document.execCommand("SelectAll"); document.execCommand("createlink", false, 1); document.execCommand("justifycenter", false); </SCRIPT> The "forecolor" line is optional: there are two different crashes that can happen, one with and one without it.
Attachments
patch v0 (4.43 KB, patch)
2010-03-17 02:51 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
v1; use inDocument() (4.42 KB, patch)
2010-04-02 22:29 PDT, Hajime Morrita 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Tony Chang
Comment 1 2010-03-08 17:15:46 PST
I will see if I can get a stack trace.
Tony Chang
Comment 2 2010-03-08 17:33:03 PST
#0 0x040ee740 in WebCore::comparePositions (a=@0xbfffd9a8, b=@0xbfffd798) at /Users/tc/webkit/WebKit/WebCore/editing/htmlediting.cpp:101 #1 0x03e214b8 in WebCore::CompositeEditCommand::moveParagraphContentsToNewBlockIfNecessary (this=0x17988f00, pos=@0xbfffd9a8) at /Users/tc/webkit/WebKit/WebCore/editing/CompositeEditCommand.cpp:666 #2 0x03d9e94f in WebCore::ApplyStyleCommand::applyBlockStyle (this=0x17988f00, style=0x17988df0) at /Users/tc/webkit/WebKit/WebCore/editing/ApplyStyleCommand.cpp:613 #3 0x03d9ef39 in WebCore::ApplyStyleCommand::doApply (this=0x17988f00) at /Users/tc/webkit/WebKit/WebCore/editing/ApplyStyleCommand.cpp:568 #4 0x03ff6f11 in WebCore::EditCommand::apply (this=0x17988f00) at /Users/tc/webkit/WebKit/WebCore/editing/EditCommand.cpp:91 #5 0x03ff6fa5 in WebCore::applyCommand (command=@0xbfffdae8) at /Users/tc/webkit/WebKit/WebCore/editing/EditCommand.cpp:217 #6 0x03ffd3f0 in WebCore::Editor::applyParagraphStyle (this=0x81e8cd0, style=0x17988df0, editingAction=WebCore::EditActionUnspecified) at /Users/tc/webkit/WebKit/WebCore/editing/Editor.cpp:747 #7 0x04006ff6 in WebCore::executeApplyParagraphStyle (frame=0x81e8800, source=WebCore::CommandFromDOM, action=WebCore::EditActionCenter, propertyID=1105, propertyValue=@0xbfffdb7c) at /Users/tc/webkit/WebKit/WebCore/editing/EditorCommand.cpp:188 #8 0x040071ce in WebCore::executeJustifyCenter (frame=0x81e8800, source=WebCore::CommandFromDOM) at /Users/tc/webkit/WebKit/WebCore/editing/EditorCommand.cpp:552 #9 0x040034f7 in WebCore::Editor::Command::execute (this=0xbfffdbe4, parameter=@0xbfffdc54, triggeringEvent=0x0) at /Users/tc/webkit/WebKit/WebCore/editing/EditorCommand.cpp:1525 #10 0x03f3a2b5 in WebCore::Document::execCommand (this=0x822ea00, commandName=@0xbfffdc58, userInterface=false, value=@0xbfffdc54) at /Users/tc/webkit/WebKit/WebCore/dom/Document.cpp:3491 #11 0x0422f33c in WebCore::jsDocumentPrototypeFunctionExecCommand (exec=0x14e9e390, thisValue={u = {asEncodedJSValue = -8455908544, asDouble = -nan(0xffffe07fd1340), asBits = {payload = 134026048, tag = -2}}}, args=@0xbfffdc8c) at /Users/tc/webkit/WebKit/WebKitBuild/Debug/DerivedSources/WebCore/JSDocument.cpp:1879
Hajime Morrita
Comment 3 2010-03-17 02:51:48 PDT
Created attachment 50884 [details] patch v0
Tony Chang
Comment 4 2010-03-17 17:44:38 PDT
This looks good to me, but I am not a reviewer.
Eric Seidel (no email)
Comment 5 2010-04-01 23:33:20 PDT
Comment on attachment 50884 [details] patch v0 Seems just using .node()->inDocument() would be better.
Hajime Morrita
Comment 6 2010-04-02 22:29:23 PDT
Created attachment 52477 [details] v1; use inDocument()
Hajime Morrita
Comment 7 2010-04-02 22:30:45 PDT
Eric, thank you for reviewing! (In reply to comment #5) > Seems just using .node()->inDocument() would be better. Certainly. I updated the patch to fix it.
Darin Adler
Comment 8 2010-04-04 22:26:31 PDT
Comment on attachment 52477 [details] v1; use inDocument() Could you explain why inDocument is the right check? The node could be somewhere far away in the same document, say in an absolute positioned element, and that wouldn't be good. I am not sure that's the right check. It seems clear that a position that's not even in a document is unusable for some purposes, but it's not clear to me this is exactly the right check here. It seems far removed from the null dereference as well, so I'm not convinced it would cover all cases.
Hajime Morrita
Comment 9 2010-04-04 23:21:15 PDT
Darin, thank you for your feedback. > Could you explain why inDocument is the right check? The node could be > somewhere far away in the same document, say in an absolute positioned element, > and that wouldn't be good. In applyBlockStyle(), we assume that nextParagraphStart and paragraphStart start with a valid, non-orphan node (or null node). Such assumption should be OK because these are created from current selection and every VisibleSelections are validated. Based on such assumption, what we do here is just discarding invalid VisiblePosition and re-creating valid one. The VisiblePosition can become invalid because moveParagraphContentsToNewBlockIfNecessary() sometimes modify the tree to make it ready to apply style, And nodes are possibly removed during the process. Another approach may be making moveParagraphContentsToNewBlockIfNecessary() not to remove any nodes. But I'm not sure it is feasible.
Joseph Pecoraro
Comment 10 2010-04-06 22:47:24 PDT
*** Bug 34568 has been marked as a duplicate of this bug. ***
Kent Tamura
Comment 11 2010-05-20 02:40:53 PDT
Comment on attachment 52477 [details] v1; use inDocument() Looks good.
Hajime Morrita
Comment 12 2010-05-20 04:14:43 PDT
Commited at http://trac.webkit.org/changeset/59828 - then webkit-patch crashed. Could anyone close this bug?
Hajime Morrita
Comment 13 2010-05-20 05:02:37 PDT
Oops. I could close this.
Note You need to log in before you can comment on or make changes to this bug.