| Summary: | Null dereference loading Blink layout test editing/inserting/insert-html-crash-01.html | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jon Honeycutt <jhoneycutt> | ||||||||||||||||||||||||||
| Component: | HTML Editing | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||
| Severity: | Normal | CC: | buildbot, commit-queue, enrica, jiewen_tan, rniwa, webkit-bug-importer | ||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | BlinkMergeCandidate, HasReduction, InRadar | ||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||
Created attachment 262923 [details]
Patch
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
Created attachment 263126 [details]
Patch
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. Created attachment 263127 [details]
Patch
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. 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? Created attachment 263511 [details]
Patch
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? 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. 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
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. 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
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. 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. Created attachment 263601 [details]
Patch
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. 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? The bug is related to Bug 149296. Created attachment 264407 [details]
Patch
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) ? Created attachment 264616 [details]
Patch
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 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
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? Created attachment 264643 [details]
Patch
Comment on attachment 264643 [details] Patch Clearing flags on attachment: 264643 Committed r192170: <http://trac.webkit.org/changeset/192170> All reviewed patches have been landed. Closing bug. |
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