Bug 220478 – Relax assertion in Element::dispatchFocusOutEvent() for non-web process case

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Julian Gonzalez]

This assertion (along with the identical one in Element::dispatchFocusInEvent()) can be erroneously hit in DumpRenderTree.

frame #0: JavaScriptCore`WTFCrashWithSecurityImplication+9
frame #1: WebCore`WebCore::Element::dispatchFocusOutEvent(WTF::AtomString const&, WTF::RefPtr<WebCore::Element, WTF::RawPtrTraits<WebCore::Element>, WTF::DefaultRefDerefTraits<WebCore::Element> >&&)+730
frame #2: WebCore`WebCore::Document::setFocusedElement(WebCore::Element*, WebCore::FocusDirection, WebCore::Document::FocusRemovalEventsMode)+1279
frame #3: WebCore`WebCore::FocusController::setFocusedElement(WebCore::Element*, WebCore::Frame&, WebCore::FocusDirection)+1498
frame #4: WebCore`WebCore::Element::focus(WebCore::SelectionRestorationMode, WebCore::FocusDirection)+1315
frame #5: WebCore`WebCore::HTMLFormControlElement::didAttachRenderers()::$_1::operator()() const+79

<rdar://problem/72670556>

[Julian Gonzalez]

Created attachment 417298 [details]
Patch

[Darin Adler]

Comment on attachment 417298 [details]
Patch

I don’t get it. this turns off the assertion entirely for legacy WebKit. Why is that correct?

[Julian Gonzalez]

(In reply to Darin Adler from comment #2)
> Comment on attachment 417298 [details]
> Patch
> 
> I don’t get it. this turns off the assertion entirely for legacy WebKit. Why
> is that correct?

Ryosuke mentioned to me (and on the radar) that:

"We probably just need to disable the assertion here. There isn’t much we can do to mitigate this since application that embeds WebKit can do anything they like."

[Ryosuke Niwa]

(In reply to Darin Adler from comment #2)
> Comment on attachment 417298 [details]
> Patch
> 
> I don’t get it. this turns off the assertion entirely for legacy WebKit. Why
> is that correct?

In the following call stack, frame #22-23 callbacks into WebView delegates. There, client application can do anything.
So long as these delegates exist, we can't really guarantee the safety / consistency of WebCore states.

In WebKit2, we made editor state update async so this issue of updating the state doesn't really exist.
We could, in theory, make _updateFontPanel async but that doesn't resolve the issue that
WebEditorClient still issues WebViewDidChangeSelectionNotification which could trigger arbitrary code.

We've already had to disable similar kinds of assertions in WebKit1 elsewhere in ScriptController::canExecuteScripts and isSafeToUpdateStyleOrLayout in Document.cpp.

frame #0: JavaScriptCore`WTFCrashWithSecurityImplication+9
frame #1: WebCore`WebCore::Element::dispatchFocusOutEvent(WTF::AtomString const&, WTF::RefPtr<WebCore::Element, WTF::RawPtrTraits<WebCore::Element>, WTF::DefaultRefDerefTraits<WebCore::Element> >&&)+730
frame #2: WebCore`WebCore::Document::setFocusedElement(WebCore::Element*, WebCore::FocusDirection, WebCore::Document::FocusRemovalEventsMode)+1279
frame #3: WebCore`WebCore::FocusController::setFocusedElement(WebCore::Element*, WebCore::Frame&, WebCore::FocusDirection)+1498
frame #4: WebCore`WebCore::Element::focus(WebCore::SelectionRestorationMode, WebCore::FocusDirection)+1315
frame #5: WebCore`WebCore::HTMLFormControlElement::didAttachRenderers()::$_1::operator()() const+79
frame #6: WebCore`WTF::Detail::CallableWrapper<WebCore::HTMLFormControlElement::didAttachRenderers()::$_1, void>::call()+13
frame #7: WebCore`WTF::Function<void ()>::operator()() const+63
frame #8: WebCore`WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler()+115
frame #9: WebCore`WebCore::Style::PostResolutionCallbackDisabler::~PostResolutionCallbackDisabler()+9
frame #10: WebCore`WebCore::Document::resolveStyle(WebCore::Document::ResolveStyleType)+1998
frame #11: WebCore`WebCore::Document::updateStyleIfNeeded()+572
frame #12: WebCore`WebCore::Document::updateLayout()+404
frame #13: WebCore`WebCore::Document::updateLayoutIgnorePendingStylesheets(WebCore::Document::RunPostLayoutTasks)+147
frame #14: WebCore`WebCore::VisiblePosition::canonicalPosition(WebCore::Position const&)+283
frame #15: WebCore`WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity)+184
frame #16: WebCore`WebCore::VisiblePosition::VisiblePosition(WebCore::Position const&, WebCore::Affinity)+9
frame #17: WebCore`WebCore::VisibleSelection::visibleStart() const+58
frame #18: WebCore`WebCore::adjustedSelectionStartForStyleComputation(WebCore::VisibleSelection const&)+228
frame #19: WebCore`WebCore::Editor::styleForSelectionStart(WebCore::Frame*, WebCore::Node*&)+355
frame #20: WebCore`WebCore::Editor::fontForSelection(bool&) const+898
frame #21: WebKitLegacy`-[WebHTMLView(WebInternal) _updateFontPanel]+561
frame #22: WebKitLegacy`-[WebHTMLView(WebInternal) _selectionChanged]+95
frame #23: WebKitLegacy`WebEditorClient::respondToChangedSelection(WebCore::Frame*)+210
frame #24: WebCore`WebCore::Editor::respondToChangedSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>)+325
frame #25: WebCore`WebCore::FrameSelection::setSelectionWithoutUpdatingAppearance(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)+1745
frame #26: WebCore`WebCore::FrameSelection::setSelection(WebCore::VisibleSelection const&, WTF::OptionSet<WebCore::FrameSelection::SetSelectionOption>, WebCore::AXTextStateChangeIntent, WebCore::FrameSelection::CursorAlignOnScroll, WebCore::TextGranularity)+347
frame #27: WebCore`WebCore::FrameSelection::moveWithoutValidationTo(WebCore::Position const&, WebCore::Position const&, bool, bool, WebCore::SelectionRevealMode, WebCore::AXTextStateChangeIntent const&)+787
frame #28: WebCore`WebCore::HTMLTextFormControlElement::setSelectionRange(int, int, WebCore::TextFieldSelectionDirection, WebCore::SelectionRevealMode, WebCore::AXTextStateChangeIntent const&)+1984
frame #29: WebCore`WebCore::HTMLTextAreaElement::setValueCommon(WTF::String const&)+863
frame #30: WebCore`WebCore::HTMLTextAreaElement::setNonDirtyValue(WTF::String const&)+14
frame #31: WebCore`WebCore::HTMLTextAreaElement::childrenChanged(WebCore::ContainerNode::ChildChange const&)+418
frame #32: WebCore`WebCore::ContainerNode::removeChildren()+1283
frame #33: WebCore`WebCore::ContainerNode::replaceAllChildren(std::nullptr_t)+188

[Darin Adler]

Yes, the fix has to be about the timing of calling *out* of legacy WebKit to clients. Not about what the client does.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #5)
> Yes, the fix has to be about the timing of calling *out* of legacy WebKit to
> clients. Not about what the client does.

I don't think that would be practical given the number of sync delegate calls that happen when it's not safe to run arbitrary scripts.

[Darin Adler]

There are lots of other assertions we’ll have to remove. This is only the first of *many*.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> There are lots of other assertions we’ll have to remove. This is only the
> first of *many*.

That's probably true. But removing assertions won't introduce new app compatibility regressions whilst changing the timing of delegate callbacks will.

[EWS]

Committed r271382: <https://trac.webkit.org/changeset/271382>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 417298 [details].