Bug 200108 – WebPageProxy::receivedPolicyDecision should check navigation ID before clear pendingAPIRequest

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Jiewen Tan]

PageLoadState::clearPendingAPIRequestURL should not clear the request when navigations mismatch.

[Jiewen Tan]

<rdar://problem/53521238>

[Jiewen Tan]

Created attachment 374858 [details]
Patch

[Chris Dumez]

Comment on attachment 374858 [details]
Patch

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

> Source/WebKit/UIProcess/PageLoadState.cpp:157
> +    m_uncommittedState.pendingAPIRequest.navigationID = 0;

Would m_uncommittedState.pendingAPIRequest = { }; work?

> Source/WebKit/UIProcess/PageLoadState.cpp:248
> +void PageLoadState::setPendingAPIRequestURL(const Transaction::Token& token, const uint64_t navigationID, const String& pendingAPIRequestURL, const URL& resourceDirectoryURL)

Could this be called setPendingAPIRequest() and then the second parameter would be a PendingAPIRequest&& ?

> Source/WebKit/UIProcess/PageLoadState.cpp:251
> +    m_uncommittedState.pendingAPIRequest.navigationID = navigationID;

m_uncommittedState.pendingAPIRequest = { navigationID, pendingAPIRequestURL }; ?

> Source/WebKit/UIProcess/PageLoadState.cpp:256
> +void PageLoadState::clearPendingAPIRequestURL(const Transaction::Token& token, const uint64_t navigationID)

No need for 'const' here.
I would call this "clearPendingAPIRequest".
I am not convinced it is a good idea to take the navigationID as parameter here. I find it a bit confusing. It might look clearer to have the call sites if their navigation is the pending one before calling clearPendingAPIRequest().

> Source/WebKit/UIProcess/PageLoadState.cpp:261
> +    m_uncommittedState.pendingAPIRequest.navigationID = 0;

m_uncommittedState.pendingAPIRequest = { }; ?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:619
> +    TestWebKitAPI::Util::sleep(0.5); // Wait until the pending api request gets clear.

Looks like this has the potential to be flaky. This could be written with a loop where you keep checking the URL and sleep just a little bit during each iteration.

[EWS Watchlist]

Comment on attachment 374858 [details]
Patch

Attachment 374858 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/12806692

New failing tests:
swipe/pushState-cached-back-swipe.html

[EWS Watchlist]

Created attachment 374869 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[Jiewen Tan]

Comment on attachment 374858 [details]
Patch

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

Thanks for reviewing this patch, Chris.

>> Source/WebKit/UIProcess/PageLoadState.cpp:157
>> +    m_uncommittedState.pendingAPIRequest.navigationID = 0;
> 
> Would m_uncommittedState.pendingAPIRequest = { }; work?

Should definitely work!

>> Source/WebKit/UIProcess/PageLoadState.cpp:248
>> +void PageLoadState::setPendingAPIRequestURL(const Transaction::Token& token, const uint64_t navigationID, const String& pendingAPIRequestURL, const URL& resourceDirectoryURL)
> 
> Could this be called setPendingAPIRequest() and then the second parameter would be a PendingAPIRequest&& ?

Got you.

>> Source/WebKit/UIProcess/PageLoadState.cpp:251
>> +    m_uncommittedState.pendingAPIRequest.navigationID = navigationID;
> 
> m_uncommittedState.pendingAPIRequest = { navigationID, pendingAPIRequestURL }; ?

Fixed.

>> Source/WebKit/UIProcess/PageLoadState.cpp:256
>> +void PageLoadState::clearPendingAPIRequestURL(const Transaction::Token& token, const uint64_t navigationID)
> 
> No need for 'const' here.
> I would call this "clearPendingAPIRequest".
> I am not convinced it is a good idea to take the navigationID as parameter here. I find it a bit confusing. It might look clearer to have the call sites if their navigation is the pending one before calling clearPendingAPIRequest().

Got you.

>> Source/WebKit/UIProcess/PageLoadState.cpp:261
>> +    m_uncommittedState.pendingAPIRequest.navigationID = 0;
> 
> m_uncommittedState.pendingAPIRequest = { }; ?

Fixed.

>> Tools/TestWebKitAPI/Tests/WebKitCocoa/DecidePolicyForNavigationAction.mm:619
>> +    TestWebKitAPI::Util::sleep(0.5); // Wait until the pending api request gets clear.
> 
> Looks like this has the potential to be flaky. This could be written with a loop where you keep checking the URL and sleep just a little bit during each iteration.

Added a counter to exit failure case.

[Jiewen Tan]

Created attachment 374903 [details]
Patch

[Chris Dumez]

Comment on attachment 374903 [details]
Patch

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

r=me

> Source/WebKit/UIProcess/PageLoadState.cpp:242
> +const PageLoadState::PendingAPIRequest& PageLoadState::pendingAPIRequest() const

FYI, you could also write:
auto PageLoadState::pendingAPIRequest() const -> const PendingAPIRequest&

This avoid the extra PageLoadState::.

> Source/WebKit/UIProcess/WebPageProxy.cpp:4625
> +        m_pageLoadState.clearPendingAPIRequest(transaction);

Any reason does not check the navigation ID before clearing instead of checking the URL? Checking the URL does not seem reliable.

[Jiewen Tan]

Comment on attachment 374903 [details]
Patch

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

Thanks Chris for r+ this patch.

>> Source/WebKit/UIProcess/PageLoadState.cpp:242
>> +const PageLoadState::PendingAPIRequest& PageLoadState::pendingAPIRequest() const
> 
> FYI, you could also write:
> auto PageLoadState::pendingAPIRequest() const -> const PendingAPIRequest&
> 
> This avoid the extra PageLoadState::.

Fixed.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:4625
>> +        m_pageLoadState.clearPendingAPIRequest(transaction);
> 
> Any reason does not check the navigation ID before clearing instead of checking the URL? Checking the URL does not seem reliable.

I made it: bool fromAPI = navigationID == m_pageLoadState.pendingAPIRequest().navigationID;

[Jiewen Tan]

Created attachment 374917 [details]
Patch for Landing

[EWS Watchlist]

Attachment 374917 [details] did not pass style-queue:


ERROR: Source/WebKit/UIProcess/PageLoadState.cpp:243:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 374917 [details]
Patch for Landing

LGTM.

[Jiewen Tan]

It looks like those API test failures have nothing to do with this patch. Let's land it!

[Jiewen Tan]

Committed r247851: <https://trac.webkit.org/changeset/247851>

[Jiewen Tan]

Comment on attachment 374917 [details]
Patch for Landing

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:4623
> +    bool fromAPI = navigationID == m_pageLoadState.pendingAPIRequest().navigationID;

This change apparently fails SOAuthorizationRedirect.AuthorizationOptions. Will revert it.

[Jiewen Tan]

Build fix:
Committed r247853: <https://trac.webkit.org/changeset/247853>