Bug 158616 – Vary:Cookie validation doesn't work in private browsing

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

We currently reload any resource with Vary:Cookie in private browsing even if we have a memory cache entry.

[Antti Koivisto]

Created attachment 281010 [details]
patch

[Antti Koivisto]

Created attachment 281012 [details]
patch

[Alex Christensen]

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

[Darin Adler]

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&&.

[Antti Koivisto]

Created attachment 281096 [details]
patch

[Radar WebKit Bug Importer]

<rdar://problem/26755067>

[Antti Koivisto]

https://trac.webkit.org/r201967

[Csaba Osztrogonác]

(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

[Antti Koivisto]

I tried fixing WinCairo in https://trac.webkit.org/r201972

[Alexey Proskuryakov]

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.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 158665

[Brady Eidson]

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.

[Antti Koivisto]

(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.

[Antti Koivisto]

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.

[WebKit Commit Bot]

Comment on attachment 281172 [details]
patch

Clearing flags on attachment: 281172

Committed r202089: <http://trac.webkit.org/changeset/202089>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.