Bug 103457 – [WK2] Forward cookie jar calls to NetworkProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

DOM and programmatic cookie access should happen in network process, because this is where loading happens.

[Alexey Proskuryakov]

Created attachment 176353 [details]
proposed patch

[WebKit Review Bot]

Attachment 176353 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebKit2/Shared/WebCoreArgumentCoders.cpp:32:  Alphabetical sorting problem.  [build/include_order] [4]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 176353 [details]
proposed patch

Attachment 176353 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/15010299

[Early Warning System Bot]

Comment on attachment 176353 [details]
proposed patch

Attachment 176353 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15026185

[EFL EWS Bot]

Comment on attachment 176353 [details]
proposed patch

Attachment 176353 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15025163

[Alexey Proskuryakov]

Created attachment 176372 [details]
with build fixes

Added strategy implementations for each WebKit1.

[Early Warning System Bot]

Comment on attachment 176372 [details]
with build fixes

Attachment 176372 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/15026209

[EFL EWS Bot]

Comment on attachment 176372 [details]
with build fixes

Attachment 176372 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15023238

[Build Bot]

Comment on attachment 176372 [details]
with build fixes

Attachment 176372 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15016267

[Build Bot]

Comment on attachment 176372 [details]
with build fixes

Attachment 176372 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/15027268

[Adam Barth]

Comment on attachment 176372 [details]
with build fixes

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

Patches like this make my unhappy.  As a consequence of supporting WebKit2, you've now added an extra layer of indirection for everyone, including ports that don't care to support WebKit2, and added ifdefs to 9 functions.  Give your previous resistance to adding complexity and ifdefs to WebKit to support Chromium, I'm surprised that you find these sorts of patches acceptable.

> Source/WebCore/loader/CookieJar.cpp:61
> +#if USE(PLATFORM_STRATEGIES)
> +    return platformStrategies()->cookiesStrategy()->cookiesForDOM(networkingContext(document), document->firstPartyForCookies(), url);
> +#else

It's unfortunate that you can't put the platformStrategies code in a separate CPP file, for example CookieJarWebKit2.cpp.  I suspect you would be unhappy if we put USE(PLATFORM_SUPPORT) ifdefs in CookieJar.cpp back when Chromium used PlatformSupport.

> Source/WebCore/platform/Cookie.h:38
> +        Cookie() { }

Please initialize scalars in these sorts of constructors.

[Adam Barth]

Comment on attachment 176372 [details]
with build fixes

I think we should find a way of implementing this feature without adding WebKit2-specific ifdefs to cross-port files like CookeJar.

[Adam Barth]

Comment on attachment 176372 [details]
with build fixes

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

> Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:86
> +    return WebCore::cookiesForDOM(context, firstParty, url);

There's how I would restructure this patch to avoid forking CookieJar.cpp:

1) For ports that USE(PLATFORM_STRATEGIES), rename these symbols to not collide with the symbols called by CookieJar.cpp.
2) Add a PlatformStrategiesCookieJar.cpp that implements these symbols in terms of PlatformStrategies.

[Alexey Proskuryakov]

> I suspect you would be unhappy if we put USE(PLATFORM_SUPPORT) ifdefs in CookieJar.cpp back when Chromium used PlatformSupport.

The difference is that WebKit2 is what nearly all WebKit ports use, while Chromium is just one port, and one that has historically standing in the project due to using a lot of unique approaches.

Isolating platform strategies code into separate files makes no sense for the project. I intend to stick to the approach in this patch.

[Adam Barth]

I disagree.  As far as I can tell, there's no reason you need to add ifdefs to this file that is used both by WebKit2 ports and by ports that don't wish to use WebKit2.

[Alexey Proskuryakov]

More files with duplicated code mean more maintenance burden. There is also a significant cognitive burden - we already have PLATFORM_STRATEGIES ifdefs, but now some of these decisions will be hardwired in project files, and some remain as ifdefs.

I do not think that needs of ports that don't use WebKit2 are a priority for the project. If there is a strong desire to avoid ifdefs, I wouldn't object to increased forking for those, if we can also agree that mainline port maintainers are not obligated to keep these building, and agree to remove those ports from EWS.

[Alexey Proskuryakov]

There are many things I dislike about PlatformStrategies, too - the ifdefs are ugly, and if you look at this patch, adding functions to strategies is a huge pain because multiple WebKit1 ports need to be updated.

But I don't think that having a fight about indirection in this particular bug is the most constructive way forward. These calls are not performance sensitive, and PlatformStrategies are our current solution for the need.

[Adam Barth]

> I do not think that needs of ports that don't use WebKit2 are a priority for the project.

If that how you feel, it sounds like we need to discuss this topic on webkit-dev.  I do think that the needs of ports that don't use WebKit2 are a priority for the project.

[Alexey Proskuryakov]

Adam, do you intend to block landing this patch for the meta discussion? You've recently blocked landing of another NetworkProcess patch, and this is becoming difficult.

[Maciej Stachowiak]

(In reply to comment #18)
> > I do not think that needs of ports that don't use WebKit2 are a priority for the project.
> 
> If that how you feel, it sounds like we need to discuss this topic on webkit-dev.  I do think that the needs of ports that don't use WebKit2 are a priority for the project.

Obviously the needs of the Chromium port are a priority for the project, as a substantial number of core contributors use it as their primary port for development. I hope Alexey did not mean to imply otherwise.


-- More broadly:

I feel like in these discussions we should draw a distinction between Chromium on the one hand, and relatively minor single-process-only ports on the other. It seems to me that essentially all non-Chromium ports that are very active (large community of active contributors, have buildbots, consistently actively maintained) have a WebKit2 port. So really saying "ports that don't use WebKit2" basically means "the Chromium port" and "WebKit2-specific changes" means "changes applying to basically all ports but Chromium". I agree that the Chromium concerns are important but I think it clouds the issue to imply that WebKit2-based ports are somehow unusual as opposed to the norm or that Chromium concerns apply to a broad range of critical ports.


-- Looking forward:

I feel like we need to work out a system for doing process indirection that is reasonable for the needs of all ports, Chromium and otherwise. So far I have been frustrated at our inability to make progress on this. There needs to be some offline discussion, most likely in person. The divergence on how to do multiprocess is a source of ongoing friction and we don't seem to be making progress by debating this in individual bugs.

In this specific patch, to me your feedback looks like asking non-Chromium ports to make their code more complex so that Chromium code can be simpler, down to even asking to make a fork of a cross-platform file that all the major non-Chromium ports would use, to avoid adding some ifdefs to the file that Chromium would use. I feel like this does not really minimize cognitive complexity for the project as a whole. I am sure your perspective is different. (And maybe I misunderstood the request.) But understanding each other's perspectives is the first step to finding an approach we can all agree on or at least live with.

I hope we can continue this wider discussion about aligning strategies in other venues.

[Adam Barth]

> In this specific patch, to me your feedback looks like asking non-Chromium ports to make their code more complex so that Chromium code can be simpler, down to even asking to make a fork of a cross-platform file that all the major non-Chromium ports would use, to avoid adding some ifdefs to the file that Chromium would use.

I'm not sure I explained what I had in mind clearly.  Let's take setCookies as an example.  Currently, all ports link in the setCookies implementation in [1].  setCookies then calls setCookiesFromDOM, which is declared in [2] and implemented separately for each platform (e.g., [3], [4], [5]).  This design was introduced just recently by Alexey in [6].

What I'm suggesting is that all ports continue to use the implementation of setCookies in [1], but that ports that wish to USE(PLATFORM_STRATEGIES) supply a PlatformCookieJar.cpp that implements the setCookiesFromDOM function from [2].  That implementation can then call CookiesStrategy::setCookiesFromDOM much in the same way that the cookiesForDOM implementation in [5] calls WebCookieJar::setCookie.

This approach avoids introducing USE(PLATFORM_STRATEGIES) ifdefs.  Instead, it uses the existing PlatformCookieJar.h abstraction to let different ports supply different cookie jar implementations.

[1] http://trac.webkit.org/browser/trunk/Source/WebCore/loader/CookieJar.cpp
[2] http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/PlatformCookieJar.h
[3] http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/mac/CookieJarMac.mm
[4] http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/qt/CookieJarQt.cpp
[5] http://trac.webkit.org/browser/trunk/Source/WebCore/platform/network/chromium/CookieJarChromium.cpp
[6] https://bugs.webkit.org/show_bug.cgi?id=101621

> I feel like this does not really minimize cognitive complexity for the project as a whole. I am sure your perspective is different. (And maybe I misunderstood the request.)

The choice is essentially between a bunch of USE(PLATFORM_STRATEGIES) ifdefs in [1] or a PlatformCookieJar.cpp implementation of PlatformCookieJar.h that ports link in when defining USE(PLATFORM_STRATEGIES).

An even better design would be to find a way to have all ports share the CookieStrategy interface.  In particular, the functions that Alexey is adding to CookieStrategy look very similar to the functions declared in this header:

http://trac.webkit.org/browser/trunk/Source/Platform/chromium/public/WebCookieJar.h

If we could agree on the names, types, and stability of these functions, then we could merge CookieJar.cpp, CookieJarChromium.cpp, and PlatformCookieJar.cpp into a single CookieJar.cpp with no extra cognitive overhead for anyone.  (I tried to sketch out how that might work in https://bugs.webkit.org/show_bug.cgi?id=99483#c16, but I suspect a bugzilla comment isn't a good medium for explaining how that would work.)

> I hope we can continue this wider discussion about aligning strategies in other venues.

That sounds like a good idea.

[Alexey Proskuryakov]

> I agree that the Chromium concerns are important but I think it clouds the issue to imply that WebKit2-based ports are somehow unusual as opposed to the norm or that Chromium concerns apply to a broad range of critical ports.

This precisely matches where I am coming from, the "not a priority" statement I made was not accurate.

[Alexey Proskuryakov]

Created attachment 176600 [details]
more build fixes

I need more time to think about Adam's proposal. I don't fully understand it yet, it seems that we'd get a lot of duplicated code, or perhaps an extra level of indirection for all non-chromium ports.

Please let me try to get a green EWS with this approach first.

> In particular, the functions that Alexey is adding to CookieStrategy look very similar to the functions declared in this header

I think that it is a good idea to encapsulate a storage session in a class, however a CookieJar is probably not the right one. On the Mac, there is no separate cookie storage, it's part of session storage, so a CookieJar class would be quite tricky to align with Mac APIs.

[Adam Barth]

> > In particular, the functions that Alexey is adding to CookieStrategy look very similar to the functions declared in this header
> 
> I think that it is a good idea to encapsulate a storage session in a class, however a CookieJar is probably not the right one. On the Mac, there is no separate cookie storage, it's part of session storage, so a CookieJar class would be quite tricky to align with Mac APIs.

I think you're getting hung up on the name.  If you look at the functions in WebCookieJar, you'll find that they're the same as the ones you're adding to CookieStrategy.  The only difference is that you're passing a NetworkingContext as the first parameter, which is something we could do in a common approach as well.

[EFL EWS Bot]

Comment on attachment 176600 [details]
more build fixes

Attachment 176600 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/15025558

[Alexey Proskuryakov]

Created attachment 176752 [details]
more build fixes

[Alexey Proskuryakov]

I'd like to discuss Adam's concrete proposal for this patch. I don't think that deeper architectural improvements should block landing it. We have a large number of strategy functions already.

> What I'm suggesting is that all ports continue to use the implementation of setCookies in [1], but that 
> ports that wish to USE(PLATFORM_STRATEGIES) supply a PlatformCookieJar.cpp that implements the 
> setCookiesFromDOM function from [2].  That implementation can then call 
> CookiesStrategy::setCookiesFromDOM much in the same way that the cookiesForDOM implementation in 
> [5] calls WebCookieJar::setCookie.
>
> This approach avoids introducing USE(PLATFORM_STRATEGIES) ifdefs.  Instead, it uses the existing 
> PlatformCookieJar.h abstraction to let different ports supply different cookie jar implementations.

So if I understand this suggestion correctly, we'd have a copy of code that calls strategies in CookieJarMac.mm, CookieJarWin.cpp, CookieJarQt.cpp and so on? Furthermore, we'd still have USE(PLATFORM_STRATEGIES) in some of these, because each platform file may be used both by ports that use strategies, and ports that don't. There is no reason to have a copy of Windows API cookie access code in two files, one that uses strategy, and one that doesn't

Furthermore, we still have some uncertainty on whether PlatformStrategies code should live in platform or outside. I want to keep files like CookieJarMac.mm as pure interfaces to underlying platform as possible. It's not entirely the case now (we call out to get default storage session, for instance), but I think that OS API interface should be kept separate from process model logic.

[Alexey Proskuryakov]

Comment on attachment 176752 [details]
more build fixes

I think that having ifdefs in this bottleneck file is a better solution than short term alternatives we have, so putting back under review.

The real tangible burden is having to update all platforms in lockstep, and as evidenced by this patch, strategies don't make it very easy. Engineers working on most ports have to pay this cost, and will inevitably be looking at ways to improve the solution.

I don't think that adding more isolation between platforms - especially at the cost of more duplicated code - is the right direction. It may make reading the code easier initially, but once you try to make a patch, you are suddenly dealing with more complicated code, multiplied.

[Adam Barth]

> So if I understand this suggestion correctly, we'd have a copy of code that calls strategies in CookieJarMac.mm, CookieJarWin.cpp, CookieJarQt.cpp and so on?

No, that is not a correct understanding of the proposal in comment #21.  I'm proposing that you create a new file named PlatformCookieJar.cpp that calls the strategy.  There is no duplicated code in this approach.

> Furthermore, we'd still have USE(PLATFORM_STRATEGIES) in some of these, because each platform file may be used both by ports that use strategies, and ports that don't.

Currently there is a one-to-one correspondence between ports and CookieJarFoo.cpp files, so I don't think this case actually occurs.

> Furthermore, we still have some uncertainty on whether PlatformStrategies code should live in platform or outside. I want to keep files like CookieJarMac.mm as pure interfaces to underlying platform as possible.

My proposal does not involve modifying CookieJarMac.mm (other than perhaps renaming the functions to not collide with the symbols in PlatformCookieJar.cpp).

> I think that having ifdefs in this bottleneck file is a better solution than short term alternatives we have, so putting back under review.

I disagree for the reasons stated above.  Specifically, I don't see any technical reason why we should put USE(PLATFORM_STRATEGIES) ifdefs in CookieJar.cpp.  We can simply put the USE(PLATFORM_STRATEGIES) code in PlatformCookieJar.cpp and have PlatformCookieJar.cpp call through to CookieStrategy the same way that CookieJarChromium.cpp calls through to WebCookieJar.

As a though exercise, consider how you would react if I told you that we should have a USE(PLATFORM_API) ifdef in CookieJar.cpp.  Further imagine that code behind the USE(PLATFORM_API) ifdef did precisely what the code in CookieJarChromium.cpp does today and that I asked you to move the CookieStrategy code to a separate CookieJarWebKit2.cpp file.  I suspect you would question why USE(PLATFORM_API) code ought to live in CookieJar.cpp but the WebKit2-related code had to live in a separate file.

As far as I can tell, the situations are symmetric and ought to be treated as such.

[Alexey Proskuryakov]

> No, that is not a correct understanding of the proposal in comment #21.  I'm proposing that you create a new file named PlatformCookieJar.cpp that calls the strategy.  There is no duplicated code in this approach.

Sorry, I tried to read the proposal this way, but I couldn't see a meaningful way to turn the idea into something workable, so I imagined a different once.

We'd need to change signatures of functions that actually use platform APIs (like the ones in CookieJarMac.mm), so that they don't collide with PlatformCookieJar. We'd need a header for those, say RealUnderlyingPlatformCookieJar.h. So, we'd have CookieJar.h, PlatformCookieJar.h, and RealUnderlyingPlatformCookieJar.h.

And when we are done with this, we'll have a number of confusingly similar functions that don't do much. CookieJar will only unwrap Document into NetworkingContext. PlatformCookieJar will only call a strategy, not even being a sensible part of "platform".

We'll have an additional level of indirection, too.

It will be much more work to me now, as every CookieJarXXX implementation (and corresponding exports) except for chromium one will need to be updated for the new signature.

I see an tremendous amount of downside for not using an ifdef in a file that's already a special purpose bottleneck one.

> As far as I can tell, the situations are symmetric and ought to be treated as such.

This is not how I see it at all. WebKit2 is a process architecture that a lot of ports use. The fact that it's mostly implemented in a separate top level directory and not in WebCore should not make it look like just a port among dozens.

Especially in the context of OS API wrappers like CookieJar, it's distinct from any port in that it only provides marshaling, and ultimately relies on also having a port-specific implementation.

[Adam Barth]

> We'd need to change signatures of functions that actually use platform APIs (like the ones in CookieJarMac.mm), so that they don't collide with PlatformCookieJar. We'd need a header for those, say RealUnderlyingPlatformCookieJar.h. So, we'd have CookieJar.h, PlatformCookieJar.h, and RealUnderlyingPlatformCookieJar.h.

That sounds exactly like what you just did in bug 101621.

> > As far as I can tell, the situations are symmetric and ought to be treated as such.
> 
> This is not how I see it at all. WebKit2 is a process architecture that a lot of ports use. The fact that it's mostly implemented in a separate top level directory and not in WebCore should not make it look like just a port among dozens.

I suspect that's the core of the disagreement.  I don't think the Chromium port should be treated as a second-class citizen.

[Alexey Proskuryakov]

> I don't think the Chromium port should be treated as a second-class citizen.

Chromium is a port though, not an architecture used by most ports.

[Alexey Proskuryakov]

> That sounds exactly like what you just did in bug 101621.

As you know, the plan of record is to merge CookieJar functions into call sites. There is some easy work there (rename the functions at call sites and call networkingContext() in place). Coming up with a syntax that supports strategies while being acceptable to chromium is not that easy, but we'll have to do that sooner or later anyway.

[Darin Adler]

Comment on attachment 176752 [details]
more build fixes

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

> Source/WebCore/platform/Cookie.h:38
> +        Cookie() { }

Should this initialize the double and the bools?

> Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:87
> +String PlatformStrategiesEfl::cookiesForDOM(NetworkingContext* context, const KURL& firstParty, const KURL& url)
> +{
> +    return WebCore::cookiesForDOM(context, firstParty, url);
> +}

Can we do this with a default implementation in the base class rather than repeating this for every platform?

[Alexey Proskuryakov]

Comment on attachment 176752 [details]
more build fixes

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

Thank you for review!

>> Source/WebCore/platform/Cookie.h:38
>> +        Cookie() { }
> 
> Should this initialize the double and the bools?

There is no need for this here, because data members will be filled when decoding a message. I don't like code that does unneeded work much, and default values for these member variables would not necessarily be safe as fallback for code that uses this constructor in the future.

>> Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:87
>> +}
> 
> Can we do this with a default implementation in the base class rather than repeating this for every platform?

Hmm, I should think about this. Going to land as is for now, but I'll be changing signatures of these functions soon, and may do it then.

[WebKit Review Bot]

Comment on attachment 176752 [details]
more build fixes

Clearing flags on attachment: 176752

Committed r136197: <http://trac.webkit.org/changeset/136197>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Maciej Stachowiak]

(In reply to comment #34)
> (From update of attachment 176752 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176752&action=review
> 
> > Source/WebCore/platform/Cookie.h:38
> > +        Cookie() { }
> 
> Should this initialize the double and the bools?
> 
> > Source/WebKit/efl/WebCoreSupport/PlatformStrategiesEfl.cpp:87
> > +String PlatformStrategiesEfl::cookiesForDOM(NetworkingContext* context, const KURL& firstParty, const KURL& url)
> > +{
> > +    return WebCore::cookiesForDOM(context, firstParty, url);
> > +}
> 
> Can we do this with a default implementation in the base class rather than repeating this for every platform?

Whoah, r+ing when another reviewer still had clear objections to the approach (and then landing based on that) is not a cool thing to do. I realize the conversation was getting frustrating for all involved, but this shouldn't be the way we do things in the WebKit project. What's going on here?