Bug 200108 - WebPageProxy::receivedPolicyDecision should check navigation ID before clear pendingAPIRequest
Summary: WebPageProxy::receivedPolicyDecision should check navigation ID before clear ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jiewen Tan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-07-24 20:12 PDT by Jiewen Tan
Modified: 2019-08-27 01:25 PDT (History)
6 users (show)

See Also:


Attachments
Patch (20.53 KB, patch)
2019-07-24 20:30 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.77 MB, application/zip)
2019-07-24 21:32 PDT, Build Bot
no flags Details
Patch (20.53 KB, patch)
2019-07-25 14:05 PDT, Jiewen Tan
no flags Details | Formatted Diff | Diff
Patch for Landing (20.66 KB, patch)
2019-07-25 15:56 PDT, Jiewen Tan
cdumez: commit-queue?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jiewen Tan 2019-07-24 20:12:57 PDT
PageLoadState::clearPendingAPIRequestURL should not clear the request when navigations mismatch.
Comment 1 Jiewen Tan 2019-07-24 20:14:39 PDT
<rdar://problem/53521238>
Comment 2 Jiewen Tan 2019-07-24 20:30:57 PDT
Created attachment 374858 [details]
Patch
Comment 3 Chris Dumez 2019-07-24 20:45:20 PDT
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.
Comment 4 Build Bot 2019-07-24 21:32:11 PDT
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
Comment 5 Build Bot 2019-07-24 21:32:13 PDT
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
Comment 6 Jiewen Tan 2019-07-25 13:48:01 PDT
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.
Comment 7 Jiewen Tan 2019-07-25 14:05:13 PDT
Created attachment 374903 [details]
Patch
Comment 8 Chris Dumez 2019-07-25 15:35:17 PDT
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.
Comment 9 Jiewen Tan 2019-07-25 15:51:08 PDT
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;
Comment 10 Jiewen Tan 2019-07-25 15:56:22 PDT
Created attachment 374917 [details]
Patch for Landing
Comment 11 Build Bot 2019-07-25 15:58:06 PDT
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.
Comment 12 Chris Dumez 2019-07-25 16:02:53 PDT
Comment on attachment 374917 [details]
Patch for Landing

LGTM.
Comment 13 Jiewen Tan 2019-07-25 18:24:35 PDT
It looks like those API test failures have nothing to do with this patch. Let's land it!
Comment 14 Jiewen Tan 2019-07-25 18:26:31 PDT
Committed r247851: <https://trac.webkit.org/changeset/247851>
Comment 15 Jiewen Tan 2019-07-25 19:34:43 PDT
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.
Comment 16 Jiewen Tan 2019-07-25 19:38:54 PDT
Build fix:
Committed r247853: <https://trac.webkit.org/changeset/247853>