Bug 35791

Summary: 2x execCommand ReadAV@NULL
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
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
Description Flags
patch v0
v1; use inDocument() none

Description Berend-Jan Wever 2010-03-05 08:12:16 PST

  document.designMode = "on";
  document.execCommand("forecolor", false, 1); // Optional, see note
  document.execCommand("InsertText", false, 1);
  document.execCommand("createlink", false, 1);
  document.execCommand("justifycenter", false);

The "forecolor" line is optional: there are two different crashes that can 
happen, one with and one without it.
Comment 1 Tony Chang 2010-03-08 17:15:46 PST
I will see if I can get a stack trace.
Comment 2 Tony Chang 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
Comment 3 Hajime Morrita 2010-03-17 02:51:48 PDT
Created attachment 50884 [details]
patch v0
Comment 4 Tony Chang 2010-03-17 17:44:38 PDT
This looks good to me, but I am not a reviewer.
Comment 5 Eric Seidel (no email) 2010-04-01 23:33:20 PDT
Comment on attachment 50884 [details]
patch v0

Seems just using .node()->inDocument() would be better.
Comment 6 Hajime Morrita 2010-04-02 22:29:23 PDT
Created attachment 52477 [details]
v1; use inDocument()
Comment 7 Hajime Morrita 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.
Comment 8 Darin Adler 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.
Comment 9 Hajime Morrita 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.
Comment 10 Joseph Pecoraro 2010-04-06 22:47:24 PDT
*** Bug 34568 has been marked as a duplicate of this bug. ***
Comment 11 Kent Tamura 2010-05-20 02:40:53 PDT
Comment on attachment 52477 [details]
v1; use inDocument()

Looks good.
Comment 12 Hajime Morrita 2010-05-20 04:14:43 PDT
Commited at http://trac.webkit.org/changeset/59828 - then webkit-patch crashed. 
Could anyone close this bug?
Comment 13 Hajime Morrita 2010-05-20 05:02:37 PDT
Oops. I could close this.