220478 2021-01-08 13:01:26 -0800 Relax assertion in Element::dispatchFocusOutEvent() for non-web process case 2021-01-11 14:57:13 -0800 1 1 1 Unclassified WebKit DOM WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 julian_a_gonzalez webkit-unassigned cdumez cmarcelo darin esprehn+autocc ews-watchlist kangil.han rniwa oldest_to_newest 1718409 0 julian_a_gonzalez 2021-01-08 13:01:26 -0800 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> 1718423 1 417298 julian_a_gonzalez 2021-01-08 13:20:28 -0800 Created attachment 417298 Patch 1718433 2 417298 darin 2021-01-08 13:57:49 -0800 Comment on attachment 417298 Patch I don’t get it. this turns off the assertion entirely for legacy WebKit. Why is that correct? 1718437 3 julian_a_gonzalez 2021-01-08 14:12:48 -0800 (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." 1718450 4 rniwa 2021-01-08 14:40:17 -0800 (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 1718456 5 darin 2021-01-08 14:54:11 -0800 Yes, the fix has to be about the timing of calling *out* of legacy WebKit to clients. Not about what the client does. 1718495 6 rniwa 2021-01-08 16:28:43 -0800 (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. 1718511 7 darin 2021-01-08 17:09:13 -0800 There are lots of other assertions we’ll have to remove. This is only the first of *many*. 1718515 8 rniwa 2021-01-08 17:19:47 -0800 (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. 1718826 9 ews-feeder 2021-01-11 14:57:10 -0800 Committed r271382: <https://trac.webkit.org/changeset/271382> All reviewed patches have been landed. Closing bug and clearing flags on attachment 417298. 417298 2021-01-08 13:20:28 -0800 2021-01-11 14:57:11 -0800 Patch bug-220478-20210108132027.patch text/plain 4719 julian_a_gonzalez U3VidmVyc2lvbiBSZXZpc2lvbjogMjcxMjU4CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggODA3ZmUyOTg1YTk2YjBl ODVlNjNlNTQwN2I0ZjdiMjU1NjZiMzRmZS4uNjAyYTA0MWI4OGQ5YTM2NmNhNzgzMjY0MzZjNjE5 ZjMyODgzY2Y3NSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIyIEBACisyMDIxLTAxLTA4ICBKdWxp YW4gR29uemFsZXogIDxqdWxpYW5fYV9nb256YWxlekBhcHBsZS5jb20+CisKKyAgICAgICAgUmVs YXggYXNzZXJ0aW9uIGluIEVsZW1lbnQ6OmRpc3BhdGNoRm9jdXNPdXRFdmVudCgpIGZvciBub24t d2ViIHByb2Nlc3MgY2FzZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1 Zy5jZ2k/aWQ9MjIwNDc4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgUmVsYXggYW4gYXNzZXJ0aW9uIGluIEVsZW1lbnQ6OmRpc3BhdGNoRm9jdXNPdXRF dmVudCgpCisgICAgICAgIChhbmQgRWxlbWVudDo6ZGlzcGF0Y2hGb2N1c0luRXZlbnQoKSkgdGhh dCBjYW4gbWlzdGFrZW5seQorICAgICAgICBmaXJlIGluIER1bXBSZW5kZXJUcmVlLiBBbHNvLCB1 c2UKKyAgICAgICAgUkVMRUFTRV9BU1NFUlRfV0lUSF9TRUNVUklUWV9JTVBMSUNBVElPTigpIGlu c3RlYWQgb2YKKyAgICAgICAgQVNTRVJUX1dJVEhfU0VDVVJJVFlfSU1QTElDQVRJT04oKS4KKwor ICAgICAgICBUZXN0OiBlZGl0aW5nL2luc2VydGluZy9jYXJldC1zdXJyb3VuZC5odG1sCisKKyAg ICAgICAgKiBkb20vRWxlbWVudC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpFbGVtZW50OjpkaXNw YXRjaEZvY3VzSW5FdmVudCk6CisgICAgICAgIChXZWJDb3JlOjpFbGVtZW50OjpkaXNwYXRjaEZv Y3VzT3V0RXZlbnQpOgorCiAyMDIxLTAxLTA3ICBGdWppaSBIaXJvbm9yaSAgPEhpcm9ub3JpLkZ1 amlpQHNvbnkuY29tPgogCiAgICAgICAgIFtXaW5DYWlyb10gV2ViR0wgaXNuJ3Qgc2hvd24gaW4g bm9uLUFDIG1vZGUKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcCBi L1NvdXJjZS9XZWJDb3JlL2RvbS9FbGVtZW50LmNwcAppbmRleCA5Y2FhM2M4ZTZlZjdmNmVkOWYy ZDYzM2Q3NDI3N2FjYzEzOWEyM2I4Li45ZGM0OTkwYmU3OWNlMTMyZDdiNmY3N2UzODI4YjdmMDZl ZWUyZjM3IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9kb20vRWxlbWVudC5jcHAKKysrIGIv U291cmNlL1dlYkNvcmUvZG9tL0VsZW1lbnQuY3BwCkBAIC0zMTE3LDE0ICszMTE3LDE0IEBAIHZv aWQgRWxlbWVudDo6Ymx1cigpCiAKIHZvaWQgRWxlbWVudDo6ZGlzcGF0Y2hGb2N1c0luRXZlbnQo Y29uc3QgQXRvbVN0cmluZyYgZXZlbnRUeXBlLCBSZWZQdHI8RWxlbWVudD4mJiBvbGRGb2N1c2Vk RWxlbWVudCkKIHsKLSAgICBBU1NFUlRfV0lUSF9TRUNVUklUWV9JTVBMSUNBVElPTihTY3JpcHRE aXNhbGxvd2VkU2NvcGU6OkluTWFpblRocmVhZDo6aXNTY3JpcHRBbGxvd2VkKCkpOworICAgIFJF TEVBU0VfQVNTRVJUX1dJVEhfU0VDVVJJVFlfSU1QTElDQVRJT04oU2NyaXB0RGlzYWxsb3dlZFNj b3BlOjpJbk1haW5UaHJlYWQ6OmlzU2NyaXB0QWxsb3dlZCgpIHx8ICFpc0luV2ViUHJvY2Vzcygp KTsKICAgICBBU1NFUlQoZXZlbnRUeXBlID09IGV2ZW50TmFtZXMoKS5mb2N1c2luRXZlbnQgfHwg ZXZlbnRUeXBlID09IGV2ZW50TmFtZXMoKS5ET01Gb2N1c0luRXZlbnQpOwogICAgIGRpc3BhdGNo U2NvcGVkRXZlbnQoRm9jdXNFdmVudDo6Y3JlYXRlKGV2ZW50VHlwZSwgRXZlbnQ6OkNhbkJ1YmJs ZTo6WWVzLCBFdmVudDo6SXNDYW5jZWxhYmxlOjpObywgZG9jdW1lbnQoKS53aW5kb3dQcm94eSgp LCAwLCBXVEZNb3ZlKG9sZEZvY3VzZWRFbGVtZW50KSkpOwogfQogCiB2b2lkIEVsZW1lbnQ6OmRp c3BhdGNoRm9jdXNPdXRFdmVudChjb25zdCBBdG9tU3RyaW5nJiBldmVudFR5cGUsIFJlZlB0cjxF bGVtZW50PiYmIG5ld0ZvY3VzZWRFbGVtZW50KQogewotICAgIEFTU0VSVF9XSVRIX1NFQ1VSSVRZ X0lNUExJQ0FUSU9OKFNjcmlwdERpc2FsbG93ZWRTY29wZTo6SW5NYWluVGhyZWFkOjppc1Njcmlw dEFsbG93ZWQoKSk7CisgICAgUkVMRUFTRV9BU1NFUlRfV0lUSF9TRUNVUklUWV9JTVBMSUNBVElP TihTY3JpcHREaXNhbGxvd2VkU2NvcGU6OkluTWFpblRocmVhZDo6aXNTY3JpcHRBbGxvd2VkKCkg fHwgIWlzSW5XZWJQcm9jZXNzKCkpOwogICAgIEFTU0VSVChldmVudFR5cGUgPT0gZXZlbnROYW1l cygpLmZvY3Vzb3V0RXZlbnQgfHwgZXZlbnRUeXBlID09IGV2ZW50TmFtZXMoKS5ET01Gb2N1c091 dEV2ZW50KTsKICAgICBkaXNwYXRjaFNjb3BlZEV2ZW50KEZvY3VzRXZlbnQ6OmNyZWF0ZShldmVu dFR5cGUsIEV2ZW50OjpDYW5CdWJibGU6OlllcywgRXZlbnQ6OklzQ2FuY2VsYWJsZTo6Tm8sIGRv Y3VtZW50KCkud2luZG93UHJveHkoKSwgMCwgV1RGTW92ZShuZXdGb2N1c2VkRWxlbWVudCkpKTsK IH0KZGlmZiAtLWdpdCBhL0xheW91dFRlc3RzL0NoYW5nZUxvZyBiL0xheW91dFRlc3RzL0NoYW5n ZUxvZwppbmRleCAxNmI1NWMxZTgyMjgzNzIwMjcxMGNhMDdlNDUzYjc4ZjJlYzY3OThjLi5hNjcz MDkyOWE1YzJhNzc0MTQ0NWNmM2Q3MTQxYTYwYWE3MWQ2ODFhIDEwMDY0NAotLS0gYS9MYXlvdXRU ZXN0cy9DaGFuZ2VMb2cKKysrIGIvTGF5b3V0VGVzdHMvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMTYg QEAKKzIwMjEtMDEtMDggIEp1bGlhbiBHb256YWxleiAgPGp1bGlhbl9hX2dvbnphbGV6QGFwcGxl LmNvbT4KKworICAgICAgICBSZWxheCBhc3NlcnRpb24gaW4gRWxlbWVudDo6ZGlzcGF0Y2hGb2N1 c091dEV2ZW50KCkgZm9yIG5vbi13ZWIgcHJvY2VzcyBjYXNlCisgICAgICAgIGh0dHBzOi8vYnVn cy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0yMjA0NzgKKworICAgICAgICBSZXZpZXdlZCBi eSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBBZGQgYSB0ZXN0IHRoYXQgdmVyaWZpZXMgd2Ug ZG8gbm90IGNyYXNoCisgICAgICAgIGR1ZSB0byB0aGUgYXNzZXJ0aW9uIGZhaWx1cmUuCisKKyAg ICAgICAgKiBlZGl0aW5nL2luc2VydGluZy9jYXJldC1zdXJyb3VuZC1leHBlY3RlZC50eHQ6IEFk ZGVkLgorICAgICAgICAqIGVkaXRpbmcvaW5zZXJ0aW5nL2NhcmV0LXN1cnJvdW5kLmh0bWw6IEFk ZGVkLgorCiAyMDIxLTAxLTA3ICBBbGV4IENocmlzdGVuc2VuICA8YWNocmlzdGVuc2VuQHdlYmtp dC5vcmc+CiAKICAgICAgICAgTnVsbCBjaGVjayBnbG9iYWwgb2JqZWN0IGluIEJsb2I6OnN0cmVh bQpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvZWRpdGluZy9pbnNlcnRpbmcvY2FyZXQtc3Vycm91 bmQtZXhwZWN0ZWQudHh0IGIvTGF5b3V0VGVzdHMvZWRpdGluZy9pbnNlcnRpbmcvY2FyZXQtc3Vy cm91bmQtZXhwZWN0ZWQudHh0Cm5ldyBmaWxlIG1vZGUgMTAwNjQ0CmluZGV4IDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAuLjJlYThlMjc5OTQ3ZDBjMGQxMjY3MDYxM2Ux NzhiMDEwYTFjOTg4YzEKLS0tIC9kZXYvbnVsbAorKysgYi9MYXlvdXRUZXN0cy9lZGl0aW5nL2lu c2VydGluZy9jYXJldC1zdXJyb3VuZC1leHBlY3RlZC50eHQKQEAgLTAsMCArMSwyIEBACitUaGlz IHRlc3RzIHRoYXQgd2UgZG8gbm90IGhpdCBhbiBhc3NlcnRpb24gd2hpbGUgdW5mb2N1c2luZy4K KwpkaWZmIC0tZ2l0IGEvTGF5b3V0VGVzdHMvZWRpdGluZy9pbnNlcnRpbmcvY2FyZXQtc3Vycm91 bmQuaHRtbCBiL0xheW91dFRlc3RzL2VkaXRpbmcvaW5zZXJ0aW5nL2NhcmV0LXN1cnJvdW5kLmh0 bWwKbmV3IGZpbGUgbW9kZSAxMDA2NDQKaW5kZXggMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAwMDAw MDAwMDAwMDAwMDAwMC4uMzVmYTU3OTljMjk3Y2QzMjNiMWMxZGJkMjM1MDc5NDgyZDY1NTU0ZQot LS0gL2Rldi9udWxsCisrKyBiL0xheW91dFRlc3RzL2VkaXRpbmcvaW5zZXJ0aW5nL2NhcmV0LXN1 cnJvdW5kLmh0bWwKQEAgLTAsMCArMSwyNSBAQAorPGh0bWw+Cis8aGVhZD4KKzxzY3JpcHQ+Citm dW5jdGlvbiB0ZXN0b25sb2FkKCkgeworICAgIGlmICh3aW5kb3cudGVzdFJ1bm5lcikKKyAgICAg ICAgdGVzdFJ1bm5lci5kdW1wQXNUZXh0KCk7CisKKyAgICBjYXJldHJhbmdlID0gZG9jdW1lbnQu Y2FyZXRSYW5nZUZyb21Qb2ludCgpOworICAgIGJ1dHRvbi5zdHlsZS5zZXRQcm9wZXJ0eSgiY29u dGVudCIsICJ1cmwoKSIpOworICAgIGJ1dHRvbi5hdXRvZm9jdXMgPSB0cnVlOworICAgIGNhcmV0 cmFuZ2Uuc3Vycm91bmRDb250ZW50cyh0ZXh0YXJlYSk7Cit9CitmdW5jdGlvbiB0b2dnbGVoYW5k bGVyKCkgeworICAgIHRleHRhcmVhLnNlbGVjdGlvblN0YXJ0ID0gMDsKK30KKzwvc2NyaXB0Pgor PC9oZWFkPgorPGJvZHkgb25sb2FkPXRlc3RvbmxvYWQoKT5UaGlzIHRlc3RzIHRoYXQgd2UgZG8g bm90IGhpdCBhbiBhc3NlcnRpb24gd2hpbGUgdW5mb2N1c2luZy4KKzx0ZXh0YXJlYSBpZD0idGV4 dGFyZWEiPlBBU1M8L3RleHRhcmVhPgorPGRldGFpbHMgb250b2dnbGU9InRvZ2dsZWhhbmRsZXIo KSIgb3Blbj0idHJ1ZSI+Cis8aW1nIHNyYz0iZW1wdHkiPjwvaW1nPgorPGJ1dHRvbiBpZD0iYnV0 dG9uIj48L2J1dHRvbj4KKzwvZGV0YWlscz4KKzwvYm9keT4KKzwvaHRtbD4K