Changeset 246456 in webkit

Timestamp:

Jun 14, 2019 8:20:42 PM (5 years ago)

Author:

Devin Rousso

Message:

waitForNavigationToComplete may be called before WebPageProxy knows it's loading
​https://bugs.webkit.org/show_bug.cgi?id=198741
<rdar://problem/31164316>

Reviewed by Joseph Pecoraro.

There's a potential race in WebAutomationSession::waitForNavigationToCompleteOnPage when
querying for the WebPageProxy's loading state (via PageLoadingState::isLoading), in that
a pending load may be committed _after_ the WebAutomationSession checks it's value. This
makes the automation session think that it isn't loading, so it will continue running
commands, which can lead to a JavaScript error ("Callback was not called before the unload
event") as any injected scripts will be cleared by the impending navigation, leaving the
script evaluation callbacks "dangling".

Expose more information from PageLoadState about whether it thinks there _may_ be a
navigation currently happening, which the WebAutomationSession can use to delay commands.

In the best case, no navigations are "missed".

In the worst case, the automation session will wait pageLoadTimeout before continuing.

• UIProcess/Automation/WebAutomationSession.cpp:

(WebKit::WebAutomationSession::waitForNavigationToCompleteOnPage):

• UIProcess/PageLoadState.h:
• UIProcess/PageLoadState.cpp:

(WebKit::PageLoadState::hasUncommittedLoad const): Added.

Location:

trunk/Source/WebKit

Files:

• ChangeLog (modified) (1 diff)
• UIProcess/Automation/WebAutomationSession.cpp (modified) (1 diff)
• UIProcess/PageLoadState.cpp (modified) (1 diff)
• UIProcess/PageLoadState.h (modified) (1 diff)

trunk/Source/WebKit/ChangeLog

@@ +1 @@
+2019-06-14  Devin Rousso  <drousso@apple.com>
+
+        waitForNavigationToComplete may be called before WebPageProxy knows it's loading
+        https://bugs.webkit.org/show_bug.cgi?id=198741
+        <rdar://problem/31164316>
+
+        Reviewed by Joseph Pecoraro.
+
+        There's a potential race in `WebAutomationSession::waitForNavigationToCompleteOnPage` when
+        querying for the `WebPageProxy`'s loading state (via `PageLoadingState::isLoading`), in that
+        a pending load may be committed _after_ the `WebAutomationSession` checks it's value. This
+        makes the automation session think that it isn't loading, so it will continue running
+        commands, which can lead to a JavaScript error ("Callback was not called before the unload
+        event") as any injected scripts will be cleared by the impending navigation, leaving the
+        script evaluation callbacks "dangling".
+
+        Expose more information from `PageLoadState` about whether it thinks there _may_ be a
+        navigation currently happening, which the `WebAutomationSession` can use to delay commands.
+
+        In the best case, no navigations are "missed".
+
+        In the worst case, the automation session will wait `pageLoadTimeout` before continuing.
+
+        * UIProcess/Automation/WebAutomationSession.cpp:
+        (WebKit::WebAutomationSession::waitForNavigationToCompleteOnPage):
+
+        * UIProcess/PageLoadState.h:
+        * UIProcess/PageLoadState.cpp:
+        (WebKit::PageLoadState::hasUncommittedLoad const): Added.
+
 2019-06-14  Youenn Fablet  <youenn@apple.com>
 

trunk/Source/WebKit/UIProcess/Automation/WebAutomationSession.cpp

@@ -485 +485 @@
 {
     ASSERT(!m_loadTimer.isActive());
-    if (loadStrategy == Inspector::Protocol::Automation::PageLoadStrategy::None || !page.pageLoadState().isLoading()) {
+    if (loadStrategy == Inspector::Protocol::Automation::PageLoadStrategy::None || (!page.pageLoadState().isLoading() && !page.pageLoadState().hasUncommittedLoad())) {
         callback->sendSuccess(JSON::Object::create());
         return;

trunk/Source/WebKit/UIProcess/PageLoadState.cpp

@@ -173 +173 @@
 }
 
+bool PageLoadState::hasUncommittedLoad() const
+{
+    return isLoading(m_uncommittedState);
+}
+
 String PageLoadState::activeURL(const Data& data)
 {

trunk/Source/WebKit/UIProcess/PageLoadState.h

@@ -126 +126 @@
     bool isFinished() const { return m_committedState.state == State::Finished; }
 
+    bool hasUncommittedLoad() const;
+
     const String& provisionalURL() const { return m_committedState.provisionalURL; }
     const String& url() const { return m_committedState.url; }