Bug 149298 – Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html

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

[Jon Honeycutt]

Created attachment 261433 [details]
crashing test

Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.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                 0000000103df0000-0000000103df2000 [    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/inserting/insert-html-crash-01.html

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

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x000000010bc02dcd WebCore::CompositeEditCommand::insertNodeAt(WTF::PassRefPtr<WebCore::Node>, WebCore::Position const&) + 77 (htmlediting.h:103)
1   com.apple.WebCore             	0x000000010bd48a58 WebCore::DeleteSelectionCommand::doApply() + 1736 (StdLibExtras.h:366)
2   com.apple.WebCore             	0x000000010bc0252b WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand>) + 43 (CompositeEditCommand.cpp:281)
3   com.apple.WebCore             	0x000000010bc04dc7 WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool) + 135 (StdLibExtras.h:366)
4   com.apple.WebCore             	0x000000010c0888fb WebCore::InsertTextCommand::doApply() + 107 (VisibleSelection.h:75)
5   com.apple.WebCore             	0x000000010bc02630 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::CompositeEditCommand>, WebCore::VisibleSelection const&) + 80 (CompositeEditCommand.cpp:296)
6   com.apple.WebCore             	0x000000010ca4dc13 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 115 (StdLibExtras.h:366)
7   com.apple.WebCore             	0x000000010ca4e091 void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) + 257 (StdLibExtras.h:366)
8   com.apple.WebCore             	0x000000010ca4d656 WebCore::TypingCommand::doApply() + 214 (TypingCommand.cpp:286)
9   com.apple.WebCore             	0x000000010bc02216 WebCore::CompositeEditCommand::apply() + 102 (ScopedEventQueue.h:71)
10  com.apple.WebCore             	0x000000010ca0f263 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WTF::PassRefPtr<WebCore::TextInsertionBaseCommand>, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) + 67 (StdLibExtras.h:366)
11  com.apple.WebCore             	0x000000010ca4cbde WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 686 (StdLibExtras.h:366)
12  com.apple.WebCore             	0x000000010be3629a WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 26 (EditorCommand.cpp:535)
13  com.apple.WebCore             	0x000000010be34876 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 182 (EditorCommand.cpp:1704)
14  com.apple.WebCore             	0x000000010bd6dc36 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 214 (Document.cpp:4666)
15  com.apple.WebCore             	0x000000010c184074 WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) + 420 (JSCJSValue.h:499)
16  ???                           	0x00004055f4e01028 0 + 70737924722728
17  com.apple.JavaScriptCore      	0x000000010b4e076f llint_entry + 22696
18  com.apple.JavaScriptCore      	0x000000010b4dace4 vmEntryToJavaScript + 299
19  com.apple.JavaScriptCore      	0x000000010b39b2d9 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 169 (JITCode.cpp:82)
20  com.apple.JavaScriptCore      	0x000000010b381a10 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 10448 (Interpreter.cpp:945)
21  com.apple.JavaScriptCore      	0x000000010b0944c5 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 469 (Completion.cpp:104)
22  com.apple.WebCore             	0x000000010c7f78ec WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 284 (JSMainThreadExecState.h:62)
23  com.apple.WebCore             	0x000000010c7f7b29 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 41 (ScriptController.cpp:180)
24  com.apple.WebCore             	0x000000010c7fdaac WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 316 (ScriptElement.cpp:309)
25  com.apple.WebCore             	0x000000010c7fc756 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1046 (StdLibExtras.h:366)
26  com.apple.WebCore             	0x000000010bff95eb WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 347 (ScriptElement.h:58)
27  com.apple.WebCore             	0x000000010bff9440 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) + 48 (HTMLScriptRunner.cpp:191)
28  com.apple.WebCore             	0x000000010bf9c466 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 86 (StdLibExtras.h:366)
29  com.apple.WebCore             	0x000000010bf9c52d WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) + 93 (HTMLDocumentParser.cpp:214)
30  com.apple.WebCore             	0x000000010bf9c0c3 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 595 (HTMLDocumentParser.cpp:259)
31  com.apple.WebCore             	0x000000010bf9d128 WebCore::HTMLDocumentParser::resumeParsingAfterScriptExecution() + 296 (DocumentParser.h:71)
32  com.apple.WebCore             	0x000000010bf9d322 WebCore::HTMLDocumentParser::notifyFinished(WebCore::CachedResource*) + 82 (RefCounted.h:99)
33  com.apple.WebCore             	0x000000010bbb8179 WebCore::CachedResource::checkNotify() + 153 (CachedResourceClientWalker.h:51)
34  com.apple.WebCore             	0x000000010c92f807 WebCore::SubresourceLoader::didFail(WebCore::ResourceError const&) + 375 (SubresourceLoader.cpp:401)
35  com.apple.WebKit              	0x000000010a475d35 void IPC::handleMessage<Messages::WebResourceLoader::DidFailResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::ResourceError const&)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(WebCore::ResourceError const&)) + 100 (StdLibExtras.h:366)
36  com.apple.WebKit              	0x000000010a24f1f1 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 127 (memory:2636)
37  com.apple.WebKit              	0x000000010a251b4a IPC::Connection::dispatchOneMessage() + 126 (memory:2656)
38  com.apple.JavaScriptCore      	0x000000010b693985 WTF::RunLoop::performWork() + 437 (functional:1742)
39  com.apple.JavaScriptCore      	0x000000010b693d32 WTF::RunLoop::performWork(void*) + 34 (RunLoopCF.cpp:39)
40  com.apple.CoreFoundation      	0x00007fff949e2c01 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
41  com.apple.CoreFoundation      	0x00007fff949d4b1c __CFRunLoopDoSources0 + 556
42  com.apple.CoreFoundation      	0x00007fff949d403f __CFRunLoopRun + 927
43  com.apple.CoreFoundation      	0x00007fff949d3a38 CFRunLoopRunSpecific + 296
44  com.apple.HIToolbox           	0x00007fff88e673bd RunCurrentEventLoopInMode + 235
45  com.apple.HIToolbox           	0x00007fff88e67153 ReceiveNextEventCommon + 432
46  com.apple.HIToolbox           	0x00007fff88e66f93 _BlockUntilNextEventMatchingListInModeWithFilter + 71
47  com.apple.AppKit              	0x00007fff870b81e7 _DPSNextEvent + 1076
48  com.apple.AppKit              	0x00007fff8748490d -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
49  com.apple.AppKit              	0x00007fff870ae0b8 -[NSApplication run] + 682
50  com.apple.AppKit              	0x00007fff87030396 NSApplicationMain + 1176
51  libxpc.dylib                  	0x00007fff8c70ff70 _xpc_objc_main + 793
52  libxpc.dylib                  	0x00007fff8c7116bf xpc_main + 494
53  com.apple.WebKit.WebContent.Development	0x0000000103df1424 main + 409 (XPCServiceMain.Development.mm:187)
54  libdyld.dylib                 	0x00007fff93aa15ad start + 1

[Radar WebKit Bug Importer]

<rdar://problem/22746918>

[Jiewen Tan]

Created attachment 262923 [details]
Patch

[Jiewen Tan]

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

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

VM Regions Near 0xbbadbeef:
--> 
    __TEXT                 00000001089d2000-00000001089d5000 [   12K] r-x/rwx SM=COW  /Users/USER/Documents/*/WebKit.framework/Versions/A/XPCServices/com.apple.WebKit.WebContent.Development.xpc/Contents/MacOS/com.apple.WebKit.WebContent.Development

Application Specific Information:
CRASHING TEST: editing/inserting/insert-html-crash-01.html

Global Trace Buffer (reverse chronological seconds):
25.616548    CFNetwork                 	0x00007fff929903eb Explicitly setting CF cookie storage singleton
25.616830    CFNetwork                 	0x00007fff929c6c85 Explicitly setting cookie storage singleton

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x0000000110bc9637 WTFCrash + 39 (Assertions.cpp:321)
1   com.apple.WebCore             	0x0000000112918e91 WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 2113 (CompositeEditCommand.cpp:1290)
2   com.apple.WebCore             	0x000000011291a6ac WebCore::CompositeEditCommand::moveParagraph(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) + 236 (CompositeEditCommand.cpp:1211)
3   com.apple.WebCore             	0x0000000112bcf6c0 WebCore::DeleteSelectionCommand::mergeParagraphs() + 2784 (DeleteSelectionCommand.cpp:675)
4   com.apple.WebCore             	0x0000000112bd0724 WebCore::DeleteSelectionCommand::doApply() + 1220 (DeleteSelectionCommand.cpp:856)
5   com.apple.WebCore             	0x0000000112913bf7 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::EditCommand>) + 71 (CompositeEditCommand.cpp:280)
6   com.apple.WebCore             	0x0000000112916292 WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool) + 242 (CompositeEditCommand.cpp:644)
7   com.apple.WebCore             	0x0000000113388a3c WebCore::InsertTextCommand::doApply() + 268 (InsertTextCommand.cpp:147)
8   com.apple.WebCore             	0x0000000112913d41 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::PassRefPtr<WebCore::CompositeEditCommand>, WebCore::VisibleSelection const&) + 161 (CompositeEditCommand.cpp:296)
9   com.apple.WebCore             	0x000000011479e03b WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 187 (TypingCommand.cpp:379)
10  com.apple.WebCore             	0x000000011479eeb2 WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const + 146 (TypingCommand.cpp:63)
11  com.apple.WebCore             	0x000000011479e7f1 void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) + 161 (TextInsertionBaseCommand.h:61)
12  com.apple.WebCore             	0x000000011479d3f8 WebCore::TypingCommand::insertText(WTF::String const&, bool) + 72 (TypingCommand.cpp:372)
13  com.apple.WebCore             	0x000000011479db6e WebCore::TypingCommand::doApply() + 350 (TypingCommand.cpp:282)
14  com.apple.WebCore             	0x0000000112913808 WebCore::CompositeEditCommand::apply() + 216 (CompositeEditCommand.cpp:229)
15  com.apple.WebCore             	0x0000000112913721 WebCore::applyCommand(WTF::PassRefPtr<WebCore::CompositeEditCommand>) + 17 (CompositeEditCommand.cpp:189)
16  com.apple.WebCore             	0x000000011471105a WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WTF::PassRefPtr<WebCore::TextInsertionBaseCommand>, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) + 122 (TextInsertionBaseCommand.cpp:50)
17  com.apple.WebCore             	0x000000011479d365 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 757 (TypingCommand.cpp:190)
18  com.apple.WebCore             	0x000000011479d065 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 213 (TypingCommand.cpp:161)
19  com.apple.WebCore             	0x0000000112dbd904 WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 52 (EditorCommand.cpp:535)
20  com.apple.WebCore             	0x0000000112dbb667 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 215 (EditorCommand.cpp:1704)
21  com.apple.WebCore             	0x0000000112c26fa9 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 89 (Document.cpp:4657)
22  com.apple.WebCore             	0x00000001135959a4 WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*) + 740 (JSDocument.cpp:5094)
23  ???                           	0x000031892ca01028 0 + 54465228967976
24  com.apple.JavaScriptCore      	0x0000000110906da7 llint_entry + 26259
25  com.apple.JavaScriptCore      	0x00000001109004fe vmEntryToJavaScript + 334
26  com.apple.JavaScriptCore      	0x000000011075d455 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 213 (JITCode.cpp:80)
27  com.apple.JavaScriptCore      	0x0000000110734312 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*) + 4450 (Interpreter.cpp:961)
28  com.apple.JavaScriptCore      	0x000000011019cd81 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 465 (Completion.cpp:104)
29  com.apple.WebCore             	0x00000001137adea5 WebCore::JSMainThreadExecState::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&) + 69 (JSMainThreadExecState.h:62)
30  com.apple.WebCore             	0x00000001142c3c2d WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&) + 317 (ScriptController.cpp:164)
31  com.apple.WebCore             	0x00000001142c3d74 WebCore::ScriptController::evaluate(WebCore::ScriptSourceCode const&) + 68 (ScriptController.cpp:180)
32  com.apple.WebCore             	0x00000001142d2bc3 WebCore::ScriptElement::executeScript(WebCore::ScriptSourceCode const&) + 483 (ScriptElement.cpp:309)
33  com.apple.WebCore             	0x00000001142d1b43 WebCore::ScriptElement::prepareScript(WTF::TextPosition const&, WebCore::ScriptElement::LegacyTypeSupport) + 1731 (ScriptElement.cpp:242)
34  com.apple.WebCore             	0x0000000113207bf5 WebCore::HTMLScriptRunner::runScript(WebCore::Element*, WTF::TextPosition const&) + 357 (HTMLScriptRunner.cpp:310)
35  com.apple.WebCore             	0x0000000113207a09 WebCore::HTMLScriptRunner::execute(WTF::PassRefPtr<WebCore::Element>, WTF::TextPosition const&) + 137 (HTMLScriptRunner.cpp:179)
36  com.apple.WebCore             	0x0000000113135890 WebCore::HTMLDocumentParser::runScriptsForPausedTreeBuilder() + 288 (HTMLDocumentParser.cpp:195)
37  com.apple.WebCore             	0x0000000113135991 WebCore::HTMLDocumentParser::canTakeNextToken(WebCore::HTMLDocumentParser::SynchronousMode, WebCore::PumpSession&) + 177 (HTMLDocumentParser.cpp:214)
38  com.apple.WebCore             	0x0000000113134cd8 WebCore::HTMLDocumentParser::pumpTokenizer(WebCore::HTMLDocumentParser::SynchronousMode) + 488 (HTMLDocumentParser.cpp:259)
39  com.apple.WebCore             	0x00000001131348b9 WebCore::HTMLDocumentParser::pumpTokenizerIfPossible(WebCore::HTMLDocumentParser::SynchronousMode) + 169 (HTMLDocumentParser.cpp:167)
40  com.apple.WebCore             	0x0000000113136423 WebCore::HTMLDocumentParser::append(WTF::RefPtr<WTF::StringImpl>&&) + 883 (HTMLDocumentParser.cpp:393)
41  com.apple.WebCore             	0x0000000112bc389f WebCore::DecodedDataDocumentParser::flush(WebCore::DocumentWriter&) + 143 (DecodedDataDocumentParser.cpp:60)
42  com.apple.WebCore             	0x0000000112cc027e WebCore::DocumentWriter::end() + 254 (DocumentWriter.cpp:245)
43  com.apple.WebCore             	0x0000000112c824be WebCore::DocumentLoader::finishedLoading(double) + 398 (DocumentLoader.cpp:438)
44  com.apple.WebCore             	0x0000000112c8229e WebCore::DocumentLoader::notifyFinished(WebCore::CachedResource*) + 270 (DocumentLoader.cpp:385)
45  com.apple.WebCore             	0x0000000112836602 WebCore::CachedResource::checkNotify() + 130 (CachedResource.cpp:296)
46  com.apple.WebCore             	0x0000000112836711 WebCore::CachedResource::finishLoading(WebCore::SharedBuffer*) + 49 (CachedResource.cpp:314)
47  com.apple.WebCore             	0x000000011283214a WebCore::CachedRawResource::finishLoading(WebCore::SharedBuffer*) + 218 (CachedRawResource.cpp:104)
48  com.apple.WebCore             	0x0000000114577295 WebCore::SubresourceLoader::didFinishLoading(double) + 517 (SubresourceLoader.cpp:374)
49  com.apple.WebKit              	0x000000010d7d4877 WebKit::WebResourceLoader::didFinishResourceLoad(double) + 151 (WebResourceLoader.cpp:156)
50  com.apple.WebKit              	0x000000010d7d9d43 void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>, 0ul>(WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>&&, std::index_sequence<0ul>) + 163 (HandleMessage.h:17)
51  com.apple.WebKit              	0x000000010d7d9c98 void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double), std::__1::tuple<double>, std::make_index_sequence<1ul> >(std::__1::tuple<double>&&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) + 88 (HandleMessage.h:23)
52  com.apple.WebKit              	0x000000010d7d8dcd void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(double)>(IPC::MessageDecoder&, WebKit::WebResourceLoader*, void (WebKit::WebResourceLoader::*)(double)) + 221 (HandleMessage.h:93)
53  com.apple.WebKit              	0x000000010d7d857c WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(IPC::Connection&, IPC::MessageDecoder&) + 636 (WebResourceLoaderMessageReceiver.cpp:68)
54  com.apple.WebKit              	0x000000010d0df410 WebKit::NetworkProcessConnection::didReceiveMessage(IPC::Connection&, IPC::MessageDecoder&) + 160 (NetworkProcessConnection.cpp:62)
55  com.apple.WebKit              	0x000000010ce96023 IPC::Connection::dispatchMessage(IPC::MessageDecoder&) + 51 (Connection.cpp:901)
56  com.apple.WebKit              	0x000000010ce8cf51 IPC::Connection::dispatchMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >) + 785 (Connection.cpp:933)
57  com.apple.WebKit              	0x000000010ce9661f IPC::Connection::dispatchOneMessage() + 1519 (Connection.cpp:962)
58  com.apple.WebKit              	0x000000010cea797d IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_10::operator()() const + 29 (Connection.cpp:895)
59  com.apple.WebKit              	0x000000010cea794d void std::__1::__invoke_void_return_wrapper<void>::__call<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_10&>(IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_10&&&) + 45 (__functional_base:441)
60  com.apple.WebKit              	0x000000010cea779c std::__1::__function::__func<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_10, std::__1::allocator<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::MessageDecoder, std::__1::default_delete<IPC::MessageDecoder> >)::$_10>, void ()>::operator()() + 44 (functional:1407)
61  com.apple.JavaScriptCore      	0x000000011066e68a std::__1::function<void ()>::operator()() const + 26 (functional:1793)
62  com.apple.JavaScriptCore      	0x0000000110c11fed WTF::RunLoop::performWork() + 621 (RunLoop.cpp:122)
63  com.apple.JavaScriptCore      	0x0000000110c125f4 WTF::RunLoop::performWork(void*) + 36 (RunLoopCF.cpp:38)
64  com.apple.CoreFoundation      	0x00007fff88dea621 __CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
65  com.apple.CoreFoundation      	0x00007fff88dc9e1c __CFRunLoopDoSources0 + 556
66  com.apple.CoreFoundation      	0x00007fff88dc933f __CFRunLoopRun + 927
67  com.apple.CoreFoundation      	0x00007fff88dc8d38 CFRunLoopRunSpecific + 296
68  com.apple.HIToolbox           	0x00007fff83b01d55 RunCurrentEventLoopInMode + 235
69  com.apple.HIToolbox           	0x00007fff83b01b8f ReceiveNextEventCommon + 432
70  com.apple.HIToolbox           	0x00007fff83b019cf _BlockUntilNextEventMatchingListInModeWithFilter + 71
71  com.apple.AppKit              	0x00007fff8a645f3a _DPSNextEvent + 1067
72  com.apple.AppKit              	0x00007fff8a645369 -[NSApplication _nextEventMatchingEventMask:untilDate:inMode:dequeue:] + 454
73  com.apple.AppKit              	0x00007fff8a639ecc -[NSApplication run] + 682
74  com.apple.AppKit              	0x00007fff8a603162 NSApplicationMain + 1176
75  libxpc.dylib                  	0x00007fff970904f2 _xpc_objc_main + 793
76  libxpc.dylib                  	0x00007fff9708ef1e xpc_main + 494
77  com.apple.WebKit.WebContent.Development	0x00000001089d3be1 main + 785 (XPCServiceMain.Development.mm:187)
78  libdyld.dylib                 	0x00007fff84d425ad start + 1

[Jiewen Tan]

Created attachment 263126 [details]
Patch

[Jiewen Tan]

Comment on attachment 263126 [details]
Patch

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

> Source/WebCore/editing/CompositeEditCommand.cpp:1561
> +    // <div><span>abs<\span><\div>: (<div>, offset, 0) == (<span>, before, 0)

Actually, in my understanding of the position rule, the (<div>, offset, 0) tuple is not allowed. As the offset type of anchor node is restricted to text nodes only. Apparently, <div> is not a text node. However, the text case can produce such a tuple so that I question that the root cause is hidden deeper in somewhere in the editing codes. This change might be just a temporary fix for this problem.

[Jiewen Tan]

Created attachment 263127 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 263126 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        However, I fix it in a way other then the Blink change since it doesn't touch the real problem.
> +        Yet, I am still questionning whether this fix really touches the root of the problem. I believe
> +        something is still hidden in the way to position a table html tag such that it requires the
> +        enhancement checking of equity.

You should explain what was going wrong in this code instead.
Saying that there could be other problems isn't anyway useful for readers of this patch.

> Source/WebCore/editing/CompositeEditCommand.cpp:1551
> +bool CompositeEditCommand::checkEquityOfVisiblePosition(const VisiblePosition& a, const VisiblePosition& b)

We should add a new helper function in Position.cpp or VisiblePosition.cpp instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:1556
> +    // basic check

This comment repeats the code. Remove.

> Source/WebCore/editing/CompositeEditCommand.cpp:1563
> +    if (aDeepEquivalent.anchorNode()->parentNode()->isEqualNode(bDeepEquivalent.anchorNode()))
> +        std::swap(aDeepEquivalent, bDeepEquivalent);

What's the point of this code?
Swapping two positions being compared doesn't do anything.
Also, "isEqualNode" exists for legacy DOM API.  Don't use that.
Use pointer comparison instead.

> Source/WebCore/editing/CompositeEditCommand.cpp:1568
> +    bool isEqualEh1 = bDeepEquivalent.anchorNode()->isEqualNode(aDeepEquivalent.anchorNode()->firstChild())
> +        && aDeepEquivalent.anchorType() == Position::AnchorType::PositionIsOffsetInAnchor
> +        && bDeepEquivalent.anchorType() == Position::AnchorType::PositionIsBeforeAnchor
> +        && !aDeepEquivalent.computeOffsetInContainerNode()
> +        && !bDeepEquivalent.computeOffsetInContainerNode();

This is too specific.  We should support each and every position type instead.

See how we use switch statements in
http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Position.cpp#L153
to handle each position type.

You effectively want to have nested switches that knows how to compare each position type with all other types.

[Ryosuke Niwa]

Comment on attachment 263127 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Enhance the equity checking of two visible positions in CompositeEditCommand::cleanupAfterDeletion.

This line should match the bug title.

> Source/WebCore/ChangeLog:9
> +        Test: editing/inserting/insert-html-crash-01.html

This line should appear below the long description but above the list of files surrounded by blank lines.
See other change log entries.

> LayoutTests/ChangeLog:3
> +        Enhance the equity checking of two visible positions in CompositeEditCommand::cleanupAfterDeletion.

Ditto about matching the bug title.

> LayoutTests/editing/inserting/insert-html-crash-01-expected.txt:4
> +PASS Not crashed
> +PASS successfullyParsed is true
> +
> +TEST COMPLETE

This output is very mysterious.  I can't tell what this test is testing at all.

> LayoutTests/editing/inserting/insert-html-crash-01.html:2
> +<script src="../../resources/js-test.js"></script>

Why does this need to be a js-test?

> LayoutTests/editing/inserting/insert-html-crash-01.html:4
> +Foo

Is this "Foo" really necessary?
If it is, can we replace it with a description as to what we're testing instead?

> LayoutTests/editing/inserting/insert-html-crash-01.html:16
> +testPassed('Not crashed');

You should be able to just do
document.write('Pass. WebKit did not crash');
instead.  Using js-test.js is an overkill here.

> LayoutTests/editing/inserting/insert-html-crash-01.html:18
> +if (window.testRunner)
> +    editable.outerHTML = '';

Do we really need to hide the content like this?

[Jiewen Tan]

Created attachment 263511 [details]
Patch

[Jiewen Tan]

Comment on attachment 263511 [details]
Patch

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

> LayoutTests/editing/inserting/insert-html-crash-01.html:1
> +<html>

For the test case, I don't think this test case itself is enough for testing the new Position::equals() method. Should I add new unit tests for the new method or layout tests?

[Build Bot]

Comment on attachment 263511 [details]
Patch

Attachment 263511 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/308984

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 263513 [details]
Archive of layout-test-results from ews101 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Build Bot]

Comment on attachment 263511 [details]
Patch

Attachment 263511 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/308986

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 263515 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Ryosuke Niwa]

What I meant is to update your change log's title, not bug's.  The bug title should always reflect the end-user impact, not what you're doing in your patch.

[Ryosuke Niwa]

Comment on attachment 263511 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Enhance the equal checking of two positions in Position::equals().

Please change this line to match the bug title.

First off, Position::equals doesn't exist so it doesn't make any sense to say we're enhancing it.
Secondly, we're introducing this function to fix a crash.
That's a lot more important information to convey.

DO NOT change the bug title in Bugzilla.
The old bug title (to which I've already reverted) was perfect.

> Source/WebCore/ChangeLog:7
> +        Reviewed by Ryosuke Niwa.

This line should remain "Reviewed by NOBODY (OOPS!)" until I r+ your patch.

> Source/WebCore/ChangeLog:11
> +        In the original operator== for Position type, it checks equity depending only on whether the
> +        tuple<anchor node, anchor type, offset> is equal. Since for any position in a DOM could have
> +        multiple tuple representations, it doesn't cover all the cases. This change enhances the check.

You're just repeating the code here.  There's no need to say what the code already says.
What you need to describe instead is why the test was crashing and how you're fixing it.

> Source/WebCore/dom/Position.cpp:1480
> +            return m_anchorNode == other.m_anchorNode && m_anchorNode->countChildNodes() == static_cast<unsigned>(m_offset);

We should probably asset that anchorNode() isn't a text node here
(probably above the switch statement)
since countChildNodes() is always 0 zero for text nodes and it doesn't make any sense
to have a position after/before children which is anchored at a text node.

> Source/WebCore/dom/Position.cpp:1491
> +        case PositionIsAfterChildren:

Ditto about asserting that other.m_anchorNode isn't a text node.

> Source/WebCore/dom/Position.cpp:1504
> +            return m_anchorNode == other.m_anchorNode->firstChild();

I don't think other.m_anchorNode is guaranteed to be non-null.
You should check the nullity first.

> Source/WebCore/dom/Position.cpp:1508
> +            return m_anchorNode == other.m_anchorNode->traverseToChildAt(other.m_offset);

Ditto.

> Source/WebCore/dom/Position.cpp:1520
> +            return m_anchorNode == other.m_anchorNode->lastChild();

Ditto.

> Source/WebCore/dom/Position.cpp:1522
> +            return other.m_offset && m_anchorNode == other.m_anchorNode->traverseToChildAt(other.m_offset - 1);

Ditto.

> Source/WebCore/dom/Position.h:211
> +    // This is a tentative enhancement of operator== to treat different representations of the same position equal.

"treat different representations of the same position equal" is a bit wordy.
"to account for different position types"?

>> LayoutTests/editing/inserting/insert-html-crash-01.html:1
>> +<html>
> 
> For the test case, I don't think this test case itself is enough for testing the new Position::equals() method. Should I add new unit tests for the new method or layout tests?

Since Position::equals isn't exposed to the testing harness, that's going to be hard.
I think it's okay to keep it untested for now.  The test coverage will improve as we deploy it in more places.

[Jiewen Tan]

Created attachment 263601 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 263601 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        deletes the destination node. This abnormality happens as a result of the incomplete
> +        equal checking of operator== of type Position in cleanupAfterDeletion() method. Therefore,

You should describe how "incomplete" it is, and why that particular check failing would result in the destination node being removed
referring to function names and line numbers of the code, etc... as needed.

> Source/WebCore/ChangeLog:13
> +        this change adds Position::equals() to fortify the equal checking of two positions.

"this" should be capitalized.

> Source/WebCore/editing/CompositeEditCommand.cpp:1124
> -    if (caretAfterDelete != destination && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
> +    if (!caretAfterDelete.deepEquivalent().equals(destination.deepEquivalent()) && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {

We should also be checking the affinity so we should add a new function on VisiblePosition that uses equals.
r- because of this.

> LayoutTests/editing/inserting/insert-html-crash-01.html:1
> +<html>

Add <!DOCTYPE html> before <html> since we're not trying to test quirks mode.
When you make that change, be sure to test that the test still reproduces the crash.

[Jiewen Tan]

Comment on attachment 263601 [details]
Patch

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

>> Source/WebCore/editing/CompositeEditCommand.cpp:1124
>> +    if (!caretAfterDelete.deepEquivalent().equals(destination.deepEquivalent()) && isStartOfParagraph(caretAfterDelete) && isEndOfParagraph(caretAfterDelete)) {
> 
> We should also be checking the affinity so we should add a new function on VisiblePosition that uses equals.
> r- because of this.

I am not sure how to fortify the equal checking of VisiblePosition by considering the affinity. Could you explain it more detailed?

[Jiewen Tan]

The bug is related to Bug 149296.

[Jiewen Tan]

Created attachment 264407 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 264407 [details]
Patch

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

> Source/WebCore/ChangeLog:15
> +        THIS change adds Position::equals() to fortify the equal checking of two positions by

Why is "THIS" capitalized?

> Source/WebCore/ChangeLog:19
> +        Besides, it also adds VisiblePosition::equals() to consider affinity while comparing
> +        two visible positions.

This is sort of backwards.  The key of this bug fix is VisiblePosition::equals which replaced VisiblePosition::operator==.

> Source/WebCore/editing/VisiblePosition.cpp:750
> +    if (m_affinity == other.m_affinity)
> +        return m_deepPosition.equals(other.m_deepPosition);
> +    return false;

Why don't we do this in single line?
return m_affinity == other.m_affinity && m_deepPosition.equals(other.m_deepPosition) ?

[Jiewen Tan]

Created attachment 264616 [details]
Patch

[Build Bot]

Comment on attachment 264616 [details]
Patch

Attachment 264616 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/373430

New failing tests:
storage/indexeddb/modern/idbobjectstore-count-failures.html

[Build Bot]

Created attachment 264625 [details]
Archive of layout-test-results from ews103 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Ryosuke Niwa]

Comment on attachment 264616 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        it fails to determine that caretAfterDelete equals to destination as operator== for
> +        Position only checks the equality of tuple <Anchor Node, Anchor Type, Offset>. It is

This statement is not quite right since we're using VisiblePosition::operator== in that function.

> Source/WebCore/ChangeLog:18
> +        Besides, it also replaces VisiblePosition::operator== with VisiblePosition::equals()

We don't quite replace VisiblePosition::operator== with VisiblePosition::equals since we're just adding the latter.

> LayoutTests/editing/inserting/insert-html-crash-01.html:5
> +

What's up with this blank line?

[Jiewen Tan]

Created attachment 264643 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 264643 [details]
Patch

Clearing flags on attachment: 264643

Committed r192170: <http://trac.webkit.org/changeset/192170>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.