Web Automation: timeout underneath Automation.evaluateJavaScriptFunction in Selenium test frame_switching_tests.py::testShouldNotBeAbleToDoAnythingTheFrameIsDeletedFromUnderUs[Safari]

• Component: WebDriver
• Status: RESOLVED FIXED

[BJ Burg]

.

[BJ Burg]

<rdar://problem/60561009>

[BJ Burg]

Created attachment 395756 [details]
Patch

[Devin Rousso]

Comment on attachment 395756 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395756&action=review

r=me, the overall idea seems solid, but I do have lots of comments 😅

> Source/WebKit/ChangeLog:35
> +        (WebKit::WebAutomationSessionProxy::ensureObserverForFrame): For non-main frames,

NIT: for multi-line explanations, I prefer to start it on the next line, as that's where my eye naturally thinks the explanation starts 😅

> Source/WebKit/WebKit.xcodeproj/project.pbxproj:10822
> +				990D39F5243BE64800199388 /* WebAutomationDOMWindowObserver.h in Headers */,

This should be sorted.

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:27
> +#include "WebAutomationDOMWindowObserver.h"

Please make sure all necessary includes are in this file, such that if unified sources get shuffled around this file doesn't cause build breakages due to previously implicit includes.

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:36
> +    ASSERT(this->frame());

Is the `this` necessary?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:37
> +    if (m_window)

When would we ever want to create a `WebAutomationDOMWindowObserver` with an invalid `DOMWindow`?  Why not pass a `DOMWindow&` instead?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:47
> +Frame* WebAutomationDOMWindowObserver::frame() const

It seems like all this is used for is inside `ASSERT`.  Perhaps we could remove this and just `ASSERT(m_window && m_window->frame())` in those places instead?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:62
> +    if (m_window)

Is this really necessary?  This function is called by `DOMWindow`, so we should be guaranteed to have a `m_window`.

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:73
> +    if (!m_wasDetached) {

Given my comment on +95, I don't think it's possible for this to be called if `m_wasDetached`.  I'm not sure we'd need the member variable in that case either.

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:75
> +        Frame* frame = this->frame();
> +        ASSERT_UNUSED(frame, frame);

Why not just `ASSERT(frame());`?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:80
> +    if (m_window)

Ditto (+62)

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:93
> +    Frame* frame = this->frame();
> +    ASSERT_UNUSED(frame, frame);

Ditto (+74)

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:94
> +    m_callback(*this);

Shouldn't we also `m_window->unregisterObserver(*this)` here too, given that `m_callback(*this)` removes this object from `m_observers`, therefore causing it to be destroyed, meaning that no further callbacks should be run?  Or is this because it's possible/allowed/valid for the frame to be re-attached and continue to be used?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.cpp:96
> +    m_wasDetached = true;

NIT: should this be set earlier to prevent reentrancy (using the `ASSERT(!m_wasDetached)`) via `m_callback`?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.h:28
> +#import <WebCore/DOMWindow.h>

Please add
```
    #include <wtf/Forward.h>
```

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.h:31
> +class Element;

This doesn't seem needed.

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.h:47
> +    // All of these observer callbacks are interpreted as a signal that a frame has been detached and
> +    // can no longer accept new commands nor finish pending commands (eg, evaluating JavaScript).

Wouldn't the same apply to `suspendForBackForwardCache`, or are frames that have been navigated away from still considered valid?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.h:53
> +    WebCore::DOMWrapperWorld& world() const { return m_world; }

We should forward declare or include `WebCore::DOMWrapperWorld` too.

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.h:59
> +    Ref<WebCore::DOMWrapperWorld> m_world;

Why is this needed?  It doesn't seem like it's used anywhere.  Is it just to keep it alive?

> Source/WebKit/WebProcess/Automation/WebAutomationDOMWindowObserver.h:60
> +    RefPtr<WebCore::Frame> m_disconnectedFrame;

When is this set?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:68
> +using namespace WebCore;

Shouldn't this be inside the `namespace WebKit {`?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:310
> +    if (!frame.coreFrame()->window() || !frame.coreFrame()->window()->frame())

Shouldn't we also check that `frame.coreFrame()` exists?

If `frame.coreFrame()` exists, then wouldn't it be equal to `frame.coreFrame()->window()->frame()`, or can they be different objects?

Als, since you access `frame.coreFrame()->window()` three different times, perhaps it's worth pulling out into a variable?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:316
> +    FrameIdentifier frameID = frame.frameID();

`auto`?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:317
> +    m_frameObservers.set(frameID, WebAutomationDOMWindowObserver::create(frame.coreFrame()->window(), WebCore::mainThreadNormalWorld(), [this, frameID] (WebAutomationDOMWindowObserver&) {

We should have an include for `DOMWrapperWorld`.

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:318
> +        this->willDestroyGlobalObjectForFrame(frameID);

NIT: is the `this->` needed?

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:350
> +    if (!frame || !frame->coreFrame()->window() || !frame->coreFrame()->window()->frame()) {

Ditto (+310)

> Source/WebKit/WebProcess/Automation/WebAutomationSessionProxy.cpp:-738
> -        String invalidNodeIdentifierrrorType = Inspector::Protocol::AutomationHelpers::getEnumConstantValue(Inspector::Protocol::Automation::ErrorMessage::InvalidNodeIdentifier);

LOL 🤣

Part of me want's to keep this for the lols :P

[BJ Burg]

Created attachment 397463 [details]
Patch

[EWS]

Committed r260653: <https://trac.webkit.org/changeset/260653>

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