Bug 207917

Summary: Re-disable top-level data URL navigations
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: WebKit Misc.Assignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: annulen, bfulgham, cdumez, commit-queue, darin, dbates, ews-watchlist, gyuyoung.kim, japhet, ryuan.choi, sergio, webkit-bug-importer, youennf
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 207719    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch
none
Patch for landing none

Description Brent Fulgham 2020-02-18 15:14:11 PST 
Calls to WKPage and WKWebView API used to load data and strings directly should be recognized by the loading code as being from client API calls. This brings these API behaviors into alignment with similar API for loading URLs and URLRequests directly. This change also allows us to enforce stricter handling of Data URLs, and to remove the need to explicitly permit top-level data URL navigation when client APIs are used.
Comment 1 Radar WebKit Bug Importer 2020-02-18 15:15:12 PST 
<rdar://problem/59568037>
Comment 2 Brent Fulgham 2020-02-18 15:22:19 PST 
Created attachment 391103 [details]
Patch
Comment 3 Brent Fulgham 2020-02-18 15:25:49 PST 
Comment on attachment 391103 [details]
Patch

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

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:156
> +    m_page.loadDataWithNavigationShared(m_process.copyRef(), m_webPageID, navigation, data, MIMEType, encoding, baseURL, userData, WebCore::ShouldTreatAsContinuingLoad::Yes, WebCore::ShouldTreatAsClientOrUserInput::No, WTFMove(websitePolicies), navigation.lastNavigationAction().shouldOpenExternalURLsPolicy);

@Youenn, @Chris: Should we ever treat ProvisionalPageProxy loads as if they were generated from client API? It doesn't look like we do for normal load requests, so I don't think we would for data loads, either.
Comment 4 Brent Fulgham 2020-02-18 15:29:27 PST 
Created attachment 391108 [details]
Patch
Comment 5 Chris Dumez 2020-02-18 15:36:20 PST 
Comment on attachment 391108 [details]
Patch

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

> Source/WebKit/UIProcess/WebPageProxy.cpp:1385
> +void WebPageProxy::loadDataWithNavigationShared(Ref<WebProcessProxy>&& process, WebCore::PageIdentifier webPageID, API::Navigation& navigation, const IPC::DataReference& data, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad, ShouldTreatAsClientOrUserInput shouldTreatAsClientOrUserInput, Optional<WebsitePoliciesData>&& websitePolicies, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy)

This is always called from API so it seems odd to take a ShouldTreatAsClientOrUserInput parameter. When is it ever ShouldTreatAsClientOrUserInput::No?
Comment 6 Brent Fulgham 2020-02-18 15:49:02 PST 
Note: We allowed top-level navigations to Data URLs in r255961. We can go back to our previous state with this change.
Comment 7 Brent Fulgham 2020-02-18 15:56:30 PST 
(In reply to Chris Dumez from comment #5)
> Comment on attachment 391108 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391108&action=review
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:1385
> > +void WebPageProxy::loadDataWithNavigationShared(Ref<WebProcessProxy>&& process, WebCore::PageIdentifier webPageID, API::Navigation& navigation, const IPC::DataReference& data, const String& MIMEType, const String& encoding, const String& baseURL, API::Object* userData, ShouldTreatAsContinuingLoad shouldTreatAsContinuingLoad, ShouldTreatAsClientOrUserInput shouldTreatAsClientOrUserInput, Optional<WebsitePoliciesData>&& websitePolicies, ShouldOpenExternalURLsPolicy shouldOpenExternalURLsPolicy)
> 
> This is always called from API so it seems odd to take a
> ShouldTreatAsClientOrUserInput parameter. When is it ever
> ShouldTreatAsClientOrUserInput::No?

Excellent -- this can be a much simpler change, then.
Comment 8 Brent Fulgham 2020-02-18 16:01:25 PST 
Created attachment 391113 [details]
Patch
Comment 9 Brent Fulgham 2020-02-18 18:51:44 PST 
Created attachment 391125 [details]
Patch
Comment 10 Darin Adler 2020-02-18 19:57:28 PST 
Comment on attachment 391125 [details]
Patch

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

> Source/WebCore/ChangeLog:3
> +        Recognize WKPage and WKWebView calls as generated by client API use

Wrong title here.
Comment 11 Chris Dumez 2020-02-19 08:21:38 PST 
Comment on attachment 391125 [details]
Patch

LGTM too.
Comment 12 Brent Fulgham 2020-02-19 10:04:32 PST 
Created attachment 391175 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2020-02-19 10:36:49 PST 
Comment on attachment 391175 [details]
Patch for landing

Clearing flags on attachment: 391175

Committed r256925: <https://trac.webkit.org/changeset/256925>
Comment 14 WebKit Commit Bot 2020-02-19 10:36:51 PST 
All reviewed patches have been landed.  Closing bug.