Bug 198741 – waitForNavigationToComplete may be called before WebPageProxy knows it's loading

• Status: RESOLVED
• Resolution: FIXED
• Component: WebDriver

[Devin Rousso]

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".

[Devin Rousso]

<rdar://problem/31164316>

[Devin Rousso]

Created attachment 371820 [details]
Patch

[Devin Rousso]

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.

[Joseph Pecoraro]

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.

[Devin Rousso]

Created attachment 372102 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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.

[Devin Rousso]

Created attachment 372169 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 372169 [details]
Patch

Clearing flags on attachment: 372169

Committed r246456: <https://trac.webkit.org/changeset/246456>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.