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
<rdar://problem/22746352>
Created attachment 263755 [details] Patch
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.
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.
Created attachment 264111 [details] Patch
> 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.
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.
Created attachment 264809 [details] Patch
Comment on attachment 264809 [details] Patch Clearing flags on attachment: 264809 Committed r192043: <http://trac.webkit.org/changeset/192043>
All reviewed patches have been landed. Closing bug.