Disable Ad Click Attribution in ephemeral sessions and make sure conversion requests use an ephemeral, stateless session

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[John Wilander]

Ad Click Attribution should be disabled in ephemeral sessions and conversion requests should use an ephemeral, stateless session.

[John Wilander]

<rdar://problem/49918702>

[John Wilander]

Created attachment 367830 [details]
Patch

[Alex Christensen]

Comment on attachment 367830 [details]
Patch

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

Seems mostly conceptually good to me.

> Source/WebCore/loader/ResourceLoader.cpp:721
> +    if (m_options.storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse
> +        || m_options.storedCredentialsPolicy == StoredCredentialsPolicy::EphemeralStatelessCookieless)

This could just be if it's not equal to ::Use.

> Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183
> -        return completionHandler("No stored Ad Click Attribution data.\n"_s);
> +        return completionHandler("\nNo stored Ad Click Attribution data.\n"_s);

You probably want to put the first newline where this string is used.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593
> +            NetworkSession* networkSession;

May as well initialize this to nullptr here, even though we know it will be initialized later.  Ideally we would have C++17's if-with-initializer.  I sent an email to webkit-dev.

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366
> +    if (!shouldBlockCookies)

This seems like a good job for the operator ||

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984
> +    initializeEphemeralStatelessCookielessSession(configuration);

We should probably lazily initialize this.  Hopefully most users will never need to even allocate this.

> Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010
> +    configuration.allowsCellularAccess = YES;

This needs to come from NetworkSessionCreationParameters or from the other sessions.  There's an internal client that needs to disallow cellular access and it should disallow it for this, too.

[John Wilander]

(In reply to Alex Christensen from comment #3)
> Comment on attachment 367830 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367830&action=review
> 
> Seems mostly conceptually good to me.
> 
> > Source/WebCore/loader/ResourceLoader.cpp:721
> > +    if (m_options.storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse
> > +        || m_options.storedCredentialsPolicy == StoredCredentialsPolicy::EphemeralStatelessCookieless)
> 
> This could just be if it's not equal to ::Use.

True. Will fix.

> > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183
> > -        return completionHandler("No stored Ad Click Attribution data.\n"_s);
> > +        return completionHandler("\nNo stored Ad Click Attribution data.\n"_s);
> 
> You probably want to put the first newline where this string is used.

The reason I added this is after dumping the /cookies/resources/echo-cookies.php frame, I don't get a newline. And in the case of empty maps, I only output this line.

> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593
> > +            NetworkSession* networkSession;
> 
> May as well initialize this to nullptr here, even though we know it will be
> initialized later.  Ideally we would have C++17's if-with-initializer.  I
> sent an email to webkit-dev.

OK, I'll initialize to nullptr.

> > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366
> > +    if (!shouldBlockCookies)
> 
> This seems like a good job for the operator ||

Ah, yes. Will fix.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984
> > +    initializeEphemeralStatelessCookielessSession(configuration);
> 
> We should probably lazily initialize this.  Hopefully most users will never
> need to even allocate this.

OK, I'll try to do that. But if Ad Click Attribution is adopted, this session will be used quite a lot.

> > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010
> > +    configuration.allowsCellularAccess = YES;
> 
> This needs to come from NetworkSessionCreationParameters or from the other
> sessions.  There's an internal client that needs to disallow cellular access
> and it should disallow it for this, too.

OK. Hopefully I can pick it up when I do my lazy initialization.

Thanks!

[John Wilander]

Created attachment 367848 [details]
Patch

[John Wilander]

Created attachment 367849 [details]
Patch

[John Wilander]

(In reply to John Wilander from comment #4)
> (In reply to Alex Christensen from comment #3)
> > Comment on attachment 367830 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=367830&action=review
> > 
> > Seems mostly conceptually good to me.
> > 
> > > Source/WebCore/loader/ResourceLoader.cpp:721
> > > +    if (m_options.storedCredentialsPolicy == StoredCredentialsPolicy::DoNotUse
> > > +        || m_options.storedCredentialsPolicy == StoredCredentialsPolicy::EphemeralStatelessCookieless)
> > 
> > This could just be if it's not equal to ::Use.
> 
> True. Will fix.
> 
> > > Source/WebKit/NetworkProcess/AdClickAttributionManager.cpp:183
> > > -        return completionHandler("No stored Ad Click Attribution data.\n"_s);
> > > +        return completionHandler("\nNo stored Ad Click Attribution data.\n"_s);
> > 
> > You probably want to put the first newline where this string is used.
> 
> The reason I added this is after dumping the
> /cookies/resources/echo-cookies.php frame, I don't get a newline. And in the
> case of empty maps, I only output this line.

I don't see a way to make the expect file to look nice with out this leading newline. Thoughts?

> > > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:593
> > > +            NetworkSession* networkSession;
> > 
> > May as well initialize this to nullptr here, even though we know it will be
> > initialized later.  Ideally we would have C++17's if-with-initializer.  I
> > sent an email to webkit-dev.
> 
> OK, I'll initialize to nullptr.
> 
> > > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:366
> > > +    if (!shouldBlockCookies)
> > 
> > This seems like a good job for the operator ||
> 
> Ah, yes. Will fix.

I did it with || in both places.

> > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:984
> > > +    initializeEphemeralStatelessCookielessSession(configuration);
> > 
> > We should probably lazily initialize this.  Hopefully most users will never
> > need to even allocate this.
> 
> OK, I'll try to do that. But if Ad Click Attribution is adopted, this
> session will be used quite a lot.

Can I really lazy initialize this since I need the configuration parameters that's sent to the constructor?

> > > Source/WebKit/NetworkProcess/cocoa/NetworkSessionCocoa.mm:1010
> > > +    configuration.allowsCellularAccess = YES;
> > 
> > This needs to come from NetworkSessionCreationParameters or from the other
> > sessions.  There's an internal client that needs to disallow cellular access
> > and it should disallow it for this, too.
> 
> OK. Hopefully I can pick it up when I do my lazy initialization.

I've changed to set this according to the parameters. But no last initialization yet. Please advise.

[Alex Christensen]

Comment on attachment 367849 [details]
Patch

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

> Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:218
> +        m_task = [cocoaSession.m_ephemeralStatelessCookielessSession dataTaskWithRequest:nsRequest];

To lazily initialize m_ephemeralStatelessCookielessSession, just put the call to initializeEphemeralStatelessCookielessSession before this line.  Then inside initializeEphemeralStatelessCookielessSession early return if m_ephemeralStatelessCookielessSession is non null.

[John Wilander]

(In reply to Alex Christensen from comment #8)
> Comment on attachment 367849 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=367849&action=review
> 
> > Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:218
> > +        m_task = [cocoaSession.m_ephemeralStatelessCookielessSession dataTaskWithRequest:nsRequest];
> 
> To lazily initialize m_ephemeralStatelessCookielessSession, just put the
> call to initializeEphemeralStatelessCookielessSession before this line. 
> Then inside initializeEphemeralStatelessCookielessSession early return if
> m_ephemeralStatelessCookielessSession is non null.

But initializeEphemeralStatelessCookielessSession() takes an NSURLSessionConfiguration.

Do you mean I should instead pick up the configuration from one of the other, already existing sessions?

[Alex Christensen]

Right.  From m_statelessSession

[John Wilander]

(In reply to Alex Christensen from comment #10)
> Right.  From m_statelessSession

Got it. Thanks!

[John Wilander]

Created attachment 367852 [details]
Patch for landing

[Chris Dumez]

Comment on attachment 367852 [details]
Patch for landing

Clearing flags on attachment: 367852

Committed r244475: <https://trac.webkit.org/changeset/244475>

[Chris Dumez]

All reviewed patches have been landed.  Closing bug.

[John Wilander]

Thanks for landing it for me, Chris! If there's any problem with the patch, send it to me, not Chris. He landed it for me because the commit queue is currently broken.