Bug 198741 - waitForNavigationToComplete may be called before WebPageProxy knows it's loading
Summary: waitForNavigationToComplete may be called before WebPageProxy knows it's loading
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebDriver (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-10 21:58 PDT by Devin Rousso
Modified: 2019-06-14 20:20 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.24 KB, patch)
2019-06-10 22:05 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.26 KB, patch)
2019-06-13 20:42 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (4.31 KB, patch)
2019-06-14 19:37 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-06-10 21:58:38 PDT
If the timing of the communications to the `WebPageProxy` about the fact that it's actually `PageLoadingState::isLoading` is too "slow" or happens after `WebAutomationSession::waitForNavigationToCompleteOnPage` is called, the automation session 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".
Comment 1 Devin Rousso 2019-06-10 21:58:55 PDT
<rdar://problem/31164316>
Comment 2 Devin Rousso 2019-06-10 22:05:03 PDT
Created attachment 371820 [details]
Patch
Comment 3 Devin Rousso 2019-06-10 22:09:53 PDT
Comment on attachment 371820 [details]
Patch

I'm not 100% sure what exactly the difference is between `m_committedState` and `m_uncommittedState`, but it makes sense when I think about it, and it worked when testing locally.

Another thing I thought of was to have `WebAutomationProxy` become a subclass of `PageLoadState::Observer` and more "eagerly" fire `waitForNavigationToComplete` callbacks `didChangeIsLoading` is called and `isLoading` is false at that time.  I figured I'd wait to get some feedback to see if this is even the right direction before trying that.
Comment 4 Joseph Pecoraro 2019-06-11 13:32:33 PDT
Comment on attachment 371820 [details]
Patch

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

Makes sense to me. r=me

> Source/WebKit/ChangeLog:12
> +        If the timing of the communications to the `WebPageProxy` about the fact that it's actually
> +        `PageLoadingState::isLoading` is too "slow" or happens after
> +        `WebAutomationSession::waitForNavigationToCompleteOnPage` is called, the automation session
> +        will continue running commands, which can lead to a JavaScript error ("Callback was not

This seems a bit complex. It just sounds like there is a race between waitForNavigationToCompleteOnPage and PageLoadingState isLoading in provisional loads, and this better addresses it.

> Source/WebKit/UIProcess/PageLoadState.h:128
> +    bool isLoadingUncommitted() const;

I wonder if a better name might be `isPerfomingProvisionalLoad()` or just a general `isPerformingLoad` which checks committed / uncommitted. Maybe the networking guys have a better suggestion given they work in this area more often.
Comment 5 Devin Rousso 2019-06-13 20:42:44 PDT
Created attachment 372102 [details]
Patch
Comment 6 Joseph Pecoraro 2019-06-13 22:32:55 PDT
Comment on attachment 372102 [details]
Patch

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

> Source/WebKit/UIProcess/PageLoadState.cpp:175
> +bool PageLoadState::hasPendingLoad() const

I'd have expected a name like this to check both committed and uncommitted.
If the name was `hasPendingProvisionalLoad` I'd expect it to check uncommitted.
Comment 7 Devin Rousso 2019-06-14 01:38:22 PDT
Comment on attachment 372102 [details]
Patch

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

>> Source/WebKit/UIProcess/PageLoadState.cpp:175
>> +bool PageLoadState::hasPendingLoad() const
> 
> I'd have expected a name like this to check both committed and uncommitted.
> If the name was `hasPendingProvisionalLoad` I'd expect it to check uncommitted.

I'd rather not use "provisional", as I think that has a different meaning in this context.  Both `m_committedState` and `m_uncommittedState` have a `provisionalURL` member.  Looking back at the commit history (<https://trac.webkit.org/r160405>), it's been referred to as the "uncommitted" state ("pending" was the closest word I could think of at the time).

Including a check for `isLoading` (which checks `m_comittedState`) in this function would be weird with the current name, as `isLoading(m_committedState)` isn't really "pending" (it's been committed).  If we keep the name as is, I'd prefer to not include `m_committedState`, but if we change it to something more "inclusive" then I have no objection.
Comment 8 Devin Rousso 2019-06-14 19:37:49 PDT
Created attachment 372169 [details]
Patch
Comment 9 WebKit Commit Bot 2019-06-14 20:20:49 PDT
Comment on attachment 372169 [details]
Patch

Clearing flags on attachment: 372169

Committed r246456: <https://trac.webkit.org/changeset/246456>
Comment 10 WebKit Commit Bot 2019-06-14 20:20:51 PDT
All reviewed patches have been landed.  Closing bug.