Bug 168229 - CookieManager only works with the default session
Summary: CookieManager only works with the default session
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 168230
  Show dependency treegraph
 
Reported: 2017-02-13 04:59 PST by Carlos Garcia Campos
Modified: 2017-02-15 11:00 PST (History)
9 users (show)

See Also:


Attachments
Patch (34.88 KB, patch)
2017-02-13 05:07 PST, Carlos Garcia Campos
achristensen: review+
Details | Formatted Diff | Diff
Patch for landing (35.15 KB, patch)
2017-02-14 01:39 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Carlos Garcia Campos 2017-02-13 04:59:42 PST
It should be possible to use the cookie manager also for ephemeral sessions.
Comment 1 Carlos Garcia Campos 2017-02-13 05:07:30 PST
Created attachment 301344 [details]
Patch
Comment 2 WebKit Commit Bot 2017-02-13 05:10:16 PST
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API
Comment 3 WebKit Commit Bot 2017-02-13 05:10:27 PST
Attachment 301344 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CookieStorage.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:38:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:44:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:69:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:31:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:33:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:43:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 4 Michael Catanzaro 2017-02-13 17:02:18 PST
Comment on attachment 301344 [details]
Patch

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

Nice work! I trust you'll find an owner to review it.

Any reason you used std::function instead of the preferred WTF::Function?

> Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46
> +    static NeverDestroyed<HashMap<CFHTTPCookieStorageRef, std::function<void ()>>> map;

You need to fix Windows:

C:\cygwin\home\buildbot\WebKit\Source\WebCore\platform\network\cf\CookieStorageCFNet.cpp(46): error C2079: 'map' uses undefined class 'WTF::NeverDestroyed<WTF::HashMap<CFHTTPCookieStorageRef,std::function<void (void)>,WTF::PtrHash<P *>,WTF::HashTraits<KeyArg>,WTF::HashTraits<MappedArg>>>' [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.vcxproj]
          with
          [
              P=OpaqueCFHTTPCookieStorage,
              KeyArg=CFHTTPCookieStorageRef,
              MappedArg=std::function<void (void)>
          ]

Probably a missing #include?
Comment 5 Alex Christensen 2017-02-13 17:27:38 PST
Comment on attachment 301344 [details]
Patch

I like the direction this code is going in.  Definitely fix the Windows build.  WTF::Functions are not copyable, and that seems preferable in this case.  Can this be tested?
Comment 6 Alex Christensen 2017-02-13 17:29:13 PST
(In reply to comment #5)
> Can this be tested?
Aha!  This is definitely related to https://bugs.webkit.org/show_bug.cgi?id=168230
Comment 7 Alex Christensen 2017-02-13 17:34:36 PST
Comment on attachment 301344 [details]
Patch

r=me, fix Windows.
Let's get rid of CookiesStrategy, too!
Comment 8 Michael Catanzaro 2017-02-13 18:02:05 PST
(In reply to comment #6)
> (In reply to comment #5)
> > Can this be tested?
> Aha!  This is definitely related to
> https://bugs.webkit.org/show_bug.cgi?id=168230

Yeah, it's because we just exposed ephemeral mode. See https://lists.webkit.org/pipermail/webkit-gtk/2017-February/002911.html. Thanks for reviewing it quickly, because we want to freeze API for the next release now!
Comment 9 Carlos Garcia Campos 2017-02-13 23:44:52 PST
(In reply to comment #4)
> Comment on attachment 301344 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=301344&action=review
> 
> Nice work! I trust you'll find an owner to review it.
> 
> Any reason you used std::function instead of the preferred WTF::Function?

Because a non copyable function can't be used as a value of a HashMap.
 
> > Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46
> > +    static NeverDestroyed<HashMap<CFHTTPCookieStorageRef, std::function<void ()>>> map;
> 
> You need to fix Windows:
> 
> C:
> \cygwin\home\buildbot\WebKit\Source\WebCore\platform\network\cf\CookieStorage
> CFNet.cpp(46): error C2079: 'map' uses undefined class
> 'WTF::NeverDestroyed<WTF::HashMap<CFHTTPCookieStorageRef,std::function<void
> (void)>,WTF::PtrHash<P
> *>,WTF::HashTraits<KeyArg>,WTF::HashTraits<MappedArg>>>'
> [C:\cygwin\home\buildbot\WebKit\WebKitBuild\Release\Source\WebCore\WebCore.
> vcxproj]
>           with
>           [
>               P=OpaqueCFHTTPCookieStorage,
>               KeyArg=CFHTTPCookieStorageRef,
>               MappedArg=std::function<void (void)>
>           ]
> 
> Probably a missing #include?

Yes.
Comment 10 Alex Christensen 2017-02-13 23:46:11 PST
(In reply to comment #9)
> > Any reason you used std::function instead of the preferred WTF::Function?
> 
> Because a non copyable function can't be used as a value of a HashMap.
You're right :(
Comment 11 Carlos Garcia Campos 2017-02-13 23:46:31 PST
(In reply to comment #5)
> Comment on attachment 301344 [details]
> Patch
> 
> I like the direction this code is going in.  Definitely fix the Windows
> build.  WTF::Functions are not copyable, and that seems preferable in this
> case.  Can this be tested?

I started using Function but had to change it to std::function to be able to use it in HashMap value.
Comment 12 Carlos Garcia Campos 2017-02-14 01:39:32 PST
Created attachment 301478 [details]
Patch for landing
Comment 13 WebKit Commit Bot 2017-02-14 02:09:33 PST
Attachment 301478 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/CookieStorage.h:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:34:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:38:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:56:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/mac/CookieStorageMac.mm:75:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:68:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:77:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.h:113:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:46:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:48:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/cf/CookieStorageCFNet.cpp:71:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:96:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/UIProcess/WebCookieManagerProxy.cpp:136:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:31:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:33:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebCore/platform/network/soup/CookieStorageSoup.cpp:43:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 16 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 Carlos Garcia Campos 2017-02-14 02:45:02 PST
Committed r212283: <http://trac.webkit.org/changeset/212283>
Comment 15 Brady Eidson 2017-02-14 10:39:14 PST
(In reply to comment #11)
> (In reply to comment #5)
> > Comment on attachment 301344 [details]
> > Patch
> > 
> > I like the direction this code is going in.  Definitely fix the Windows
> > build.  WTF::Functions are not copyable, and that seems preferable in this
> > case.  Can this be tested?
> 
> I started using Function but had to change it to std::function to be able to
> use it in HashMap value.

Nothing about a Function precludes it from being a HashMap value; We use Functions in HashMap values all the time.

What was preventing you from doing so?

Could you please revisit this?
Comment 16 Brady Eidson 2017-02-14 10:48:58 PST
(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #5)
> > > Comment on attachment 301344 [details]
> > > Patch
> > > 
> > > I like the direction this code is going in.  Definitely fix the Windows
> > > build.  WTF::Functions are not copyable, and that seems preferable in this
> > > case.  Can this be tested?
> > 
> > I started using Function but had to change it to std::function to be able to
> > use it in HashMap value.
> 
> Nothing about a Function precludes it from being a HashMap value; We use
> Functions in HashMap values all the time.

For hints, search WebKit2 for:
`HashMap<uint64_t, Function`
Comment 17 Carlos Garcia Campos 2017-02-14 22:29:32 PST
(In reply to comment #15)
> (In reply to comment #11)
> > (In reply to comment #5)
> > > Comment on attachment 301344 [details]
> > > Patch
> > > 
> > > I like the direction this code is going in.  Definitely fix the Windows
> > > build.  WTF::Functions are not copyable, and that seems preferable in this
> > > case.  Can this be tested?
> > 
> > I started using Function but had to change it to std::function to be able to
> > use it in HashMap value.
> 
> Nothing about a Function precludes it from being a HashMap value; We use
> Functions in HashMap values all the time.
> 
> What was preventing you from doing so?
> 
> Could you please revisit this?

You are right, now I remember exactly the issue. You can indeed use Function as a value of a HshMap, but you can't get it, because that always requires a copy, at least with the current HashMap implementation, I'm not sure if it's possible to implement a git without copying.
Comment 18 Carlos Garcia Campos 2017-02-14 22:31:51 PST
(In reply to comment #16)
> (In reply to comment #15)
> > (In reply to comment #11)
> > > (In reply to comment #5)
> > > > Comment on attachment 301344 [details]
> > > > Patch
> > > > 
> > > > I like the direction this code is going in.  Definitely fix the Windows
> > > > build.  WTF::Functions are not copyable, and that seems preferable in this
> > > > case.  Can this be tested?
> > > 
> > > I started using Function but had to change it to std::function to be able to
> > > use it in HashMap value.
> > 
> > Nothing about a Function precludes it from being a HashMap value; We use
> > Functions in HashMap values all the time.
> 
> For hints, search WebKit2 for:
> `HashMap<uint64_t, Function`

I did it when I found the issue, and all those were cases where function is added once and then taken, like completion handlers. In that case there's no problem because take doesn't need to copy, it just moves. Here we need to set once and get multiple times. We could take and set on every get, but I don't think it's worth it.
Comment 19 Michael Catanzaro 2017-02-15 09:50:38 PST
I guess it caused bug #168375.