Bug 200793 – REGRESSION(r248713): WebDriver commands which target the implicit main frame now hit an ASSERT

• Component: WebDriver
• Status: RESOLVED FIXED

[Blaze Burg]

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.

[Blaze Burg]

Need to be careful to avoid problems like https://bugs.webkit.org/show_bug.cgi?id=159777

[Blaze Burg]

Created attachment 376506 [details]
[WIP]

[Blaze Burg]

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.

[Blaze Burg]

Created attachment 376780 [details]
Proposed Fix

[Radar WebKit Bug Importer]

<rdar://problem/54516988>

[Blaze Burg]

After building with these changes, safaridriver has no changes in test results from the recent rebaselining.

[Blaze Burg]

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

[Blaze Burg]

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));
    });
}

[Carlos Alberto Lopez Perez]

This affects also the GTK/WPE port. I tested this patch and it fixes the crashes for GTK.

[Simon Fraser (smfr)]

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?

[Chris Dumez]

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.

[Chris Dumez]

(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).

[Blaze Burg]

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

[Blaze Burg]

Created attachment 377138 [details]
Patch v2

[EWS Watchlist]

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.

[Blaze Burg]

Committed r249062: <https://trac.webkit.org/changeset/249062>