Bug 159377

Summary: Send cookieAcceptPolicy to newly created processes
Product: WebKit Reporter: Alex Christensen <achristensen>
Component: New BugsAssignee: Alex Christensen <achristensen>
Severity: Normal CC: ap, commit-queue, mitz
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

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]
Comment 2 Alexey Proskuryakov 2016-07-01 23:22:27 PDT
Comment on attachment 282615 [details]

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]

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]
Comment 7 Alexey Proskuryakov 2016-07-05 14:52:48 PDT
Comment on attachment 282812 [details]

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]

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.