| Summary: | Nullptr crash in EditCommand::EditCommand via CompositeEditCommand::removeNode | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Component: | HTML Editing | Assignee: | Jack <shihchieh_lee> | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, ajuma, apinheiro, bfulgham, cdumez, cfleizach, dmazzoni, esprehn+autocc, eugenebut, ews-feeder, ews-watchlist, ggaren, japhet, jcraig, jdiggs, kangil.han, megan_gardner, mifenton, product-security, samuel_white, sdefresne, shihchieh_lee, webkit-bug-importer, wenson_hsieh, zalan | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Description
Ryosuke Niwa
2020-02-11 16:43:22 PST
Created attachment 390468 [details]
Test case
Created attachment 392594 [details]
Another test case
This is another test case where m_frame is null in EditCommand::EditCommand, though the rest of the stack is different. The stack here is:
#0 0x3348786f1 in std::__1::unique_ptr<WebCore::FrameSelection, std::__1::default_delete<WebCore::FrameSelection> >::operator*() const (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0xcfe6f1)
#1 0x336dd5cc9 in WebCore::EditCommand::EditCommand(WebCore::Document&, WebCore::EditAction) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x325bcc9)
#2 0x336d94678 in WebCore::CompositeEditCommand::CompositeEditCommand(WebCore::Document&, WebCore::EditAction) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x321a678)
#3 0x336e679a1 in WebCore::RemoveNodePreservingChildrenCommand::RemoveNodePreservingChildrenCommand(WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&, WebCore::ShouldAssumeContentIsAlwaysEditable, WebCore::EditAction) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32ed9a1)
#4 0x336daf847 in WebCore::RemoveNodePreservingChildrenCommand::create(WTF::Ref<WebCore::Node, WTF::DumbPtrTraits<WebCore::Node> >&&, WebCore::ShouldAssumeContentIsAlwaysEditable, WebCore::EditAction) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3235847)
#5 0x336d9f2f0 in WebCore::CompositeEditCommand::removeNodePreservingChildren(WebCore::Node&, WebCore::ShouldAssumeContentIsAlwaysEditable) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32252f0)
#6 0x336dd3179 in WebCore::DeleteSelectionCommand::removeRedundantBlocks() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3259179)
#7 0x336dd37da in WebCore::DeleteSelectionCommand::doApply() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32597da)
#8 0x336dae3ba in WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::DumbPtrTraits<WebCore::EditCommand> >&&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32343ba)
#9 0x336da957d in WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x322f57d)
#10 0x336db4de0 in WebCore::CompositeEditCommand::moveParagraphs(WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, WebCore::VisiblePosition const&, bool, bool) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x323ade0)
#11 0x336e57cee in WebCore::InsertListCommand::listifyParagraph(WebCore::VisiblePosition const&, WebCore::QualifiedName const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32ddcee)
#12 0x336e56393 in WebCore::InsertListCommand::doApplyForSingleParagraph(bool, WebCore::HTMLQualifiedName const&, WebCore::Range*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32dc393)
#13 0x336e5556e in WebCore::InsertListCommand::doApply() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32db56e)
#14 0x336d90476 in WebCore::CompositeEditCommand::apply() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3216476)
#15 0x336e3e530 in WebCore::executeInsertUnorderedList(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x32c4530)
#16 0x336acdc91 in WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f53c91)
#17 0x334587800 in WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0xa0d800)
#18 0x334444625 in long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x8ca625)
#19 0x2a3107c01177 (<unknown module>)
#20 0x34e6db45b in llint_entry (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa8c45b)
#21 0x34e6c43d8 in vmEntryToJavaScript (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0xa753d8)
#22 0x34fcec937 in JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x209d937)
#23 0x350318140 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9140)
#24 0x350318242 in JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c9242)
#25 0x35031861f in JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/JavaScriptCore.framework/Versions/A/JavaScriptCore:x86_64+0x26c961f)
#26 0x33641401b in WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x289a01b)
#27 0x33643d071 in WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x28c3071)
#28 0x336bdf35b in WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc>, WebCore::EventTarget::EventInvokePhase) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x306535b)
#29 0x336bda6f2 in WebCore::EventTarget::fireEventListeners(WebCore::Event&, WebCore::EventTarget::EventInvokePhase) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x30606f2)
#30 0x33795a69d in WebCore::DOMWindow::dispatchEvent(WebCore::Event&, WebCore::EventTarget*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3de069d)
#31 0x33796bfa8 in WebCore::DOMWindow::dispatchLoadEvent() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3df1fa8)
#32 0x336ab7180 in WebCore::Document::dispatchWindowLoadEvent() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f3d180)
#33 0x336ab6af0 in WebCore::Document::implicitClose() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f3caf0)
#34 0x3377a4bc2 in WebCore::FrameLoader::checkCompleted() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x3c2abc2)
#35 0x336a9d00e in WebCore::Document::loadEventDelayTimerFired() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x2f2300e)
#36 0x337cb6f06 in WebCore::ThreadTimers::sharedTimerFiredInternal() (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x413cf06)
#37 0x337d2e40e in WebCore::timerFired(__CFRunLoopTimer*, void*) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebCore.framework/Versions/A/WebCore:x86_64+0x41b440e)
#38 0x7fff35c0fe14 in __CFRUNLOOP_IS_CALLING_OUT_TO_A_TIMER_CALLBACK_FUNCTION__ (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x59e14)
#39 0x7fff35c0f9c0 in __CFRunLoopDoTimer (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x599c0)
#40 0x7fff35c0f4f9 in __CFRunLoopDoTimers (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x594f9)
#41 0x7fff35bf0b33 in __CFRunLoopRun (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3ab33)
#42 0x7fff35bf0084 in CFRunLoopRunSpecific (/System/Library/Frameworks/CoreFoundation.framework/Versions/A/CoreFoundation:x86_64+0x3a084)
#43 0x7fff37e64a9e in -[NSRunLoop(NSRunLoop) runMode:beforeDate:] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x1ca9e)
#44 0x7fff37e64973 in -[NSRunLoop(NSRunLoop) run] (/System/Library/Frameworks/Foundation.framework/Versions/C/Foundation:x86_64+0x1c973)
#45 0x7fff622dc1d6 in _xpc_objc_main (/usr/lib/system/libxpc.dylib:x86_64+0x111d6)
#46 0x7fff622dbcd8 in xpc_main (/usr/lib/system/libxpc.dylib:x86_64+0x10cd8)
#47 0x328904465 in WebKit::XPCServiceMain(int, char const**) (/Users/chrome-bot/clusterfuzz/bot/builds/mac_asan_webkit/custom/Release/WebKit.framework/Versions/A/WebKit:x86_64+0x904465)
#48 0x7fff620a93d4 in start (/usr/lib/system/libdyld.dylib:x86_64+0x163d4)
Based on Jack’s analysis, the issue here is that ApplyStyleCommand is removing an iframe and unload event handler is detaching the document in which this command is running from its frame. As a result, ApplyStyleCommand’s call to appendNode hits a crash in EditCommand’s constructor as frame is null. EditCommand::frame which returns Frame& doesn’t make much sense either because there is no gurantee that Document this command is associated with will continue to have a Frame. Created attachment 396822 [details]
Patch
For regression test and review. (In reply to Jack from comment #4) > Created attachment 396822 [details] > Patch Created attachment 396826 [details]
Patch
In this case there is reentrancy in nested JS commands. And in the second JS command, document is detached from frame, so when the first JS command continues and need to create an EditCommand (in this case appendNode), the code crashes.
Geoff's idea is to move selection and editor to document, so they are not depending on frame, and by definition editing commands should be applied to document, not frame.
There will be several incremental changes to fully separate edit commands from frame. In this patch, we first move selection to document so EditCommnd will not dereference selection from frame.
Majority or the EditCommand APIs are still passing frame as argument, so frame.selection/editor are still defined but is redirected to document's selection/editor.
And since RefPtr on document is defined in EditCommand and Editor::command, there is no need for protection on document access in EditCommand. However, in Editor and frameSelection as well as some other classes, RefPtr on document are added along side the RefPtr on frame.
Besides, RefPtr on frame remains in most of the files since I am not sure if frame are still being accessed in the call stack.
Lastly, three editor functions are called in frameLoader at the time document has been detached:
m_frame.editor().clearUndoRedoOperations();
m_frame.editor().clearLastEditCommand();
m_frame.editor().clear();
However, I am not sure if they should be called in document. It seems that document and editor have same life cycle, so maybe clear is not needed?
Comment on attachment 396826 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396826&action=review Fundamentally, if a document does not have a frame then there is no visual representation (no render tree, etc etc) for editing operations and selection. So, no need for editor, selection etc. First glance seems mind boggling to invert this relationship. Possibly memory impact and perf regression could fall out as well > Source/WebCore/editing/Editor.cpp:1206 > + , m_frame(*m_document.frame()) A frame always has a document (*). But a document may NOT have a frame. Same comments for all the times frame is deferenced and stored below. (*) just for me: there is a very tiny window of less than 5 source lines where this isn't true: a frame can have no document. Doesn't effect the general rule: a frame always has a document. Daniel, really appreciate your spending time to review the huge change. There are many potential reentrancy like this so we though it would be better to change it once than to patch as fuzzer finds more similar holes. For dereferencing m_document.frame() in editor, it is going to be removed next upload. I kept them temporarily to see if it makes difference in regression tests. For memory impact, hope there would not be many detached documents? (In reply to Daniel Bates from comment #8) > Comment on attachment 396826 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=396826&action=review > > Fundamentally, if a document does not have a frame then there is no visual > representation (no render tree, etc etc) for editing operations and > selection. So, no need for editor, selection etc. First glance seems mind > boggling to invert this relationship. Possibly memory impact and perf > regression could fall out as well > > > Source/WebCore/editing/Editor.cpp:1206 > > + , m_frame(*m_document.frame()) > > A frame always has a document (*). But a document may NOT have a frame. Same > comments for all the times frame is deferenced and stored below. > > (*) just for me: there is a very tiny window of less than 5 source lines > where this isn't true: a frame can have no document. Doesn't effect the > general rule: a frame always has a document. Created attachment 396864 [details]
Patch
Looks like the right approach to me, but these test failures will need investigation: > editing/undo/undo-iframe-location-change.html Please look at the history of this test and find out why the author thought an undo operation should be prohibited after location change. It's possible that allowing the operation, now that operations are per-document, is OK. Or we may need to figure a way to prohibit operations after navigation. > fast/xsl/transform-to-html.xml crash > fast/xsl/xslt-transform-to-fragment-crash.html crash > http/tests/security/xss-DENIED-xsl-document.xml crash > imported/blink/editing/undo/crash-redo-with-iframes.html crash These crashes look like they're caused by this patch. > Fundamentally, if a document does not have a frame then there is no visual
> representation (no render tree, etc etc) for editing operations and
> selection. So, no need for editor, selection etc.
An exception to this general rule is the case where a document has a frame and starts an editing or selection operation, and then during the course of the operation it triggers a side effect that disconnects the document from the frame.
To address that exceptional case, the options we've considered so far are (a) null check the frame in every line of editing code (doable once but easy to forget in future); or (b) move these objects to Document.
To address the observation that some Documents never have a visual representation, perhaps we should consider null checking the frame in the Document constructor, and leaving these objects null when the frame is null.
Created attachment 397042 [details]
Patch
Created attachment 397048 [details]
Patch
Created attachment 397122 [details]
Patch
Created attachment 397160 [details]
Patch
Created attachment 397166 [details]
Patch
Created attachment 397178 [details]
Patch
Solutions in the patch: 1. Restore FrameSelection state when frame is not changed: call setSelectionFromNone and setCaretVisible in constructor to restore the internal state of the FrameSelection. What didn't work: tried modifying function FocusController::setFocusedFrame such that it calls FrameSelection::setFocused(). Although it restores the state of frame selection and solves test failures related to Caret Visibility, tests in counting number of blur/focus start failing. Besides, it also requires that FrameSelection being constructed with m_focused = false, and that makes other tests fail. 2. Initial state of m_focused in FrameSelection constructor: need to check the focused state of the original frame (m_frame) and restore the state because FocusController::setFocusedFrame will ignore the focus state change request if frame is not changed. 3. Possible detachment of document from frame in Editor: m_frame in Editor is removed so it will not be misused when a document is detached. Dependency on frame/page is replaced with m_document.frame()/page(). Not every function checks if document is detached. However, they are null deref so there is no security concern. One such bug is fixed in Editor::applyStyle in this change. 4. Possible change of frame in EditorCommand: EditorCommand and its callers are tightly coupled with frame. Decoupling will take more considerations and it's better done incrementally. For the time being, it is safe to store m_frame since it is assumed to remain unchanged throughout the processing of the commands. Besides, to prevent UAF, m_frame and m_document are both constructed as RefPtr. There is also check if m_document is in the same frame in Editor::Command::execute (unlikely to fail). Created attachment 397318 [details]
Patch
In this patch, the failed test case in api-ios is fixed. The root cause is that in the original code "selection().prepareForDestruction()" is called after document gets detached, which makes WebEditorClient::respondToChangedSelection being skipped. Function respondToChangedSelection is required for restoring "m_hasPendingEditorStateUpdate" in WebPage in order for the EditorStateUpdate event to be delivered to other components. The fix is moving the function call into Document::prepareForDestruction since now selection is part of document. Besides, "editor().clear()" is also moved into the function. Created attachment 397362 [details]
Patch
In this patch the failed tests in previous upload are resolved. 1. Crash due to AlternativeTextUITimer fires after document is detached. Fix: stop AlternativeTextUITimer in Editor::clear(). 2. Extra selection change notification when a document is closed: because FrameSelection is associated with document now, its prepareForDestruction() is called when a document is closed, resulting in more selection change notifications. Fix: modify expected result for editing/pasteboard/drag-drop-dead-frame.html. Created attachment 397416 [details]
Patch
Created attachment 397417 [details]
Patch
Looks like editing/inserting/insert-list-then-edit-command-crash.html is still crashing on iOS-wk2. Comment on attachment 397417 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397417&action=review > Source/WebCore/dom/Document.cpp:2596 > + editor().clear(); > + selection().prepareForDestruction(); Are we sure we need this? Destruction should naturally clear selection and stop all timers, so I'm a little unclear on why we need this. > Source/WebCore/loader/FrameLoader.cpp:574 > + if (currentDocument) currentDocument needs to be RefPtr. stopLoading() will fire the JavaScript unload event, which can have arbitrary side-effects. > Source/WebCore/loader/FrameLoader.cpp:-646 > - m_frame.editor().clear(); Kind of nice that we can remove lines of code like this: Now, the new document in the new load naturally starts out with an empty editor state. > LayoutTests/platform/mac/editing/pasteboard/drag-drop-dead-frame-expected.txt:7 > +EDITING DELEGATE: webViewDidChangeSelection:WebViewDidChangeSelectionNotification It looks like this means that we now report document destruction as a selection change because we clear selection before destruction. Seems OK. eah, I just look at it. The root cause is this in function shouldChangeSelection.
Most likely it is in nested command execution this line of code is still using frame().
bool Editor::shouldChangeSelection (...) {
#if PLATFORM(IOS_FAMILY)
if (m_document.frame()->selectionChangeCallbacksDisabled())
return true;
#endif
}
Can we just return true or false when document is detached?
If we move function selectionChangeCallbacksDisabled to Document, callers like below needs to be modified. Document should have already been attached when this is called?
- (void)setSelectionChangeCallbacksDisabled:(BOOL)flag
{
WebCore::Frame *frame = core(self);
frame->setSelectionChangeCallbacksDisabled(flag);
}
(In reply to Geoffrey Garen from comment #26)
> Looks like editing/inserting/insert-list-then-edit-command-crash.html is
> still crashing on iOS-wk2.
(In reply to Geoffrey Garen from comment #27) > Comment on attachment 397417 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=397417&action=review > > > Source/WebCore/dom/Document.cpp:2596 > > + editor().clear(); > > + selection().prepareForDestruction(); > > Are we sure we need this? Destruction should naturally clear selection and > stop all timers, so I'm a little unclear on why we need this. Thanks. Yeah, the timers are being stopped in prepareForDestruction. FrameSelection only has default destructor. Should we move the code into destructor? > > Source/WebCore/loader/FrameLoader.cpp:574 > > + if (currentDocument) > > currentDocument needs to be RefPtr. stopLoading() will fire the JavaScript > unload event, which can have arbitrary side-effects. Got it. Thanks. I will add that. (In reply to Jack from comment #29) > (In reply to Geoffrey Garen from comment #27) > > Comment on attachment 397417 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=397417&action=review > > > > > Source/WebCore/dom/Document.cpp:2596 > > > + editor().clear(); > > > + selection().prepareForDestruction(); > > > > Are we sure we need this? Destruction should naturally clear selection and > > stop all timers, so I'm a little unclear on why we need this. > Thanks. Yeah, the timers are being stopped in prepareForDestruction. > FrameSelection only has default destructor. Should we move the code into > destructor? Sorry, I forgot that we need to stop the timer before detaching the document, otherwise some timer callback functions may still use frame(). > bool Editor::shouldChangeSelection (...) { > #if PLATFORM(IOS_FAMILY) > if (m_document.frame()->selectionChangeCallbacksDisabled()) > return true; > #endif > } > > Can we just return true or false when document is detached? Yes. Probably true. > If we move function selectionChangeCallbacksDisabled to Document, callers > like below needs to be modified. I don't think we want to move it, at least not right now. Moving this setting would change its meaning because it would cause the setting to disappear after navigation. That change might not be compatible with existing client expectations. > > > Source/WebCore/dom/Document.cpp:2596
> > > + editor().clear();
> > > + selection().prepareForDestruction();
> >
> > Are we sure we need this? Destruction should naturally clear selection and
> > stop all timers, so I'm a little unclear on why we need this.
> Thanks. Yeah, the timers are being stopped in prepareForDestruction.
> FrameSelection only has default destructor. Should we move the code into
> destructor?
I see. "prepareForDestruction" really should be named something like "willRemoveFromFrame". It's not directly tied to object lifetime, though they may coincidentally happen at the same time.
I don't think we should move this code into the Document destructor. That's worse. Generally, we want to avoid relying on object destruction to implement any observable behavior other than memory safety. The reason is that reference counted objects can be destroyed at unpredictable times -- especially if they are referenced by the JavaScript garbage collector -- so you can't rely on object destruction to guarantee any behavior that needs to happen at a particular time.
> > Can we just return true or false when document is detached?
>
> Yes. Probably true.
Wenson suggested false to match the status quo. I guess that's a little better.
Thanks! Got it! I will change the name off the function. (In reply to Geoffrey Garen from comment #32) > > > > Source/WebCore/dom/Document.cpp:2596 > > > > + editor().clear(); > > > > + selection().prepareForDestruction(); > > > > > > Are we sure we need this? Destruction should naturally clear selection and > > > stop all timers, so I'm a little unclear on why we need this. > > Thanks. Yeah, the timers are being stopped in prepareForDestruction. > > FrameSelection only has default destructor. Should we move the code into > > destructor? > > I see. "prepareForDestruction" really should be named something like > "willRemoveFromFrame". It's not directly tied to object lifetime, though > they may coincidentally happen at the same time. > > I don't think we should move this code into the Document destructor. That's > worse. Generally, we want to avoid relying on object destruction to > implement any observable behavior other than memory safety. The reason is > that reference counted objects can be destroyed at unpredictable times -- > especially if they are referenced by the JavaScript garbage collector -- so > you can't rely on object destruction to guarantee any behavior that needs to > happen at a particular time. Created attachment 397490 [details]
Patch
In this revision: 1. In function Editor::shouldChangeSelection, return false if document has been detached from frame. 2. Change function names: Frame::prepareForDestruction -> willBeRemovedFromDocument Document::prepareForDestruction -> willBeRemovedFromFrame (In reply to Jack from comment #35) > Created attachment 397490 [details] > Patch Sorry for the typo. Please see correction below: (In reply to Jack from comment #36) > In this revision: > 1. In function Editor::shouldChangeSelection, return false if document has > been detached from frame. > 2. Change function names: > FrameSelection::prepareForDestruction -> willBeRemovedFromDocument > Document::prepareForDestruction -> willBeRemovedFromFrame > > (In reply to Jack from comment #35) > > Created attachment 397490 [details] > > Patch Comment on attachment 397490 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397490&action=review > Source/WebCore/editing/FrameSelection.cpp:1577 > +void FrameSelection::willBeRemovedFromDocument() This should probably be named willBeRemovedFromFrame. A given selection object is always a member of its document. Created attachment 397526 [details]
Patch
That's right. Thanks. Please see the new revision.
> > +void FrameSelection::willBeRemovedFromDocument()
>
> This should probably be named willBeRemovedFromFrame. A given selection
> object is always a member of its document.
Comment on attachment 397526 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=397526&action=review I think this patch is ready, but I spotted two details that could use tidying. > Source/WebCore/editing/FrameSelection.h:143 > + WEBCORE_EXPORT explicit FrameSelection(Document* = nullptr); Is there actually a case where someone constructs a FrameSelection with a null Document? Since FrameSelection is now a data member of Document, I think we always have a Document. If so, it would be better to change the type of the constructor argument and data member to Document&. > Source/WebCore/editing/FrameSelection.h:342 > Frame* m_frame; Can you remove the m_frame data member now? Anything that used to use m_frame should ideally be gone now. If uses remain, they should use m_document->frame() will a null check. (In reply to Geoffrey Garen from comment #41) > > Source/WebCore/editing/FrameSelection.h:143 > > + WEBCORE_EXPORT explicit FrameSelection(Document* = nullptr); > > Is there actually a case where someone constructs a FrameSelection with a > null Document? Since FrameSelection is now a data member of Document, I > think we always have a Document. If so, it would be better to change the > type of the constructor argument and data member to Document&. Yeah, there are a few places in TypingCommand.cpp, WebFrame.mm and AccessibilityRenderObject.cpp where the selection is explicitly set after the construction. > > Source/WebCore/editing/FrameSelection.h:342 > > Frame* m_frame; > > Can you remove the m_frame data member now? Anything that used to use > m_frame should ideally be gone now. If uses remain, they should use > m_document->frame() will a null check. Yeah, that should be true. Let me try this. Thanks. > Yeah, there are a few places in TypingCommand.cpp, WebFrame.mm and > AccessibilityRenderObject.cpp where the selection is explicitly set after > the construction. I checked the DOMUIKitExtensions.mm case, and it looks like we could provide a document in the constructor (using the Range's document). TypingCommand has a Document as a data member. Perhaps all these cases can provide a Document in the constructor. > > > > Source/WebCore/editing/FrameSelection.h:342 > > > Frame* m_frame; > > > > Can you remove the m_frame data member now? Anything that used to use > > m_frame should ideally be gone now. If uses remain, they should use > > m_document->frame() will a null check. > Yeah, that should be true. Let me try this. Thanks. (In reply to Geoffrey Garen from comment #43) > > Yeah, there are a few places in TypingCommand.cpp, WebFrame.mm and > > AccessibilityRenderObject.cpp where the selection is explicitly set after > > the construction. > > I checked the DOMUIKitExtensions.mm case, and it looks like we could provide > a document in the constructor (using the Range's document). TypingCommand > has a Document as a data member. Perhaps all these cases can provide a > Document in the constructor. Thanks. Yeah, I am trying this approach as well. Hopefully all have easy access to document. Created attachment 397731 [details]
Patch
Created attachment 397773 [details]
Patch
Resolve the merge conflicts in previous upload. Created attachment 397780 [details]
Patch
Add additional argument for document in wordSelectionContainingCaretSelection. Need this to instantiate FrameSelection. (In reply to Jack from comment #48) > Created attachment 397780 [details] > Patch Created attachment 397781 [details]
Patch
Created attachment 397795 [details]
Patch
Created attachment 397809 [details]
Patch
Isolate test failures in previous upload. First resolve the merge conflicts. (In reply to Jack from comment #52) > Created attachment 397809 [details] > Patch Comment on attachment 397809 [details]
Patch
I'll say r+ because this patch passed EWS and it seems ready to go. Please follow up to remove that m_frame data member, since that will be the compile-time verification that instances of this bug are gone.
Thanks. Yes. Currently am resolving the test failures and hope to submit the rest of the changes shortly. (In reply to Geoffrey Garen from comment #54) > Comment on attachment 397809 [details] > Patch > > I'll say r+ because this patch passed EWS and it seems ready to go. Please > follow up to remove that m_frame data member, since that will be the > compile-time verification that instances of this bug are gone. Committed r260831: <https://trac.webkit.org/changeset/260831> All reviewed patches have been landed. Closing bug and clearing flags on attachment 397809 [details]. Reopening to attach new patch. Created attachment 397872 [details]
Patch
Created attachment 397889 [details]
Patch
Created attachment 398025 [details]
Patch
In this revision: 1. Data member m_frame in FrameSelection is removed. 2. Improvement of test run time (from Alan). (In reply to Jack from comment #60) > Created attachment 398025 [details] > Patch Also tried changing m_document in FrameSelection to reference member, instead of pointer. However, some tests fail if we try to pass (to FrameSelection constructor) the document in the context where FrameSelection is instantiated. Doing so change the behavior in some member functions in FrameSelection. In the original code, those functions null-check document/frame and return early. It seems that the document/frame pointer is overloaded as a flag for certain behavior. We can create a flag when calling the constructor and do the null-checks. I experimented it and the code passes all the failed cases. However, there is no clear definition for when the flag should be set. And it is weird for the caller to pass parameters of document along with a flag saying don’t use the document. Therefore, we decided to only remove m_frame in FrameSelection for now. Created attachment 398028 [details]
Patch
The previous patch failed in editing/selection/focus-iframe-removal-crash.html.
<iframe></iframe>
<script>
if (window.testRunner)
testRunner.dumpAsText();
function run()
{
var iframe = document.getElementsByTagName('iframe')[0];
iframe.contentDocument.documentElement.contentEditable = true;
iframe.contentDocument.documentElement.addEventListener('focusout', function () {
iframe.parentNode.removeChild(iframe);
}, false);
iframe.contentDocument.documentElement.focus();
document.write("PASS. WebKit didn't crash.");
}
document.addEventListener('DOMContentLoaded', run);
</script>
Root cause:
This is another reentrancy issue.
1. Focus command (frame #45) is followed by removeChild (frame #13).
2. Document is detached before Focus can finish.
3. Code crashes when we access frame() in function selectFrameElementInParentIfFullySelected.
* frame #0: 0x000000044bc7d54c WebCore`WebCore::FrameDestructionObserver::observeFrame(this={ origin = file://, url = about:blank, inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }, frame={ origin = , url = , isMainFrame = 0, backForwardCacheState = }) at FrameDestructionObserver.cpp:50:13
frame #1: 0x0000000449e6607b WebCore`WebCore::Document::detachFromFrame(this={ origin = file://, url = about:blank, inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }) at Document.cpp:8284:5
frame #2: 0x0000000449e674c1 WebCore`WebCore::Document::willBeRemovedFromFrame(this={ origin = file://, url = about:blank, inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }) at Document.cpp:2599:5
frame #3: 0x000000044bc6df73 WebCore`WebCore::Frame::setView(this={ origin = file://, url = about:blank, isMainFrame = 0, backForwardCacheState = NotInBackForwardCache }, view=0x00007ffee3c1d8a0) at Frame.cpp:235:16
frame #4: 0x000000044b824bb1 WebCore`WebCore::FrameLoader::closeAndRemoveChild(this=0x0000614000007440, child={ origin = file://, url = about:blank, isMainFrame = 0, backForwardCacheState = NotInBackForwardCache }) at FrameLoader.cpp:2738:11
frame #5: 0x000000044b8248d5 WebCore`WebCore::FrameLoader::detachFromParent(this=0x0000614000051c40) at FrameLoader.cpp:2858:26
frame #6: 0x000000044b825ce4 WebCore`WebCore::FrameLoader::frameDetached(this=0x0000614000051c40) at FrameLoader.cpp:2833:5
frame #7: 0x000000044a92304b WebCore`WebCore::HTMLFrameOwnerElement::disconnectContentFrame(this=0x000061300005be40) at HTMLFrameOwnerElement.cpp:80:25
frame #8: 0x0000000449da300b WebCore`WebCore::disconnectSubframes(root=0x000061300005be40, policy=RootAndDescendants) at ContainerNodeAlgorithms.cpp:308:25
frame #9: 0x0000000449d97485 WebCore`WebCore::disconnectSubframesIfNeeded(root=0x000061300005be40, policy=RootAndDescendants) at ContainerNodeAlgorithms.h:78:5
frame #10: 0x0000000449d97849 WebCore`WebCore::ContainerNode::removeNodeWithScriptAssertion(this=0x000060c000085300, childToRemove=0x000061300005be40, source=API) at ContainerNode.cpp:145:13
frame #11: 0x0000000449d96889 WebCore`WebCore::ContainerNode::removeChild(this=0x000060c000085300, oldChild=0x000061300005be40) at ContainerNode.cpp:577:10
frame #12: 0x000000044a1c4868 WebCore`WebCore::Node::removeChild(this=0x000060c000085300, oldChild=0x000061300005be40) at Node.cpp:490:43
frame #13: 0x0000000445d75db9 WebCore`WebCore::jsNodePrototypeFunctionRemoveChildBody(lexicalGlobalObject=0x000061f000032ce8, callFrame=0x00007ffee3c1e820, castedThis=0x000060e000064c68, throwScope=0x00007ffee3c1e660) at JSNode.cpp:956:63
frame #14: 0x0000000445c5c0de WebCore`long long WebCore::IDLOperation<WebCore::JSNode>::call<&(lexicalGlobalObject=0x000061f000032ce8, callFrame=0x00007ffee3c1e820, operationName="removeChild")), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) at JSDOMOperation.h:53:16
frame #15: 0x0000000445c5bc34 WebCore`WebCore::jsNodePrototypeFunctionRemoveChild(lexicalGlobalObject=0x000061f000032ce8, callFrame=0x00007ffee3c1e820) at JSNode.cpp:962:12
frame #16: 0x00005e9a69801178
frame #17: 0x0000000474896c3e JavaScriptCore`llint_entry at LowLevelInterpreter.asm:1045
frame #18: 0x0000000474877562 JavaScriptCore`vmEntryToJavaScript at LowLevelInterpreter64.asm:296
frame #19: 0x0000000476d8fd90 JavaScriptCore`JSC::JITCode::execute(this=0x00006040000e7290, vm=0x000062f00001c400, protoCallFrame=0x00007ffee3c1edc0) at JITCodeInlines.h:38:38
frame #20: 0x0000000476d91107 JavaScriptCore`JSC::Interpreter::executeCall(this=0x00006020000157f0, lexicalGlobalObject=0x000061f00003fee8, function=0x000062d0001d3d00, callData=0x00007ffee3c1fd00, thisValue=JSValue @ 0x00007ffee3c1ebe0, args=0x00007ffee3c20040) at Interpreter.cpp:921:31
frame #21: 0x0000000477724160 JavaScriptCore`JSC::call(globalObject=0x000061f00003fee8, functionObject=JSValue @ 0x00007ffee3c1f220, callData=0x00007ffee3c1fd00, thisValue=JSValue @ 0x00007ffee3c1f240, args=0x00007ffee3c20040) at CallData.cpp:58:28
frame #22: 0x0000000477724672 JavaScriptCore`JSC::call(globalObject=0x000061f00003fee8, functionObject=JSValue @ 0x00007ffee3c1f420, callData=0x00007ffee3c1fd00, thisValue=JSValue @ 0x00007ffee3c1f440, args=0x00007ffee3c20040, returnedException=0x00007ffee3c1ffc0) at CallData.cpp:65:22
frame #23: 0x000000047772531a JavaScriptCore`JSC::profiledCall(globalObject=0x000061f00003fee8, reason=Other, functionObject=JSValue @ 0x00007ffee3c1f720, callData=0x00007ffee3c1fd00, thisValue=JSValue @ 0x00007ffee3c1f740, args=0x00007ffee3c20040, returnedException=0x00007ffee3c1ffc0) at CallData.cpp:86:12
frame #24: 0x000000044919756e WebCore`WebCore::JSExecState::profiledCall(lexicalGlobalObject=0x000061f00003fee8, reason=Other, functionObject=JSValue @ 0x00007ffee3c1f9a0, callData=0x00007ffee3c1fd00, thisValue=JSValue @ 0x00007ffee3c1f9c0, args=0x00007ffee3c20040, returnedException=0x00007ffee3c1ffc0) at JSExecState.h:73:16
frame #25: 0x00000004491e631e WebCore`WebCore::JSEventListener::handleEvent(this=0x00006060000bab60, scriptExecutionContext={ origin = file://, url = about:blank, inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }, event=0x000060b00005c9d0) at JSEventListener.cpp:179:22
frame #26: 0x000000044a0db8cb WebCore`WebCore::EventTarget::innerInvokeEventListeners(this=0x000060c000086e00, event=0x000060b00005c9d0, listeners={ size = 1, capacity = 1 }, phase=Bubbling) at EventTarget.cpp:335:40
frame #27: 0x000000044a0d20b4 WebCore`WebCore::EventTarget::fireEventListeners(this=0x000060c000086e00, event=0x000060b00005c9d0, phase=Bubbling) at EventTarget.cpp:267:9
frame #28: 0x000000044a1db9e6 WebCore`WebCore::Node::handleLocalEvents(this=0x000060c000086e00, event=0x000060b00005c9d0, phase=Bubbling) at Node.cpp:2371:5
frame #29: 0x000000044a0a89cf WebCore`WebCore::EventContext::handleLocalEvents(this=0x00006040000fe9d0, event=0x000060b00005c9d0, phase=Bubbling) const at EventContext.cpp:55:17
frame #30: 0x000000044a0a8d54 WebCore`WebCore::MouseOrFocusEventContext::handleLocalEvents(this=0x00006040000fe9d0, event=0x000060b00005c9d0, phase=Bubbling) const at EventContext.cpp:81:19
frame #31: 0x000000044a0a9ebe WebCore`WebCore::dispatchEventInDOM(event=0x000060b00005c9d0, path=0x00007ffee3c20ea0) at EventDispatcher.cpp:100:22
frame #32: 0x000000044a0a96fb WebCore`WebCore::EventDispatcher::dispatchEvent(node=0x000060c000086e00, event=0x000060b00005c9d0) at EventDispatcher.cpp:154:9
frame #33: 0x000000044a1dba3d WebCore`WebCore::Node::dispatchEvent(this=0x000060c000086e00, event=0x000060b00005c9d0) at Node.cpp:2381:5
frame #34: 0x000000044a27fa64 WebCore`WebCore::ScopedEventQueue::dispatchEvent(this=0x00000004551d8460, event=0x000060b00005c9d0) const at ScopedEventQueue.cpp:57:37
frame #35: 0x000000044a27f96e WebCore`WebCore::ScopedEventQueue::enqueueEvent(this=0x00000004551d8460, event=0x00007ffee3c212c0) at ScopedEventQueue.cpp:52:9
frame #36: 0x000000044a0a8eeb WebCore`WebCore::EventDispatcher::dispatchScopedEvent(node=0x000060c000086e00, event=0x000060b00005c9d0) at EventDispatcher.cpp:52:35
frame #37: 0x000000044a1dba0d WebCore`WebCore::Node::dispatchScopedEvent(this=0x000060c000086e00, event=0x000060b00005c9d0) at Node.cpp:2376:5
frame #38: 0x000000044a029881 WebCore`WebCore::Element::dispatchFocusOutEvent(this=0x000060c000086e00, eventType={ length = 8, contents = 'focusout' }, newFocusedElement=0x00007ffee3c215c0) at Element.cpp:3095:5
frame #39: 0x0000000449e833ee WebCore`WebCore::Document::setFocusedElement(this={ origin = file://, url = about:blank, inMainFrame = Detached, backForwardCacheState = NotInBackForwardCache }, element=0x000060c000086f80, direction=FocusDirectionNone, eventsMode=Dispatch) at Document.cpp:4306:32
frame #40: 0x000000044bc4a066 WebCore`WebCore::FocusController::setFocusedElement(this=0x000060700001f7b0, element=0x000060c000086f80, newFocusedFrame={ origin = file://, url = about:blank, isMainFrame = 0, backForwardCacheState = NotInBackForwardCache }, direction=FocusDirectionNone) at FocusController.cpp:864:45
frame #41: 0x000000044a55ea34 WebCore`WebCore::FrameSelection::setFocusedElementIfNeeded(this=0x00006120000568c0) at FrameSelection.cpp:2268:55
frame #42: 0x000000044a55d3f6 WebCore`WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(this=0x00006120000568c0, newSelectionPossiblyWithoutDirection=0x00007ffee3c22760, options={ size = 2 }, align=AlignCursorOnScrollIfNeeded, granularity=CharacterGranularity) at FrameSelection.cpp:385:9
frame #43: 0x000000044a50705a WebCore`WebCore::FrameSelection::setSelection(this=0x00006120000568c0, selection=0x00007ffee3c22760, options={ size = 2 }, intent=AXTextStateChangeIntent @ 0x00007ffee3c22350, align=AlignCursorOnScrollIfNeeded, granularity=CharacterGranularity) at FrameSelection.cpp:406:10
frame #44: 0x000000044a02892b WebCore`WebCore::Element::updateFocusAppearance(this=0x000060c000086e00, (null)=Restore, revealMode=Reveal) at Element.cpp:3064:32
frame #45: 0x000000044a027877 WebCore`WebCore::Element::focus(this=0x000060c000086e00, restorePreviousSelection=true, direction=FocusDirectionNone) at Element.cpp:3040:1
frame #46: 0x000000044537e196 WebCore`WebCore::jsHTMLElementPrototypeFunctionFocusBody(lexicalGlobalObject=0x000061f00003fee8, callFrame=0x00007ffee3c22f90, castedThis=0x000060e000062f88, throwScope=0x00007ffee3c22dc0) at JSHTMLElement.cpp:4090:10
frame #47: 0x00000004451c216e WebCore`long long WebCore::IDLOperation<WebCore::JSHTMLElement>::call<&(lexicalGlobalObject=0x000061f00003fee8, callFrame=0x00007ffee3c22f90, operationName="focus")), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) at JSDOMOperation.h:53:16
frame #48: 0x00000004451c1cc4 WebCore`WebCore::jsHTMLElementPrototypeFunctionFocus(lexicalGlobalObject=0x000061f00003fee8, callFrame=0x00007ffee3c22f90) at JSHTMLElement.cpp:4096:12
frame #49: 0x00005e9a69801178
frame #50: 0x0000000474896c3e JavaScriptCore`llint_entry at LowLevelInterpreter.asm:1045
frame #51: 0x0000000474877562 JavaScriptCore`vmEntryToJavaScript at LowLevelInterpreter64.asm:296
frame #52: 0x0000000476d8fd90 JavaScriptCore`JSC::JITCode::execute(this=0x00006040000e7290, vm=0x000062f00001c400, protoCallFrame=0x00007ffee3c23520) at JITCodeInlines.h:38:38
frame #53: 0x0000000476d91107 JavaScriptCore`JSC::Interpreter::executeCall(this=0x00006020000157f0, lexicalGlobalObject=0x000061f000032ce8, function=0x000062d0001809c0, callData=0x00007ffee3c24460, thisValue=JSValue @ 0x00007ffee3c23340, args=0x00007ffee3c247a0) at Interpreter.cpp:921:31
frame #54: 0x0000000477724160 JavaScriptCore`JSC::call(globalObject=0x000061f000032ce8, functionObject=JSValue @ 0x00007ffee3c23980, callData=0x00007ffee3c24460, thisValue=JSValue @ 0x00007ffee3c239a0, args=0x00007ffee3c247a0) at CallData.cpp:58:28
frame #55: 0x0000000477724672 JavaScriptCore`JSC::call(globalObject=0x000061f000032ce8, functionObject=JSValue @ 0x00007ffee3c23b80, callData=0x00007ffee3c24460, thisValue=JSValue @ 0x00007ffee3c23ba0, args=0x00007ffee3c247a0, returnedException=0x00007ffee3c24720) at CallData.cpp:65:22
frame #56: 0x000000047772531a JavaScriptCore`JSC::profiledCall(globalObject=0x000061f000032ce8, reason=Other, functionObject=JSValue @ 0x00007ffee3c23e80, callData=0x00007ffee3c24460, thisValue=JSValue @ 0x00007ffee3c23ea0, args=0x00007ffee3c247a0, returnedException=0x00007ffee3c24720) at CallData.cpp:86:12
frame #57: 0x000000044919756e WebCore`WebCore::JSExecState::profiledCall(lexicalGlobalObject=0x000061f000032ce8, reason=Other, functionObject=JSValue @ 0x00007ffee3c24100, callData=0x00007ffee3c24460, thisValue=JSValue @ 0x00007ffee3c24120, args=0x00007ffee3c247a0, returnedException=0x00007ffee3c24720) at JSExecState.h:73:16
frame #58: 0x00000004491e631e WebCore`WebCore::JSEventListener::handleEvent(this=0x00006060000ba020, scriptExecutionContext={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000060700003c930) at JSEventListener.cpp:179:22
frame #59: 0x000000044a0db8cb WebCore`WebCore::EventTarget::innerInvokeEventListeners(this={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000060700003c930, listeners={ size = 1, capacity = 1 }, phase=Bubbling) at EventTarget.cpp:335:40
frame #60: 0x000000044a0d20b4 WebCore`WebCore::EventTarget::fireEventListeners(this={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000060700003c930, phase=Bubbling) at EventTarget.cpp:267:9
frame #61: 0x000000044a1db9e6 WebCore`WebCore::Node::handleLocalEvents(this={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000060700003c930, phase=Bubbling) at Node.cpp:2371:5
frame #62: 0x000000044a0a89cf WebCore`WebCore::EventContext::handleLocalEvents(this=0x00006040000e6e90, event=0x000060700003c930, phase=Bubbling) const at EventContext.cpp:55:17
frame #63: 0x000000044a0a9ebe WebCore`WebCore::dispatchEventInDOM(event=0x000060700003c930, path=0x00007ffee3c255c0) at EventDispatcher.cpp:100:22
frame #64: 0x000000044a0a96fb WebCore`WebCore::EventDispatcher::dispatchEvent(node={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000060700003c930) at EventDispatcher.cpp:154:9
frame #65: 0x000000044a1dba3d WebCore`WebCore::Node::dispatchEvent(this={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }, event=0x000060700003c930) at Node.cpp:2381:5
frame #66: 0x0000000449e9c2ff WebCore`WebCore::Document::finishedParsing(this={ origin = file://, url = file:///Users/jacklee/browser2/OpenSource/LayoutTests/editing/selection/focus-iframe-removal-crash.html, inMainFrame = 1, backForwardCacheState = NotInBackForwardCache }) at Document.cpp:5821:5
frame #67: 0x000000044aeab228 WebCore`WebCore::HTMLConstructionSite::finishedParsing(this=0x0000612000054ae0) at HTMLConstructionSite.cpp:419:16
frame #68: 0x000000044af32c80 WebCore`WebCore::HTMLTreeBuilder::finished(this=0x0000612000054ac0) at HTMLTreeBuilder.cpp:2843:12
frame #69: 0x000000044aeb9742 WebCore`WebCore::HTMLDocumentParser::end(this=0x000062500022b100) at HTMLDocumentParser.cpp:449:20
frame #70: 0x000000044aeb5482 WebCore`WebCore::HTMLDocumentParser::attemptToRunDeferredScriptsAndEnd(this=0x000062500022b100) at HTMLDocumentParser.cpp:458:5
frame #71: 0x000000044aeb50d2 WebCore`WebCore::HTMLDocumentParser::prepareToStopParsing(this=0x000062500022b100) at HTMLDocumentParser.cpp:153:5
frame #72: 0x000000044aeb9861 WebCore`WebCore::HTMLDocumentParser::attemptToEnd(this=0x000062500022b100) at HTMLDocumentParser.cpp:470:5
frame #73: 0x000000044aeb9999 WebCore`WebCore::HTMLDocumentParser::finish(this=0x000062500022b100) at HTMLDocumentParser.cpp:498:5
frame #74: 0x000000044b713ccd WebCore`WebCore::DocumentWriter::end(this=0x000061f000031110) at DocumentWriter.cpp:288:15
frame #75: 0x000000044b711abf WebCore`WebCore::DocumentLoader::finishedLoading(this=0x000061f000031080) at DocumentLoader.cpp:452:14
frame #76: 0x000000044b710d76 WebCore`WebCore::DocumentLoader::notifyFinished(this=0x000061f000031080, resource=0x0000619000086b80) at DocumentLoader.cpp:396:9
frame #77: 0x000000044ba3e169 WebCore`WebCore::CachedResource::checkNotify(this=0x0000619000086b80) at CachedResource.cpp:376:17
frame #78: 0x000000044ba34948 WebCore`WebCore::CachedResource::finishLoading(this=0x0000619000086b80, (null)=0x00006060000a4180) at CachedResource.cpp:392:5
frame #79: 0x000000044ba3713d WebCore`WebCore::CachedRawResource::finishLoading(this=0x0000619000086b80, data=0x00006060000a4180) at CachedRawResource.cpp:123:21
frame #80: 0x000000044b939732 WebCore`WebCore::SubresourceLoader::didFinishLoading(this=0x000061a000019e80, networkLoadMetrics=0x00007ffee3c275e8) at SubresourceLoader.cpp:734:17
frame #81: 0x00000004332d818a WebKit`WebKit::WebResourceLoader::didFinishResourceLoad(this=0x0000608000044820, networkLoadMetrics=0x00007ffee3c275e8) at WebResourceLoader.cpp:251:19
frame #82: 0x00000004340f3b07 WebKit`void IPC::callMemberFunctionImpl<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, 0ul>(object=0x0000608000044820, function=b0 7a 2d 33 04 00 00 00 00 00 00 00 00 00 00 00, args=size=1, (null)=std::__1::index_sequence<0UL> @ 0x00007ffee3c273f8)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>&&, std::__1::integer_sequence<unsigned long, 0ul>) at HandleMessage.h:41:5
frame #83: 0x00000004340f37ee WebKit`void IPC::callMemberFunction<WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&), std::__1::tuple<WebCore::NetworkLoadMetrics>, std::__1::integer_sequence<unsigned long, 0ul> >(args=size=1, object=0x0000608000044820, function=b0 7a 2d 33 04 00 00 00 00 00 00 00 00 00 00 00)(WebCore::NetworkLoadMetrics const&)) at HandleMessage.h:47:5
frame #84: 0x00000004340ed860 WebKit`void IPC::handleMessage<Messages::WebResourceLoader::DidFinishResourceLoad, WebKit::WebResourceLoader, void (WebKit::WebResourceLoader::*)(WebCore::NetworkLoadMetrics const&)>(decoder=0x000060b0000503d0, object=0x0000608000044820, function=b0 7a 2d 33 04 00 00 00 00 00 00 00 00 00 00 00)(WebCore::NetworkLoadMetrics const&)) at HandleMessage.h:114:5
frame #85: 0x00000004340eaf5c WebKit`WebKit::WebResourceLoader::didReceiveWebResourceLoaderMessage(this=0x0000608000044820, connection=0x0000614000000e40, decoder=0x000060b0000503d0) at WebResourceLoaderMessageReceiver.cpp:70:9
frame #86: 0x0000000433251575 WebKit`WebKit::NetworkProcessConnection::didReceiveMessage(this=0x000060800000a820, connection=0x0000614000000e40, decoder=0x000060b0000503d0) at NetworkProcessConnection.cpp:91:32
frame #87: 0x000000043010a599 WebKit`IPC::Connection::dispatchMessage(this=0x0000614000000e40, decoder=0x000060b0000503d0) at Connection.cpp:1008:14
frame #88: 0x000000043010bff2 WebKit`IPC::Connection::dispatchMessage(this=0x0000614000000e40, message=unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> > @ 0x00007ffee3c293a0) at Connection.cpp:1077:9
frame #89: 0x000000043010e05d WebKit`IPC::Connection::dispatchOneIncomingMessage(this=0x0000614000000e40) at Connection.cpp:1146:5
frame #90: 0x000000043015390e WebKit`IPC::Connection::enqueueIncomingMessage(this=0x00006020000451b8)::$_7::operator()() at Connection.cpp:985:28
frame #91: 0x00000004301537ee WebKit`WTF::Detail::CallableWrapper<IPC::Connection::enqueueIncomingMessage(std::__1::unique_ptr<IPC::Decoder, std::__1::default_delete<IPC::Decoder> >)::$_7, void>::call(this=0x00006020000451b0) at Function.h:52:39
frame #92: 0x00000004738291a5 JavaScriptCore`WTF::Function<void ()>::operator(this=0x00007ffee3c29520)() const at Function.h:84:35
frame #93: 0x0000000473968315 JavaScriptCore`WTF::RunLoop::performWork(this=0x0000607000005ce0) at RunLoop.cpp:119:9
frame #94: 0x0000000473969782 JavaScriptCore`WTF::RunLoop::performWork(context=0x0000607000005ce0) at RunLoopCF.cpp:38:37
frame #95: 0x00007fff366f8f12 CoreFoundation`__CFRUNLOOP_IS_CALLING_OUT_TO_A_SOURCE0_PERFORM_FUNCTION__ + 17
frame #96: 0x00007fff366f8eb1 CoreFoundation`__CFRunLoopDoSource0 + 103
frame #97: 0x00007fff366f8ccb CoreFoundation`__CFRunLoopDoSources0 + 209
frame #98: 0x00007fff366f79fa CoreFoundation`__CFRunLoopRun + 927
frame #99: 0x00007fff366f6ffe CoreFoundation`CFRunLoopRunSpecific + 462
frame #100: 0x00007fff38d8b2a8 Foundation`-[NSRunLoop(NSRunLoop) runMode:beforeDate:] + 212
frame #101: 0x00007fff38e3dd2f Foundation`-[NSRunLoop(NSRunLoop) run] + 76
frame #102: 0x00007fff7071c51a libxpc.dylib`_xpc_objc_main.cold.4 + 49
frame #103: 0x00007fff7071c460 libxpc.dylib`_xpc_objc_main + 559
frame #104: 0x00007fff7071bf93 libxpc.dylib`xpc_main + 377
frame #105: 0x00000004313e11a5 WebKit`WebKit::XPCServiceMain(argc=1, argv=0x00007ffee3c2aab8) at XPCServiceMain.mm:172:5
frame #106: 0x0000000434271a4b WebKit`WKXPCServiceMain(argc=1, argv=0x00007ffee3c2aab8) at WKMain.mm:33:12
frame #107: 0x000000010bfd5e22 com.apple.WebKit.WebContent.Development`main(argc=1, argv=0x00007ffee3c2aab8) at AuxiliaryProcessMain.cpp:30:12
frame #108: 0x00007fff704cecc9 libdyld.dylib`start + 1
frame #109: 0x00007fff704cecc9 libdyld.dylib`start + 1
Created attachment 398035 [details]
Patch
Created attachment 398038 [details]
Patch
Two changes were made in this revision: 1. Check null frame() in FrameSelection::setSelectionWithoutUpdatingAppearance after setFocusedElementIfNeeded() is called and return early to avoid crashes cause by reentrancy that detaches document. 2. Check null frame() before calling frame::selectionChangeCallbacksDisabled (iOS platform only). (In reply to Jack from comment #66) > Created attachment 398038 [details] > Patch Many thanks to Alan for spending extra time to further simplify the test case.
Also found is that testRunner.waitUntilDone() is required for printing the passing message because this case is timing sensitive.
New test case:
<div style="width: 1px; height: 1px;"></div><div contentEditable=true><object data="?" onload=objectOnLoad()></object></div><span>text</span>
<script>
if (window.testRunner) {
testRunner.dumpAsText();
testRunner.waitUntilDone();
}
document.getSelection().empty();
document.execCommand("selectAll", false);
function objectOnLoad() {
document.execCommand("insertUnorderedList", false);
document.execCommand("italic", false);
document.body.innerHTML = "<p> Tests inserting list followed by an edit command. The test passes if WebKit doesn't crash or hit an assertion.</p>";
testRunner.notifyDone();
}
</script>
Comment on attachment 398038 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398038&action=review > Source/WebCore/ChangeLog:50 > - (WebCore::Document::prepareForDestruction): > + (WebCore::Document::willBeRemovedFromFrame): > (WebCore::m_undoManager): Deleted. > + (WebCore::Document::prepareForDestruction): Deleted. These seems like accidental changes to another ChangeLog. > Source/WebCore/editing/FrameSelection.cpp:159 > + , m_focused(document && document->frame() && document->frame()->page() && document->page()->focusController().focusedFrame() == document->frame()) document->frame()->page() here can be document->page() (In reply to Geoffrey Garen from comment #69) > Comment on attachment 398038 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=398038&action=review > > > Source/WebCore/ChangeLog:50 > > - (WebCore::Document::prepareForDestruction): > > + (WebCore::Document::willBeRemovedFromFrame): > > (WebCore::m_undoManager): Deleted. > > + (WebCore::Document::prepareForDestruction): Deleted. > > These seems like accidental changes to another ChangeLog. Sorry for the confusion. The previous change log was stale because I forgot to add changed function in new revisions, so am trying to amend it here. > > Source/WebCore/editing/FrameSelection.cpp:159 > > + , m_focused(document && document->frame() && document->frame()->page() && document->page()->focusController().focusedFrame() == document->frame()) > > document->frame()->page() here can be document->page() Got it! Thanks. Created attachment 398072 [details]
Patch
Comment on attachment 398072 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=398072&action=review Looks almost ready to land. > Source/WebCore/ChangeLog:60 > - (WebCore::Document::prepareForDestruction): > + (WebCore::Document::willBeRemovedFromFrame): > (WebCore::m_undoManager): Deleted. > + (WebCore::Document::prepareForDestruction): Deleted. > * dom/Document.h: > (WebCore::Document::editor): > (WebCore::Document::editor const): > (WebCore::Document::selection): > (WebCore::Document::selection const): > - * dom/PositionIterator.cpp: > - (WebCore::PositionIterator::isCandidate const): > * editing/AlternativeTextController.cpp: > (WebCore::AlternativeTextController::AlternativeTextController): > (WebCore::AlternativeTextController::stopPendingCorrection): I think you still need to revert this section, which edits a previous ChangeLog entry. > Source/WebCore/ChangeLog:193 > (WebCore::Editor::findString): > (WebCore::Editor::countMatchesForText): > (WebCore::Editor::respondToChangedSelection): > - (WebCore::Editor::shouldDetectTelephoneNumbers): > + (WebCore::Editor::shouldDetectTelephoneNumbers const): > (WebCore::Editor::scanSelectionForTelephoneNumbers): > (WebCore::Editor::editorUIUpdateTimerFired): > (WebCore::Editor::selectionStartHasMarkerFor const): > - (WebCore::candidateRangeForSelection): > (WebCore::Editor::stringForCandidateRequest const): > (WebCore::Editor::contextRangeForCandidateRequest const): > (WebCore::Editor::fontAttributesAtSelectionStart const): Ditto > Source/WebCore/ChangeLog:229 > (WebCore::FrameSelection::modifyMovingRight): > (WebCore::FrameSelection::modifyMovingLeft): > (WebCore::FrameSelection::modify): > - (WebCore::FrameSelection::prepareForDestruction): > + (WebCore::FrameSelection::willBeRemovedFromFrame): > (WebCore::FrameSelection::absoluteCaretBounds): > (WebCore::FrameSelection::recomputeCaretRect): > (WebCore::FrameSelection::contains const): Ditto > Source/WebCore/ChangeLog:252 > (WebCore::FrameSelection::updateAppearanceAfterLayoutOrStyleChange): > (WebCore::FrameSelection::selectRangeOnElement): > (WebCore::FrameSelection::setCaretBlinks): > + (WebCore::FrameSelection::prepareForDestruction): Deleted. > * editing/FrameSelection.h: > * editing/InsertIntoTextNodeCommand.cpp: > (WebCore::InsertIntoTextNodeCommand::doApply): Ditto > Source/WebCore/ChangeLog:300 > * editing/TypingCommand.h: > * editing/cocoa/EditorCocoa.mm: > (WebCore::Editor::selectionInHTMLFormat): > + (WebCore::selectionAsAttributedString): > (WebCore::Editor::writeSelectionToPasteboard): > (WebCore::Editor::writeSelection): > (WebCore::Editor::selectionInWebArchiveFormat): Ditto > Source/WebCore/ChangeLog:330 > * editing/win/EditorWin.cpp: > (WebCore::Editor::pasteWithPasteboard): > (WebCore::Editor::webContentFromPasteboard): > + * history/CachedFrame.cpp: > + (WebCore::CachedFrame::destroy): > * loader/FrameLoader.cpp: > (WebCore::FrameLoader::willTransitionToCommitted): > (WebCore::FrameLoader::closeURL): Ditto > Source/WebCore/ChangeLog:339 > (WebCore::FrameLoader::clear): > * page/Frame.cpp: > (WebCore::Frame::Frame): > + (WebCore::Frame::setView): > + (WebCore::Frame::setDocument): > (WebCore::Frame::requestDOMPasteAccess): > (WebCore::Frame::setPageAndTextZoomFactors): > * page/Frame.h: Ditto Created attachment 398222 [details]
Patch
Remove the amendment in change log. (In reply to Jack from comment #73) > Created attachment 398222 [details] > Patch Comment on attachment 398222 [details]
Patch
r=me
Committed r261018: <https://trac.webkit.org/changeset/261018> All reviewed patches have been landed. Closing bug and clearing flags on attachment 398222 [details]. Change log is amended manually: git-svn-id: https://svn.webkit.org/repository/webkit/trunk@261032 268f45cc-cd09-0410-ab3c-d52691b4dbfc |