Bug 158616 - Vary:Cookie validation doesn't work in private browsing
Summary: Vary:Cookie validation doesn't work in private browsing
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on: 158665
Blocks:
  Show dependency treegraph
 
Reported: 2016-06-10 07:47 PDT by Antti Koivisto
Modified: 2016-06-15 07:10 PDT (History)
9 users (show)

See Also:


Attachments
patch (19.58 KB, patch)
2016-06-10 08:30 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (19.63 KB, patch)
2016-06-10 09:46 PDT, Antti Koivisto
darin: review+
Details | Formatted Diff | Diff
patch (24.64 KB, patch)
2016-06-11 02:56 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
patch (22.40 KB, patch)
2016-06-13 07:03 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2016-06-10 07:47:18 PDT
We currently reload any resource with Vary:Cookie in private browsing even if we have a memory cache entry.
Comment 1 Antti Koivisto 2016-06-10 08:30:27 PDT
Created attachment 281010 [details]
patch
Comment 2 Antti Koivisto 2016-06-10 09:46:46 PDT
Created attachment 281012 [details]
patch
Comment 3 Alex Christensen 2016-06-10 09:49:22 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=281010&action=review

Is this a NetworkStorageSession in the WebProcess?  Are its cookies synchronized with the cookies in the NetworkProcess somehow?  Could we use this map from SessionTracker so we don't have duplicate code?

> Source/WebCore/ChangeLog:13
> +        around curretnly WebKit2 level SessionTracker.

currently
Comment 4 Darin Adler 2016-06-10 15:28:35 PDT
Comment on attachment 281012 [details]
patch

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

> Source/WebCore/platform/network/NetworkStorageSession.h:77
> +    NetworkStorageSession(SessionID);

Maybe mark this explicit?

> Source/WebCore/platform/network/cf/NetworkStorageSessionCFNet.cpp:44
>  NetworkStorageSession::NetworkStorageSession(SessionID sessionID, RetainPtr<CFURLStorageSessionRef> platformSession)

Argument type should be RetainPtr&&.
Comment 5 Antti Koivisto 2016-06-11 02:56:20 PDT
Created attachment 281096 [details]
patch
Comment 6 Radar WebKit Bug Importer 2016-06-11 03:53:54 PDT
<rdar://problem/26755067>
Comment 7 Antti Koivisto 2016-06-11 04:08:58 PDT
https://trac.webkit.org/r201967
Comment 8 Csaba Osztrogonác 2016-06-11 06:23:10 PDT
(In reply to comment #7)
> https://trac.webkit.org/r201967

It broke the WinCairo build, see build.webkit.org for details. cc-ing port maintainers too
Comment 9 Antti Koivisto 2016-06-11 10:57:51 PDT
I tried fixing WinCairo in https://trac.webkit.org/r201972
Comment 10 Alexey Proskuryakov 2016-06-11 12:27:26 PDT
This change caused assertion failures on IndexedDB tests (not always the same test).

For whatever reason, I don't see this on build.webkit.org bots, only internally. I'm going to roll out, and will I'll e-mail links to the crashes.
Comment 11 WebKit Commit Bot 2016-06-11 12:30:31 PDT
Re-opened since this is blocked by bug 158665
Comment 12 Brady Eidson 2016-06-11 17:41:53 PDT
The assert was no specific to IndexedDB tests but rather was underneath InjectedBundle::setPrivateBrowsingEnabled

IDB tests are the vast majority of setPrivateBrowsingEnabled users, but not the exclusive ones - Other tests do it too.
Comment 13 Antti Koivisto 2016-06-13 02:39:51 PDT
(In reply to comment #12)
> The assert was no specific to IndexedDB tests but rather was underneath
> InjectedBundle::setPrivateBrowsingEnabled
> 
> IDB tests are the vast majority of setPrivateBrowsingEnabled users, but not
> the exclusive ones - Other tests do it too.

setPrivateBrowsingEnabled uses non-unique “legacyPrivateSessionID" which causes the assert. Not sure why public bots don't hit it though. Maybe two setPrivateBrowsingEnabled tests never run in the same process there for some reason.
Comment 14 Antti Koivisto 2016-06-13 07:03:08 PDT
Created attachment 281172 [details]
patch

Different approach, add a SessionId version of the cookie getter function and do the mapping to NetworkStorageSession on WebKit side.
Comment 15 WebKit Commit Bot 2016-06-15 07:10:38 PDT
Comment on attachment 281172 [details]
patch

Clearing flags on attachment: 281172

Committed r202089: <http://trac.webkit.org/changeset/202089>
Comment 16 WebKit Commit Bot 2016-06-15 07:10:43 PDT
All reviewed patches have been landed.  Closing bug.