Bug 208026 – App-bound domains should have separate Network Sessions

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[Kate Cheney]

We should switch between App-Bound and In-App-Browser Network Sessions

[Kate Cheney]

<rdar://problem/59434006>

[Kate Cheney]

Created attachment 391338 [details]
Patch

[Kate Cheney]

Created attachment 391339 [details]
Patch

[Brent Fulgham]

Comment on attachment 391339 [details]
Patch

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

This looks very good! I have a few minor things I'd like addressed.

> Source/WebKit/NetworkProcess/NetworkDataTask.cpp:53
> +    return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain);

At some point, it seems like we should just past the parameters object to the 'create' method! :-)

> Source/WebKit/NetworkProcess/NetworkLoadParameters.h:64
> +    bool isNavigatingToAppBoundDomain { true };

It seems like the default state should be false here (most traffic is likely not app-bound).

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:124
> +

Extra whitespace.

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77
> +    bool isNavigatingToAppBoundDomain;

Whitespace, and should have a default argument.

> Source/WebKit/Shared/LoadParameters.h:72
> +    bool isNavigatingToAppBoundDomain;

Please provide a default initializer (false).

> Source/WebKit/UIProcess/WebPageProxy.cpp:3064
> +    void send(PolicyDecision&& policyDecision)

Oops! :-)

> Source/WebKit/UIProcess/WebPageProxy.cpp:3083
> +bool WebPageProxy::isAppBoundDomain(WebCore::RegistrableDomain domain)

Could this be "const WebCore::RegistrableDomain&" ?

> Source/WebKit/UIProcess/WebPageProxy.h:2259
> +    bool isAppBoundDomain(WebCore::RegistrableDomain);

Could this be const? Also seems like the argument (which is effectively a String) would benefit from being passed by reference.

> Source/WebKit/UIProcess/WebPageProxy.h:2261
> +    bool isNavigatingToAppBoundDomain() { return m_isNavigatingToAppBoundDomain; }

This should be "bool isNavigatingToAppBoundDomain() const"

> Source/WebKit/WebProcess/WebPage/WebPage.h:1290
> +    bool isNavigatingToAppBoundDomain() { return m_isNavigatingToAppBoundDomain; }

Should be const.

> Source/WebKit/WebProcess/WebPage/WebPage.h:2073
> +    bool m_isNavigatingToAppBoundDomain { true };

I think this should default to 'false'.

[Kate Cheney]

(In reply to Brent Fulgham from comment #4)
> Comment on attachment 391339 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=391339&action=review
> 
> This looks very good! I have a few minor things I'd like addressed.
> 

Thanks for the comments! I'll make these changes.

> > Source/WebKit/NetworkProcess/NetworkDataTask.cpp:53
> > +    return NetworkDataTaskCocoa::create(session, client, parameters.request, parameters.webFrameID, parameters.webPageID, parameters.storedCredentialsPolicy, parameters.contentSniffingPolicy, parameters.contentEncodingSniffingPolicy, parameters.shouldClearReferrerOnHTTPSToHTTPRedirect, parameters.shouldPreconnectOnly, parameters.isMainFrameNavigation, parameters.isMainResourceNavigationForAnyFrame, parameters.networkActivityTracker, parameters.isNavigatingToAppBoundDomain);
> 
> At some point, it seems like we should just past the parameters object to
> the 'create' method! :-)
> 

Agreed!

> > Source/WebKit/NetworkProcess/NetworkLoadParameters.h:64
> > +    bool isNavigatingToAppBoundDomain { true };
> 
> It seems like the default state should be false here (most traffic is likely
> not app-bound).
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:124
> > +
> 
> Extra whitespace.
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77
> > +    bool isNavigatingToAppBoundDomain;
> 
> Whitespace, and should have a default argument.
> 
> > Source/WebKit/Shared/LoadParameters.h:72
> > +    bool isNavigatingToAppBoundDomain;
> 
> Please provide a default initializer (false).
> 
> > Source/WebKit/UIProcess/WebPageProxy.cpp:3064
> > +    void send(PolicyDecision&& policyDecision)
> 
> Oops! :-)
> 

Yep, my bad. This was my own mistake from a previous patch...
> > Source/WebKit/WebProcess/WebPage/WebPage.h:2073
> > +    bool m_isNavigatingToAppBoundDomain { true };
> 
> I think this should default to 'false'.

I think there's a few other places where I default to true, I'll change those too!

[Kate Cheney]

Created attachment 391355 [details]
Patch

[Brent Fulgham]

Comment on attachment 391355 [details]
Patch

Looks great!

[Brent Fulgham]

Comment on attachment 391355 [details]
Patch

r=me once you confirm that EWS is happy. Please do double-check with Chris or Youenn if they are available tomorrow.

[Kate Cheney]

(In reply to Brent Fulgham from comment #8)
> Comment on attachment 391355 [details]
> Patch
> 
> r=me once you confirm that EWS is happy. Please do double-check with Chris
> or Youenn if they are available tomorrow.

Thanks! Sounds good, I also asked John to take a look.

[John Wilander]

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

First batch of comments.

> Source/WebKit/ChangeLog:13
> +        domains. This requires updating the webPage when a navigation occurs

Weird camel casing of webPage. Is that a reference to a variable?

> Source/WebKit/ChangeLog:103
> +        the new webPage via the ProvisionalPageProxy.

Same thing with the camel casing.

> Source/WebKit/ChangeLog:112
> +        These functions determine if the category is appBound and update the

Should be app-bound.

> Source/WebKit/NetworkProcess/NetworkProcess.h:346
> +    void setInAppBrowserPrivacyEnabled(PAL::SessionID, bool, CompletionHandler<void()>&&);

I would give the bool a name even though it is pretty obvious what it's for.

> Source/WebKit/NetworkProcess/NetworkSession.h:100
> +

Was there an edit here?

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:98
> +    SessionWrapper& sessionWrapperForTask(const WebCore::ResourceRequest&, WebCore::StoredCredentialsPolicy, bool);

The bool needs to have a name here.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.h:100
> +    bool isInAppBrowserPrivacyEnabled() { return m_isInAppBrowserPrivacyEnabled; }

Const.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1201
> +    if (m_isInAppBrowserPrivacyEnabled) {

This should be if (m_isInAppBrowserPrivacyEnabled && isNavigatingToAppBoundDomain) and then no curly braces.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1296
> +    if (m_appBoundSession)

This should be return !!m_appBoundSession. Could be moved to the header if you so wish.

> Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataStore.mm:489
> +    auto category = webPageProxy->configuration().webViewCategory();

Do we have to use a variable here or could the call be made like so: completionHandler(toWKWebViewCategory(webPageProxy->configuration().webViewCategory()) ?

> Source/WebKit/UIProcess/ProvisionalPageProxy.cpp:348
> +        reply(PolicyDecision { identifier, true, WebCore::PolicyAction::Ignore, navigationID, DownloadID(), WTF::nullopt });

I think we need to add a boolean enum NavigatingToAppBoundDomain::{Yes, No} so that PolicyDecision.h can take one of those. Instantiating a PolicyDecision with just "true" makes it unclear what your code does. With such an enum you can make your getters isNavigatingToAppBoundDomain() return it instead of a boolean too and you don't have to name all those bool parameters in header files. Load parameters can use it too.

> Source/WebKit/UIProcess/ProvisionalPageProxy.h:95
> +    void loadRequest(API::Navigation&, WebCore::ResourceRequest&&, API::Object* userData, bool, Optional<WebsitePoliciesData>&& = WTF::nullopt);

This bool needs a name.

[Kate Cheney]

Thanks John!

Seems like Mac-wk2 tests also need some investigating, looking into those.

[John Wilander]

Comment on attachment 391355 [details]
Patch

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

Second batch of comments. Looks good overall!

> Source/WebKit/ChangeLog:13
> +        domains. This requires updating the webPage when a navigation occurs

Weird camel casing of webPage. Is that a reference to a variable?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3083
> +bool WebPageProxy::isAppBoundDomain(WebCore::RegistrableDomain domain)

RegistrableDomain& and const both for the parameter and the function?

> Source/WebKit/UIProcess/WebPageProxy.cpp:3091
> +    if (m_preferences->isInAppBrowserPrivacyEnabled()) {

This should be if (m_preferences->isInAppBrowserPrivacyEnabled() && isMainFrame) so that you can drop one nested context.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3093
> +            RegistrableDomain currentDomain { requestURL };

This variable is never used.

> Source/WebKit/UIProcess/WebPageProxy.cpp:3094
> +            if (!isAppBoundDomain(RegistrableDomain(requestURL))) {

You could have a convenience function isAppBoundDomain(URL&) that does the conversion for you and calls isAppBoundDomain(isAppBoundDomain&).

> Source/WebKit/UIProcess/WebPageProxy.h:2259
> +    bool isAppBoundDomain(WebCore::RegistrableDomain);

This should probably be isAppBoundDomain(const WebCore::RegistrableDomain&) const;

> Source/WebKit/UIProcess/WebPageProxy.h:2260
> +    void setIsNavigatingToAppBoundDomain(bool isMainFrame, const URL& requestURL);

requestURL is unnecessary.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:330
> +    auto* webFrameLoaderClient = toWebFrameLoaderClient(resourceLoader.frame()->loader().client());

Can resourceLoader.frame() return null?

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:339
>          // FIXME: Instead of passing userContentControllerIdentifier, the NetworkProcess should be able to get it using webPageId.

Does this FIXME need to be moved along with the assignment statements? I don't think so.

> Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp:687
>      // FIXME: Instead of passing userContentControllerIdentifier, we should just pass webPageId to NetworkProcess.

Ditto.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:839
> +        m_frame->didReceivePolicyDecision(listenerID, PolicyDecision { identifier, true, PolicyAction::Ignore, 0, { }, { } });

Again, we should not call with raw booleans like this.

> Source/WebKit/WebProcess/WebCoreSupport/WebFrameLoaderClient.cpp:996
> +            m_frame->didReceivePolicyDecision(listenerID, PolicyDecision { requestIdentifier, false, PolicyAction::Ignore, 0, { }, { } });

Ditto.

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:3265
> +void WebPage::didReceivePolicyDecision(FrameIdentifier frameID, uint64_t listenerID, PolicyDecision&& policyDecision)

Good catch.

> Tools/ChangeLog:11
> +        manually for the test. One ensures that setting the webView category

Weird camel casing of webView.

> Tools/WebKitTestRunner/TestController.h:263
> +    void getWebViewCategory();

Const?

> Tools/WebKitTestRunner/TestController.h:303
> +    bool hasAppBoundSession();

Const?

> LayoutTests/platform/mac-wk2/TestExpectations:61
> +http/tests/in-app-browser-privacy/ [ Pass ]

You should probably have the same comment here.

[John Wilander]

(In reply to John Wilander from comment #12)
> You could have a convenience function isAppBoundDomain(URL&) that does the
> conversion for you and calls isAppBoundDomain(isAppBoundDomain&).

I mean to say "… calls isAppBoundDomain(RegistrableDomain&)."

[Kate Cheney]

Created attachment 391418 [details]
Patch

[Kate Cheney]

Created attachment 391421 [details]
Patch

[Kate Cheney]

Created attachment 391426 [details]
Patch

[Kate Cheney]

Created attachment 391429 [details]
Patch

[Kate Cheney]

(In reply to katherine_cheney from comment #17)
> Created attachment 391429 [details]
> Patch

trying to get this new mac-wk2 test to pass (runs fine locally, not sure what's going on here)

[Kate Cheney]

Created attachment 391430 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 391430 [details]
Patch

Clearing flags on attachment: 391430

Committed r257185: <https://trac.webkit.org/changeset/257185>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59691947>