Summary: | REGRESSION (r229831): CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||
Component: | Page Loading | Assignee: | Chris Dumez <cdumez> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | achristensen, ap, beidson, commit-queue, dbates, ews-watchlist, japhet, webkit-bug-importer | ||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||
Version: | WebKit Nightly Build | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | |||||||||||||
Bug Blocks: | 180568 | ||||||||||||
Attachments: |
|
Description
Chris Dumez
2018-04-16 19:09:37 PDT
Created attachment 338074 [details]
Patch
Created attachment 338114 [details]
Patch
Comment on attachment 338114 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338114&action=review > Source/WebKit/UIProcess/WebPageProxy.messages.in:103 > + DecidePolicyForNavigationActionSync(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, uint64_t listenerID, WebKit::UserData userData) -> (uint64_t newNavigationID, enum WebCore::PolicyAction policyAction, WebKit::DownloadID downloadID, std::optional<WebKit::WebsitePoliciesData> websitePolicies) I think it would look a lot nicer if this were delayed. (In reply to Alex Christensen from comment #4) > Comment on attachment 338114 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338114&action=review > > > Source/WebKit/UIProcess/WebPageProxy.messages.in:103 > > + DecidePolicyForNavigationActionSync(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, uint64_t listenerID, WebKit::UserData userData) -> (uint64_t newNavigationID, enum WebCore::PolicyAction policyAction, WebKit::DownloadID downloadID, std::optional<WebKit::WebsitePoliciesData> websitePolicies) > > I think it would look a lot nicer if this were delayed. Not very familiar with this but I'll figure it out from existing code. Created attachment 338124 [details]
Patch
(In reply to Alex Christensen from comment #4) > Comment on attachment 338114 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=338114&action=review > > > Source/WebKit/UIProcess/WebPageProxy.messages.in:103 > > + DecidePolicyForNavigationActionSync(uint64_t frameID, struct WebCore::SecurityOriginData frameSecurityOrigin, uint64_t navigationID, struct WebKit::NavigationActionData navigationActionData, struct WebKit::FrameInfoData originatingFrameInfoData, uint64_t originatingPageID, WebCore::ResourceRequest originalRequest, WebCore::ResourceRequest request, uint64_t listenerID, WebKit::UserData userData) -> (uint64_t newNavigationID, enum WebCore::PolicyAction policyAction, WebKit::DownloadID downloadID, std::optional<WebKit::WebsitePoliciesData> websitePolicies) > > I think it would look a lot nicer if this were delayed. Done, much nicer / simpler indeed. Comment on attachment 338124 [details]
Patch
Alex pointed out an issue offline.
Created attachment 338125 [details]
Patch
Comment on attachment 338125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338125&action=review > Source/WebKit/UIProcess/WebPageProxy.cpp:3952 > + // If the client did not respond synchronously, proceed with the load. > + if (auto syncNavigationActionPolicyReply = WTFMove(m_syncNavigationActionPolicyReply)) > + syncNavigationActionPolicyReply->send(navigationID, PolicyAction::Use, { }, { }); What should we do with the call to WebPageProxy::receivedPolicyDecision which will happen later, then? I think we should do nothing there. Comment on attachment 338125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338125&action=review >> Source/WebKit/UIProcess/WebPageProxy.cpp:3952 >> + syncNavigationActionPolicyReply->send(navigationID, PolicyAction::Use, { }, { }); > > What should we do with the call to WebPageProxy::receivedPolicyDecision which will happen later, then? I think we should do nothing there. If we do not do anything here then the WebProcess will be hung for an undetermined duration, while we await the client response. I think we tried this in the past and this caused regression, no? I want to restore pre-r229831 behavior, where the load would keep going if the client does not answer synchronously. I am trying to fix a regression from r229831 here and restoring pre-r229831 behavior is the safest bet. What you are suggesting seems to be another behavior. Comment on attachment 338125 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=338125&action=review >>> Source/WebKit/UIProcess/WebPageProxy.cpp:3952 >>> + syncNavigationActionPolicyReply->send(navigationID, PolicyAction::Use, { }, { }); >> >> What should we do with the call to WebPageProxy::receivedPolicyDecision which will happen later, then? I think we should do nothing there. > > If we do not do anything here then the WebProcess will be hung for an undetermined duration, while we await the client response. I think we tried this in the past and this caused regression, no? > > I want to restore pre-r229831 behavior, where the load would keep going if the client does not answer synchronously. I am trying to fix a regression from r229831 here and restoring pre-r229831 behavior is the safest bet. What you are suggesting seems to be another behavior. To answer your question, my understanding is that when the client responds asynchronously, WebPageProxy::receivedPolicyDecision() will send the WebPage::DidReceivePolicyDecision IPC to the WebProcess, which the WebProcess will ignore because there is no longer any listener for this ListenerID. Comment on attachment 338125 [details] Patch Clearing flags on attachment: 338125 Committed r230721: <https://trac.webkit.org/changeset/230721> All reviewed patches have been landed. Closing bug. |