[webkit-reviews] review granted: [Bug 210162] Web Automation: timeout underneath Automation.evaluateJavaScriptFunction in Selenium test frame_switching_tests.py::testShouldNotBeAbleToDoAnythingTheFrameIsDeletedFromUnderUs[Safari] : [Attachment 395756] Patch

[bugzilla-daemon at webkit.org]

Devin Rousso <drousso at apple.com> has granted Brian Burg <bburg at apple.com>'s
request for review:
Bug 210162: Web Automation: timeout underneath
Automation.evaluateJavaScriptFunction in Selenium test
frame_switching_tests.py::testShouldNotBeAbleToDoAnythingTheFrameIsDeletedFromU
nderUs[Safari]
https://bugs.webkit.org/show_bug.cgi?id=210162

Attachment 395756: Patch

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




--- Comment #3 from Devin Rousso <drousso at apple.com> ---
Comment on attachment 395756
  --> https://bugs.webkit.org/attachment.cgi?id=395756
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::Protoco
l::Automation::ErrorMessage::InvalidNodeIdentifier);

LOL ��

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