Summary: | WebPageProxy::receivedPolicyDecision should check navigation ID before clear pendingAPIRequest | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Jiewen Tan <jiewen_tan> | ||||||||||
Component: | WebKit Misc. | Assignee: | Jiewen Tan <jiewen_tan> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | cdumez, ews-watchlist, jiewen_tan, rniwa, webkit-bug-importer, youennf | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=201175 | ||||||||||||
Attachments: |
|
Description
Jiewen Tan
2019-07-24 20:12:57 PDT
Created attachment 374858 [details]
Patch
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 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 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 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. Created attachment 374903 [details]
Patch
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 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; Created attachment 374917 [details]
Patch for Landing
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 on attachment 374917 [details]
Patch for Landing
LGTM.
It looks like those API test failures have nothing to do with this patch. Let's land it! Committed r247851: <https://trac.webkit.org/changeset/247851> 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. Build fix: Committed r247853: <https://trac.webkit.org/changeset/247853> |