Bug 159377 - Send cookieAcceptPolicy to newly created processes
Summary: Send cookieAcceptPolicy to newly created processes
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Alex Christensen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-07-01 17:34 PDT by Alex Christensen
Modified: 2016-07-05 15:22 PDT (History)
3 users (show)

See Also:


Attachments
Patch (12.30 KB, patch)
2016-07-01 17:38 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff
Patch (3.03 KB, patch)
2016-07-05 13:23 PDT, Alex Christensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alex Christensen 2016-07-01 17:34:26 PDT
Send cookieAcceptPolicy to newly created processes
Comment 1 Alex Christensen 2016-07-01 17:38:52 PDT
Created attachment 282615 [details]
Patch
Comment 2 Alexey Proskuryakov 2016-07-01 23:22:27 PDT
Comment on attachment 282615 [details]
Patch

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

> Source/WebKit2/ChangeLog:3
> +        Send cookieAcceptPolicy to newly created processes

I do not think that this is the correct fix.

Have you verified that all processes are actually using the same cookie storage on disk? The observed issue would be easiest to explain if WebKit failed to share the storage (see rdar://problem/20572830 for some details). Another potential explanation is that the fix for this very issue in rdar://problem/20904218 didn't quite work.

> Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:-163
> -    // - When not testing, Cocoa has the policy persisted, and thus new processes use it (even for ephemeral sessions).

What this comment says.

> Source/WebKit2/UIProcess/mac/WebCookieManagerProxyMac.mm:34
> +    // This will probably eventually be stored on the disk if this process stays alive long enough, but probably not immediately.
> +    // Do not rely on this immediately changing anything other than a value in memory in the current process.

Please see rdar://problem/20904218 for details on the expected behavior.
Comment 3 Alex Christensen 2016-07-05 11:02:37 PDT
I am aware of these problems and their solutions, and I still think this is the correct solution to this problem.  The case I have observed is when the UIProcess calls setHTTPCookieAcceptPolicy and then immediately creates its first WebProcess and NetworkProcess before CFNetwork has persisted the change to disk.  The NetworkProcess and WebProcess now have the old policy, and they then persist the old policy to disk.  The solution is simply not to rely on CFNetwork having persisted anything to disk, unless there is a guarantee that it will happen immediately and synchronously, which there is not.
Comment 4 Alex Christensen 2016-07-05 11:03:25 PDT
Comment on attachment 282615 [details]
Patch

Noting that Alexey initially gave this patch an r- and re-requesting review.
Comment 5 Alexey Proskuryakov 2016-07-05 11:29:20 PDT
I still don't like the idea of multiple processes racing to write the policy to disk.
Comment 6 Alex Christensen 2016-07-05 13:23:40 PDT
Created attachment 282812 [details]
Patch
Comment 7 Alexey Proskuryakov 2016-07-05 14:52:48 PDT
Comment on attachment 282812 [details]
Patch

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

> Source/WebKit2/ChangeLog:12
> +        * UIProcess/mac/WebCookieManagerProxyMac.mm:

One day we should rename the ones that are also used on iOS.
Comment 8 WebKit Commit Bot 2016-07-05 15:22:29 PDT
Comment on attachment 282812 [details]
Patch

Clearing flags on attachment: 282812

Committed r202834: <http://trac.webkit.org/changeset/202834>
Comment 9 WebKit Commit Bot 2016-07-05 15:22:33 PDT
All reviewed patches have been landed.  Closing bug.