This code used to encode the implicit main frame as std::nullopt and a specified frame as a non-null Optional<uint64_t>. Now FrameIdentifier is expected to always exist, so we should replace an unspecified frame with the main frame identifier earlier and remove Optional<FrameIdentifier> in favor of FrameIdentifier where it makes sense.
Need to be careful to avoid problems like https://bugs.webkit.org/show_bug.cgi?id=159777
Created attachment 376506 [details] [WIP]
Comment on attachment 376506 [details] [WIP] Trying an approach where we always have a FrameIdentifier. I need to rebaseline test results prior to the regressing change to get a good idea if this fix is insufficient or introduces new issues.
Created attachment 376780 [details] Proposed Fix
<rdar://problem/54516988>
After building with these changes, safaridriver has no changes in test results from the recent rebaselining.
I'm not sure what this wincairo failure is about: C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/Ref.h(60): error C2039: 'deref': is not a member of 'WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e>' C:\Buildbot\WinCairo-EWS\build\Source\WebKit\UIProcess/Automation/WebAutomationSession.cpp(1094): note: see declaration of 'WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e>' C:\Buildbot\WinCairo-EWS\build\WebKitBuild\Release\WTF\Headers\wtf/Ref.h(54): note: while compiling class template member function 'WTF::Ref<WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e>,WTF::DumbPtrTraits<T>>::~Ref(void)' with [ T=WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e> ] C:\Buildbot\WinCairo-EWS\build\Source\WebKit\UIProcess/Automation/WebAutomationSession.cpp(1091): note: see reference to function template instantiation 'WTF::Ref<WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e>,WTF::DumbPtrTraits<T>>::~Ref(void)' being compiled with [ T=WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e> ] C:\Buildbot\WinCairo-EWS\build\Source\WebKit\UIProcess/Automation/WebAutomationSession.cpp(1084): note: see reference to class template instantiation 'WTF::Ref<WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e>,WTF::DumbPtrTraits<T>>' being compiled with [ T=WebKit::WebAutomationSession::resolveParentFrameHandle::<lambda_c796265cdb2c19385925e6b6b1a6903e> ] C:\Buildbot\WinCairo-EWS\build\Source\WebKit\UIProcess\Automation\WebAutomationSession.h(87): note: see reference to class template instantiation 'WTF::Optional<WTF::String>' being compiled
The entire function referenced: void WebAutomationSession::resolveParentFrameHandle(const String& browsingContextHandle, const String& frameHandle, Ref<ResolveParentFrameHandleCallback>&& callback) { WebPageProxy* page = webPageProxyForHandle(browsingContextHandle); if (!page) ASYNC_FAIL_WITH_PREDEFINED_ERROR(WindowNotFound); whenMainFrameAvailable(*page, [this, protectedThis = makeRef(*this), callback = WTFMove(callback), page = makeRef(*page), frameHandle](bool available) mutable { if (!available) ASYNC_FAIL_WITH_PREDEFINED_ERROR(WindowNotFound); auto frameID = webFrameIDForHandle(page.get(), frameHandle); if (!frameID) ASYNC_FAIL_WITH_PREDEFINED_ERROR(FrameNotFound); WTF::CompletionHandler<void(Optional<String>, Optional<FrameIdentifier>)> completionHandler = [this, protectedThis = makeRef(*this), callback = callback.copyRef()](Optional<String> errorType, Optional<FrameIdentifier> frameID) mutable { if (errorType) { callback->sendFailure(STRING_FOR_PREDEFINED_ERROR_MESSAGE(*errorType)); return; } callback->sendSuccess(handleForWebFrameID(frameID)); }; page->process().sendWithAsyncReply(Messages::WebAutomationSessionProxy::ResolveParentFrame(page->pageID(), *frameID), WTFMove(completionHandler)); }); }
This affects also the GTK/WPE port. I tested this patch and it fixes the crashes for GTK.
Comment on attachment 376780 [details] Proposed Fix This is a lot of changes. Could you, instead, just use an Optional<FrameIdentifier> and use the main frame when it's nullopt?
Comment on attachment 376780 [details] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=376780&action=review > Source/WebKit/ChangeLog:14 > + the special value of '0' for frameID was used as a placeholder value for the main frame's frameID. My previous patch used Optional<FrameIdentifier> where nullopt was the special value for MainFrame (instead of 0 previously). I likely missed one spot since we're asserting but why change the whole approach? > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:210 > +Optional<FrameIdentifier> WebAutomationSession::webFrameIDForHandle(const WebPageProxy& page, Optional<String> frameHandle) const Optional<String>& ? > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:228 > + Optional<String> frameHandle; Seems like this could use your new convertToOptional() and avoid some code duplication. > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:475 > +void WebAutomationSession::whenMainFrameAvailable(const WebPageProxy& page, WTF::CompletionHandler<void(bool)>&& callback) No need for WTF:: > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:482 > + m_callbacksWaitingForMainFramePerPage.add(page.pageID(), WTFMove(callback)); Should we assert that page.pageID() is not already in the map? Otherwise, we'd drop this callback on the floor. > Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1003 > + m_evaluateJavaScriptFunctionCallbacks.set(callbackID, WTFMove(callback)); Why set() and not add() (note that set() is slower)? Also should we assert() that callbackID is not already in the map. And since it cannot already be in the map, add() makes more sense.
(In reply to Simon Fraser (smfr) from comment #10) > Comment on attachment 376780 [details] > Proposed Fix > > This is a lot of changes. Could you, instead, just use an > Optional<FrameIdentifier> and use the main frame when it's nullopt? Yes, please. This is what my patch was attempting to do (although I likely messed up something if we're asserting).
I tried doing what you suggested, but it is not sufficient. There are other cases where a concrete frameID is expected before continuing with a command, namely, performInteractionSequence(). I could try changing SimulatedInputDispatcher to support Optional<FrameIdentifier>, I suppose. It may still yet be an easier fix. #0 0x000000010291a49e in ::WTFCrash() at /Users/bburg/repos/webkit/OpenSource/Source/WTF/wtf/Assertions.cpp:305 #1 0x0000000109934919 in WTF::Optional<WTF::ObjectIdentifier<WebCore::FrameIdentifierType> >::value() & at /Users/bburg/Library/Developer/Xcode/DerivedData/Safari-ffowujeoioebkpbndwdjtcebbgph/Build/Products/Debug/usr/local/include/wtf/Optional.h:544 #2 0x000000010a2b0958 in WebKit::WebAutomationSession::performInteractionSequence(WTF::String const&, WTF::String const*, WTF::JSONImpl::Array const&, WTF::JSONImpl::Array const&, WTF::Ref<Inspector::AutomationBackendDispatcherHandler::PerformInteractionSequenceCallback, WTF::DumbPtrTraits<Inspector::AutomationBackendDispatcherHandler::PerformInteractionSequenceCallback> >&&) at /Users/bburg/repos/webkit/OpenSource/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1925 #3 0x00000001095045aa in Inspector::AutomationBackendDispatcher::performInteractionSequence(long, WTF::RefPtr<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&) at /Users/bburg/Library/Developer/Xcode/DerivedData/Safari-ffowujeoioebkpbndwdjtcebbgph/Build/Products/Debug/DerivedSources/WebKit2/AutomationBackendDispatchers.cpp:553 #4 0x0000000109500ae7 in Inspector::AutomationBackendDispatcher::dispatch(long, WTF::String const&, WTF::Ref<WTF::JSONImpl::Object, WTF::DumbPtrTraits<WTF::JSONImpl::Object> >&&) at /Users/bburg/Library/Developer/Xcode/DerivedData/Safari-ffowujeoioebkpbndwdjtcebbgph/Build/Products/Debug/DerivedSources/WebKit2/AutomationBackendDispatchers.cpp:118 #5 0x00000001039ba785 in Inspector::BackendDispatcher::dispatch(WTF::String const&) at /Users/bburg/repos/webkit/OpenSource/Source/JavaScriptCore/inspector/InspectorBackendDispatcher.cpp:180 #6 0x000000010a29f1dc in WebKit::WebAutomationSession::dispatchMessageFromRemote(WTF::String const&) at /Users/bburg/repos/webkit/OpenSource/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:129 #7 0x0000000102ebc1bf in ::___ZN9Inspector24RemoteConnectionToTarget19sendMessageToTargetEP8NSString_block_invoke() at /Users/bburg/repos/webkit/OpenSource/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:233 #8 0x0000000102ec3f4d in WTF::BlockPtr<void ()>::operator()() const at /Users/bburg/Library/Developer/Xcode/DerivedData/Safari-ffowujeoioebkpbndwdjtcebbgph/Build/Products/Debug/usr/local/include/wtf/BlockPtr.h:184 #9 0x0000000102ec3c0a in Inspector::RemoteTargetHandleRunSourceGlobal(void*) at /Users/bburg/repos/webkit/OpenSource/Source/JavaScriptCore/inspector/remote/cocoa/RemoteConnectionToTargetCocoa.mm:62
Created attachment 377138 [details] Patch v2
Attachment 377138 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp:1465: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/Automation/WebAutomationSession.h:156: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebKit/UIProcess/Automation/SimulatedInputDispatcher.h:137: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 3 in 5 files If any of these errors are false positives, please file a bug against check-webkit-style.
Committed r249062: <https://trac.webkit.org/changeset/249062>