Bug 184678

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 LoadingAssignee: 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 Flags
Patch
none
Patch
none
Patch
none
Patch none

Description Chris Dumez 2018-04-16 19:09:37 PDT
CMD-clicking an iCloud web app link unexpectedly opens that link in a new tab and the current tab.
Comment 1 Chris Dumez 2018-04-16 19:09:53 PDT
<rdar://problem/39422122>
Comment 2 Chris Dumez 2018-04-16 20:54:57 PDT
Created attachment 338074 [details]
Patch
Comment 3 Chris Dumez 2018-04-17 08:36:48 PDT
Created attachment 338114 [details]
Patch
Comment 4 Alex Christensen 2018-04-17 08:53:13 PDT
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.
Comment 5 Chris Dumez 2018-04-17 08:55:02 PDT
(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.
Comment 6 Chris Dumez 2018-04-17 09:07:14 PDT
Created attachment 338124 [details]
Patch
Comment 7 Chris Dumez 2018-04-17 09:07:38 PDT
(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 8 Chris Dumez 2018-04-17 09:12:58 PDT
Comment on attachment 338124 [details]
Patch

Alex pointed out an issue offline.
Comment 9 Chris Dumez 2018-04-17 09:18:02 PDT
Created attachment 338125 [details]
Patch
Comment 10 Alex Christensen 2018-04-17 09:51:44 PDT
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 11 Chris Dumez 2018-04-17 10:40:40 PDT
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 12 Chris Dumez 2018-04-17 10:42:30 PDT
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 13 WebKit Commit Bot 2018-04-17 11:13:55 PDT
Comment on attachment 338125 [details]
Patch

Clearing flags on attachment: 338125

Committed r230721: <https://trac.webkit.org/changeset/230721>
Comment 14 WebKit Commit Bot 2018-04-17 11:13:57 PDT
All reviewed patches have been landed.  Closing bug.