Bug 149289 – Null dereference loading Blink layout test editing/execCommand/delete-hidden-crash.html

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

[Jon Honeycutt]

Created attachment 261423 [details]
crashing test

Null dereference loading Blink layout test editing/execCommand/delete-hidden-crash.html.

Stack trace:

Crashed Thread:        0  Dispatch queue: com.apple.main-thread

Exception Type:        EXC_BAD_ACCESS (SIGSEGV)
Exception Codes:       KERN_INVALID_ADDRESS at 0x0000000000000014
Exception Note:        EXC_CORPSE_NOTIFY

VM Regions Near 0x14:
--> 
    __TEXT                 000000010102c000-000000010102e000 [    8K] r-x/rwx SM=COW  /Users/USER/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development

Application Specific Information:
CRASHING TEST: temp-tests/editing/execCommand/delete-hidden-crash.html

Global Trace Buffer (reverse chronological seconds):
41.945279    CFNetwork                 	0x00007fff88d43b97 Explicitly setting CF cookie storage singleton
41.945648    CFNetwork                 	0x00007fff88d8f211 Explicitly setting cookie storage singleton

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x0000000109855560 WebCore::Node::isDescendantOf(WebCore::Node const*) const + 32 (Node.h:638)
1   com.apple.WebCore             	0x000000010901d5bc WebCore::DeleteSelectionCommand::removePreviouslySelectedEmptyTableRows() + 556 (DeleteSelectionCommand.cpp:708)
2   com.apple.WebCore             	0x000000010901e9ce WebCore::DeleteSelectionCommand::doApply() + 1598 (DeleteSelectionCommand.cpp:856)
3   com.apple.WebCore             	0x0000000108ed852b WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand>) + 43 (CompositeEditCommand.cpp:281)
4   com.apple.WebCore             	0x0000000108edadc7 WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool) + 135 (StdLibExtras.h:366)
5   com.apple.WebCore             	0x000000010935e8fb WebCore::InsertTextCommand::doApply() + 107 (VisibleSelection.h:75)
6   com.apple.WebCore             	0x0000000108ed8630 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::CompositeEditCommand>, WebCore::VisibleSelection const&) + 80 (CompositeEditCommand.cpp:296)
7   com.apple.WebCore             	0x0000000109d23c13 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 115 (StdLibExtras.h:366)
8   com.apple.WebCore             	0x0000000109d24091 void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) + 257 (StdLibExtras.h:366)
9   com.apple.WebCore             	0x0000000109d23656 WebCore::TypingCommand::doApply() + 214 (TypingCommand.cpp:286)
10  com.apple.WebCore             	0x0000000108ed8216 WebCore::CompositeEditCommand::apply() + 102 (ScopedEventQueue.h:71)
11  com.apple.WebCore             	0x0000000109ce5263 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WTF::PassRefPtr<WebCore::TextInsertionBaseCommand>, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) + 67 (StdLibExtras.h:366)
12  com.apple.WebCore             	0x0000000109d22bde WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 686 (StdLibExtras.h:366)
13  com.apple.WebCore             	0x000000010910c29a WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 26 (EditorCommand.cpp:535)
14  com.apple.WebCore             	0x000000010910a876 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 182 (EditorCommand.cpp:1704)
15  com.apple.WebCore             	0x0000000109043c36 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 214 (Document.cpp:4666)
16  com.apple.WebCore             	0x000000010945a074 WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) + 420 (JSCJSValue.h:499)
17  ???                           	0x00002bb58a001028 0 + 48058704334888
18  com.apple.JavaScriptCore      	0x00000001087b676f llint_entry + 22696
19  com.apple.JavaScriptCore      	0x00000001087b0ce4 vmEntryToJavaScript + 299
20  com.apple.JavaScriptCore      	0x00000001086712d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (JITCode.cpp:82)
21  com.apple.JavaScriptCore      	0x0000000108657d12 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 450 (Interpreter.cpp:1008)
22  com.apple.JavaScriptCore      	0x00000001083238b7 JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 71 (MarkedBlock.h:241)
23  com.apple.WebCore             	0x00000001094defa4 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) + 996 (JSMainThreadExecState.h:56)
24  com.apple.WebCore             	0x00000001091454db WebCore::EventTarget::fireEventListeners(WebCore::Event*, WebCore::EventTargetData*, WTF::Vector<WebCore::RegisteredEventListener, 1ul, WTF::CrashOnOverflow, 16ul>&) + 635 (InspectorInstrumentation.h:285)
25  com.apple.WebCore             	0x00000001091451a0 WebCore::EventTarget::fireEventListeners(WebCore::Event*) + 224 (EventTarget.cpp:208)
26  com.apple.WebCore             	0x00000001090d1bf4 WebCore::DOMWindow::dispatchEvent(WTF::PassRefPtr<WebCore::Event>, WTF::PassRefPtr<WebCore::EventTarget>) + 260 (DOMWindow.cpp:1900)
27  com.apple.WebCore             	0x00000001090d730b WebCore::DOMWindow::dispatchLoadEvent() + 347 (StdLibExtras.h:366)
28  com.apple.WebCore             	0x000000010903a294 WebCore::Document::implicitClose() + 324 (Document.cpp:4077)
29  com.apple.WebCore             	0x00000001091c7003 WebCore::FrameLoader::checkCompleted() + 275 (FrameLoader.cpp:839)
30  com.apple.WebCore             	0x00000001091c595b WebCore::FrameLoader::finishedParsing() + 123 (FrameLoader.cpp:760)
31  com.apple.WebCore             	0x0000000109045281 WebCore::Document::finishedParsing() + 417 (Frame.h:377)
32  com.apple.WebCore             	0x0000000109271e05 WebCore::HTMLDocumentParser::prepareToStopParsing() + 165 (RefCounted.h:99)
33  com.apple.WebCore             	0x000000010907569a WebCore::DocumentWriter::end() + 58 (StdLibExtras.h:366)
34  com.apple.WebCore             	0x000000010905d9ec WebCore::DocumentLoader::finishedLoading(double) + 268 (ResourceErrorBase.h:42)
35  com.apple.WebCore             	0x0000000108e8e179 WebCore::CachedResource::checkNotify() + 153 (CachedResourceClientWalker.h:51)
36  com.apple.WebCore             	0x0000000108e8a433 WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 227 (CachedRawResource.cpp:104)
37  com.apple.WebCore             	0x0000000109c05501 WebCore::SubresourceLoader::didFinishLoading(double) + 1153 (ResourceLoader.h:154)
38  com.apple.WebKit              	0x000000010774b98d WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 561 (HandleMessage.h:16)
39  com.apple.WebKit              	0x00000001075251f1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 127 (memory:2636)
40  com.apple.WebKit              	0x0000000107527b4a IPC::Connection::dispatchOneMessage() + 126 (memory:2656)
41  com.apple.JavaScriptCore      	0x0000000108969985 WTF::RunLoop::performWork() + 437 (functional:1742)
42  com.apple.JavaScriptCore      	0x0000000108969d32 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
43  com.apple.CoreFoundation      	0x00007fff949e2c01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
44  com.apple.CoreFoundation      	0x00007fff949d4b1c __CFRunLoopDoSources0 + 556
45  com.apple.CoreFoundation      	0x00007fff949d403f __CFRunLoopRun + 927
46  com.apple.CoreFoundation      	0x00007fff949d3a38 CFRunLoopRunSpecific + 296
47  com.apple.HIToolbox           	0x00007fff88e673bd RunCurrentEventLoopInMode + 235
48  com.apple.HIToolbox           	0x00007fff88e67153 ReceiveNextEventCommon + 432
49  com.apple.HIToolbox           	0x00007fff88e66f93 _BlockUntilNextEventMatchingListInModeWithFilter + 71
50  com.apple.AppKit              	0x00007fff870b81e7 _DPSNextEvent + 1076
51  com.apple.AppKit              	0x00007fff8748490d -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
52  com.apple.AppKit              	0x00007fff870ae0b8 -[NSApplication run] + 682
53  com.apple.AppKit              	0x00007fff87030396 NSApplicationMain + 1176
54  libxpc.dylib                  	0x00007fff8c70ff70 _xpc_objc_main + 793
55  libxpc.dylib                  	0x00007fff8c7116bf xpc_main + 494
56  com.apple.WebKit.WebContent.Development	0x000000010102d424 main + 409 (XPCServiceMain.Development.mm:187)
57  libdyld.dylib                 	0x00007fff93aa15ad start + 1

[Radar WebKit Bug Importer]

<rdar://problem/22746352>

[Jiewen Tan]

Created attachment 263755 [details]
Patch

[Enrica Casucci]

Comment on attachment 263755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263755&action=review

> Source/WebCore/editing/htmlediting.cpp:288
> +Position firstEditablePositionAfterPositionInRoot(const Position& position, Node* highestRoot)

It is not clear to me why you had to make the change to the returned value. It doesn't seem to add anything to the fix itself. Also, not creating a VisiblePosition from a position prevents the position from being canonicalized and could potentially change the behavior of the system. What I'm saying is that given a Position p, VisiblePosition( p ).deepEquivalent() is not guaranteed to be equal to p.
Are all the other editing tests still passing?

> LayoutTests/editing/execCommand/delete-hidden-crash.html:29
> +</html>

We tend to write modern editing tests using the framework in editing.js and markup.js. Look for an example under the LayoutTests/editing folder.

[Jiewen Tan]

Comment on attachment 263755 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=263755&action=review

>> Source/WebCore/editing/htmlediting.cpp:288
>> +Position firstEditablePositionAfterPositionInRoot(const Position& position, Node* highestRoot)
> 
> It is not clear to me why you had to make the change to the returned value. It doesn't seem to add anything to the fix itself. Also, not creating a VisiblePosition from a position prevents the position from being canonicalized and could potentially change the behavior of the system. What I'm saying is that given a Position p, VisiblePosition( p ).deepEquivalent() is not guaranteed to be equal to p.
> Are all the other editing tests still passing?

Yes, it passed all the other editing tests. Actually, if you search over the whole workspace, except the one I modified (Editor::advanceToNextMisspelling), all the other code is using a VisiblePosition type to catch the return value. Therefore, the return value will be automatically converted to its corresponding VisiblePosition in all other places. However, since VisiblePosition could be null. the original methods will cause problem for those callers who need Position type. The deepEquivalent() will not work for a null object. Alternatively, we could add methods that deal with Position type. However, as the functions of the new methods are exactly the same as the one who deal with VisiblePosition, it will be redundant. Considering the fact that a Position type can be converted to VisiblePosition automatically, it will be good to just deal with Position type instead of both Position and VisiblePosition.

[Jiewen Tan]

Created attachment 264111 [details]
Patch

[Enrica Casucci]

> Yes, it passed all the other editing tests. Actually, if you search over the
> whole workspace, except the one I modified
> (Editor::advanceToNextMisspelling), all the other code is using a
> VisiblePosition type to catch the return value. 
For this case you have definitely modified the behavior though.

Therefore, the return value
> will be automatically converted to its corresponding VisiblePosition in all
> other places. However, since VisiblePosition could be null. the original
> methods will cause problem for those callers who need Position type. The
> deepEquivalent() will not work for a null object. Alternatively, we could
> add methods that deal with Position type. However, as the functions of the
> new methods are exactly the same as the one who deal with VisiblePosition,
> it will be redundant. Considering the fact that a Position type can be
> converted to VisiblePosition automatically, it will be good to just deal
> with Position type instead of both Position and VisiblePosition.

I don't understand why you are saying this. A position can be null as well. The problem
you are addressing is fixed with making sure you are not starting or ending on a non editable position. Maybe I'm missing the point here, but I still don't see how this is relevant and it still worries me that you are changing the behavior by avoiding canonicalizing in one place.

[Jiewen Tan]

Yes, I am trying to make sure that the starting position and ending position are within editable areas. Hence, I am trying to reuse these two functions: VisiblePosition firstEditablePositionAfterPositionInRoot() and VisiblePosition lastEditablePositionBeforePositionInRoot(), to move the starting and ending positions to proper positions. However, the original flow of these two functions will convert the result which is a Position type to a VisiblePosition type before return. Therefore, any non visible position will be ignored and then a null object will be returned. Thus, I cannot use these two functions directly. Either I create two new methods which do the exact same thing but without cast the result to VisiblePosition, or I modify the original methods to return Position directly. After careful examination (maybe not careful enough), I found it is better to go with the latter one. Therefore that's how this patch came out. If this approach is not good, should I go with my first idea? Or do you have any other suggestions I could follow?

(In reply to comment #6)
> > Yes, it passed all the other editing tests. Actually, if you search over the
> > whole workspace, except the one I modified
> > (Editor::advanceToNextMisspelling), all the other code is using a
> > VisiblePosition type to catch the return value. 
> For this case you have definitely modified the behavior though.
> 
> Therefore, the return value
> > will be automatically converted to its corresponding VisiblePosition in all
> > other places. However, since VisiblePosition could be null. the original
> > methods will cause problem for those callers who need Position type. The
> > deepEquivalent() will not work for a null object. Alternatively, we could
> > add methods that deal with Position type. However, as the functions of the
> > new methods are exactly the same as the one who deal with VisiblePosition,
> > it will be redundant. Considering the fact that a Position type can be
> > converted to VisiblePosition automatically, it will be good to just deal
> > with Position type instead of both Position and VisiblePosition.
> 
> I don't understand why you are saying this. A position can be null as well.
> The problem
> you are addressing is fixed with making sure you are not starting or ending
> on a non editable position. Maybe I'm missing the point here, but I still
> don't see how this is relevant and it still worries me that you are changing
> the behavior by avoiding canonicalizing in one place.

[Jiewen Tan]

Created attachment 264809 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264809 [details]
Patch

Clearing flags on attachment: 264809

Committed r192043: <http://trac.webkit.org/changeset/192043>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.