(no longer active) Reporter
	Description • 7 years ago

See this profile <https://clptr.io/2jkLOAZ> for example where we spend ~3.5 *seconds* on Msg_GetCookieString sync IPC messages.

Right now we delegate to the parent process to ensure that there is only one source of truth for cookie information, but this is extremely costly.  Perhaps it's time to rethink this decision?

My proposal is to replace these sync IPC messages with async messages from the parent to child letting it know when a cookie changes, and maintain a hashtable of cookies in the child process (similar to the one we maintain in the parent process) and use that to implement the document.cookie getter.

For the document.cookie setter, I suggest that we should update the hashtable immediately (so that |document.cookie = "foo=bar"; document.cookie == "foo=bar"| would hold true) and send an async message to the parent process to let it update the real cookie value.  This update will again asynchronously update the child process' notion of the cookie value.  This will correctly deal with the case where a cookie is set through an HTTP header in the parent, scheduling an async IPC message to update the child, and then the child immediately getting a call to document.cookie setter modifying the same cookie while the said IPC message is in flight.

This effectively means that the child process' notion of the current cookie value will lag behind the parent's.  I think the only case where this matters in practice is the case I mentioned at the end of the last paragraph, and even there the behavior is already racy in the sense that the said HTTP header can be processed either before or after the document.cookie setter being called.

Needinfoing jdm as the cookie peer and Patrick and Jason as Necko peers for feedback on this plan.

	Patrick McManus [:mcmanus]
	Comment 1 • 7 years ago

(In reply to :Ehsan Akhgari from comment #0)
> See this profile <https://clptr.io/2jkLOAZ> for example where we spend ~3.5
> *seconds* on Msg_GetCookieString sync IPC messages.

based on your description I couldn't bear to look at the profile.

absent content setter (which you address), but present an http setter on the document would document.cookie be racy or do we just apply those in the child-table along with OnStart?

	(no longer active) Reporter
	Comment 2 • 7 years ago

(In reply to Patrick McManus [:mcmanus] from comment #1)
> absent content setter (which you address), but present an http setter on the
> document would document.cookie be racy or do we just apply those in the
> child-table along with OnStart?

Yes.  More specifically, consider this:

1. We load doc A which sets cookie foo=bar.
2. We load doc B which sets cookie foo=baz.
3. We read document.cookie from doc A.

At this point, if the Set-Cookie header from step 2 is processed, we return "foo=baz", otherwise we return "foo=bar".  This is an existing race which is present even in non-e10s mode.

With this proposal, the race still exists, except that we'll return "foo=bar" until the async job that updates the child's hashtable is processed (as opposed to the Set-Cookie header in the parent), so the timings change but the fact that the API is racy doesn't.

	Jason Duell
	Comment 3 • 7 years ago

> My proposal is to replace these sync IPC messages with async messages from the
> parent to child letting it know when a cookie changes, and maintain a
> hashtable of cookies in the child process (similar to the one we maintain in
> the parent process) and use that to implement the document.cookie getter.

An alternative architecture could be to keep the cookie hashtable in IPDL shared memory (but that would require that our shmem implementation have shared memory mutexes--I don't know if it does).  This would take up less memory, use less IPDL traffic, and might be a little less racy (thought that seems mostly moot--we've got some innate raciness as described in previous comments).  OTOH it might be harder to program and might have tricky edge cases (startup, shutdown).

I'm trying to think of whether there's a security advantage to be had here. Ehsan's architecture has the bonus that we could only ship a child the cookies "it needs to know about".  I'm not sure in practice how hard it would be for the parent to keep track of that, though (if we keep a list of all the domains that a child has loaded resources from, we'd know all the cookies it ought to be able to see, right?).  I don't know if this is practical, but not allowing a compromised child to read the entire cookie database would be a very nice feature.

	(no longer active) Reporter
	Comment 4 • 7 years ago

(In reply to Jason Duell [:jduell] (needinfo me) from comment #3)
> > My proposal is to replace these sync IPC messages with async messages from the
> > parent to child letting it know when a cookie changes, and maintain a
> > hashtable of cookies in the child process (similar to the one we maintain in
> > the parent process) and use that to implement the document.cookie getter.
> 
> An alternative architecture could be to keep the cookie hashtable in IPDL
> shared memory (but that would require that our shmem implementation have
> shared memory mutexes--I don't know if it does).

We have CrossProcessMutex for that purpose.

But the bigger problem with storing the hashtable in IPDL shared memory is that we don't have an existing way of doing this.  nsTHashtable doesn't support a user provided allocator, and a proper cross-process hashtable would want to deal with memory usage growth and whatnot.

> This would take up less
> memory, use less IPDL traffic, and might be a little less racy (thought that
> seems mostly moot--we've got some innate raciness as described in previous
> comments).  OTOH it might be harder to program and might have tricky edge
> cases (startup, shutdown).
> 
> I'm trying to think of whether there's a security advantage to be had here.
> Ehsan's architecture has the bonus that we could only ship a child the
> cookies "it needs to know about".  I'm not sure in practice how hard it
> would be for the parent to keep track of that, though (if we keep a list of
> all the domains that a child has loaded resources from, we'd know all the
> cookies it ought to be able to see, right?).  I don't know if this is
> practical, but not allowing a compromised child to read the entire cookie
> database would be a very nice feature.

In theory all we need to provide to the child process are the cookies that can be retrieved through document.cookie, which are the ones from the origins of the documents that exist in the child process.  So in theory for example if we had a way to tag a channel as the channel from which a document is going to be loaded, we could look up the cookies belonging to its final channel URI (considering the path) and only send down those...

Having been intrigued by this idea, I looked up the consumers of this API in the content process, and currently the only consumers are document.cookie and the NPNURLVCookie NPAPI.  Unfortunately the NPAPI allows you to retrieve arbitrary cookies by default, it seems.  As part of this, we can restrict it only to cookies that the document can access, which would be a privacy win as well.  Chris, should we check with Adobe about adding this restriction?

On the parent side, we need to keep track of the URIs of the active documents, and when dispatching cookie change notifications to the child, check to see if the base domain and path from the URL match one belonging to our actor.  We can store the base domain + path pair in a hashtable to ensure we don't regress the performance of cookie updates.

I'm really liking this plan the more I think about it.

	(no longer active) Reporter
	Comment 5 • 7 years ago

I went ahead and checked to see what Chrome does for document.cookie.  They also do a sync IPC call to another process.  This gives us an opportunity to be faster than them.  :-)

	Jason Duell
	Comment 6 • 7 years ago

>  if we had a way to tag a channel as the channel from which a document 
> is going to be loaded

Channels with the LOAD_DOCUMENT_URI load flag === top-level document loads.

	Jason Duell
	Comment 7 • 7 years ago

Sounds like we could skip sending http-only cookies to the child (and https-only ones if a domain was only loaded with http://).

	Chris Peterson [:cpeterson]
	Comment 8 • 7 years ago

(In reply to :Ehsan Akhgari from comment #4)
> Having been intrigued by this idea, I looked up the consumers of this API in
> the content process, and currently the only consumers are document.cookie
> and the NPNURLVCookie NPAPI.  Unfortunately the NPAPI allows you to retrieve
> arbitrary cookies by default, it seems.  As part of this, we can restrict it
> only to cookies that the document can access, which would be a privacy win
> as well.  Chris, should we check with Adobe about adding this restriction?

Flash has no ActionScript API to get or set browser cookies, other than making external calls to JavaScript. IIRC, the Flash plugin only uses NPNURLVCookie to get browser cookies that it will add to some HTTP requests it makes "behind the back" of NPN_GetURL and NPN_PostURL. So NPNURLVCookie should return any cookies we would expect to be in an HTTP request, including http-only cookies.

I can ask Adobe for more information. If returning all cookies complicates your async cookie IPC, we might have some flexibility in what you return.

	Josh Matthews [:jdm]
	Comment 9 • 7 years ago

I really like the idea proposed in comment 4! I'm fine with the idea from comment 0, too; I could swear that was the original design of e10s cookies and we ended up moving to the current model to reduce complexity, but I can't find evidence of it in the history of nsCookieService.cpp.

	(no longer active) Reporter
	Comment 10 • 7 years ago

(In reply to Chris Peterson [:cpeterson] from comment #8)
> (In reply to :Ehsan Akhgari from comment #4)
> > Having been intrigued by this idea, I looked up the consumers of this API in
> > the content process, and currently the only consumers are document.cookie
> > and the NPNURLVCookie NPAPI.  Unfortunately the NPAPI allows you to retrieve
> > arbitrary cookies by default, it seems.  As part of this, we can restrict it
> > only to cookies that the document can access, which would be a privacy win
> > as well.  Chris, should we check with Adobe about adding this restriction?
> 
> Flash has no ActionScript API to get or set browser cookies, other than
> making external calls to JavaScript. IIRC, the Flash plugin only uses
> NPNURLVCookie to get browser cookies that it will add to some HTTP requests
> it makes "behind the back" of NPN_GetURL and NPN_PostURL. So NPNURLVCookie
> should return any cookies we would expect to be in an HTTP request,
> including http-only cookies.

Ugh.  :(  And I assume the URL here can be any random URL?

> I can ask Adobe for more information. If returning all cookies complicates
> your async cookie IPC, we might have some flexibility in what you return.

It doesn't complicate it per se, it just means that we would have to provide all of the cookies we know about to the child process, which renders the privacy benefit suggested in comment 3 pointless (because if the child process is allowed to read all cookies for Flash, there's no point in locking down what we for document.cookie.

It would be really helpful if you can find out exactly what URLs they expect to be able to retrieve cookies for, and if they're OK with restricting it for example only to the cookies that the document.cookie getter for the containing document can see.

	Chris Peterson [:cpeterson]
	Comment 11 • 7 years ago

(In reply to :Ehsan Akhgari from comment #10)
> It would be really helpful if you can find out exactly what URLs they expect
> to be able to retrieve cookies for, and if they're OK with restricting it
> for example only to the cookies that the document.cookie getter for the
> containing document can see.

I emailed Adobe about Flash's use of browser cookies.

	(no longer active) Reporter
	Comment 12 • 7 years ago

(In reply to Chris Peterson [:cpeterson] from comment #11)
> (In reply to :Ehsan Akhgari from comment #10)
> > It would be really helpful if you can find out exactly what URLs they expect
> > to be able to retrieve cookies for, and if they're OK with restricting it
> > for example only to the cookies that the document.cookie getter for the
> > containing document can see.
> 
> I emailed Adobe about Flash's use of browser cookies.

Adobe confirmed that they're not using this feature, so I am removing it in bug 1334509 so that we can proceed here with the proposal in comment 4.  \o/

I'm planning to post a more concrete implementation plan here soon.

	:Harald Kirschner :digitarald
	Comment 13 • 7 years ago

LinkedIn saw this come up during page load. They use cookies to read their CSRF token 60+ times while rendering their single page app. LI is also looking at addressing this on their site, as the token doesn't change for 30min.

	(no longer active) Reporter
	Comment 14 • 7 years ago

Here is a rough list of steps that I think we should take to fix this bug:

1. Come up with a way to communicate to Necko in the parent process that this channel load needs to send down cookies to the child process.  We can use a load flag similar to LOAD_DOCUMENT_URI, but we actually only need a subset of such loads.  Let's call the new flag LOAD_DOCUMENT_NEEDS_COOKIES.  We only set it if:
  a) LOAD_DOCUMENT_URI is being set.
  b) the document isn't sandboxed (to match the check in <http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/dom/html/nsHTMLDocument.cpp#1259>.)

2. On the parent process side, before sending the OnStartRequest notification we need to send the matching cookies if needed.  A good place for doing this would be HTTPChannelParent::OnStartRequest().  What we need to do is to check if the load flags include both LOAD_DOCUMENT_URI and LOAD_DOCUMENT_NEEDS_COOKIES, and then look at the final channel URI and send down all of the cookies that match these criteria:
  a) If the cookie's host matches the final channel URI's using DomainMatches(),
  b) If the cookie's secure flag matches whether the final channel URI is https,
  c) If the cookie is not HTTP-only (since bug 1339129 is removing the access to HTTP-only cookies to the content process),
  d) If the cookie's path matches that of the final channel URI using PathMatches(),
  e) If the cookie has not expired yet.

These criteria match the ones used in this loop: <http://searchfox.org/mozilla-central/rev/d3307f19d5dac31d7d36fc206b00b686de82eee4/netwerk/cookie/nsCookieService.cpp#3275>.  This should guarantee that all cookies accessible by document.cookie are sent to the child process.

3. On the child process side, make CookieServiceChild maintain a cookies hash table.  This will probably be in the form of the following data structures:
  a) A hashtable similar to nsCookieService::hostTable.  This is the cookie store that the content process uses to implement CookieServiceChild::GetCookieString().  Note that nsCookieService uses a separate hashtable for storing cookies for private browsing windows, so I suggest we do the same here as well.  Everywhere that I talk about accessing any of these data structures, you should access the correct one depending on the privacy status of the document.
  b) A list of all of the document URIs that the child process knows about.  I think this can be a map of the host name to a struct containing an array of tuples of full URI path names, isSecure flags and refcounts.  Let's call this map DocumentCookieMap.

When receiving a cookie from step 2 above, do the following:
  a) Add the cookies to the hostTable maintained in the child process side.
  b) Add an entry to the map in (b) above for the host name from the document URI if one does not exist.
  c) Search the list of path names, isSecure flag and refcount tuple.  If one exists for the document URI, increment the refcount. Otherwise insert an entry with a refcount of 1.  Let's call this pair DocumentCookieInfo, and the list DocumentCookieInfoList.  Note that the isSecure flag is useful for checking secure-only cookies.

4. To maintain these data structures when a document is destroyed, in ~Document() notify the CookieServiceChild object that the document is going away.  When that happens, search DocumentCookieInfoList and decrement the refcount if it's greater than 1, or delete the entry from the list otherwise.  When deleting the entry, iterate over the cookies in hostTable matching our domain and delete the ones that aren't accessible to any of the existing paths.  When removing the last entry from this list, delete all cookies for the domain and remove the hostTable entry.

Be careful that the document URI might have changed since step 3 if the page uses things such as history.pushState(), so you should look at mOriginalURI as well.

5. The parent process also needs to know which content processes care about which cookies to be able to notify the right process when such a cookie changes.  The information that the parent process needs to know is actually the same as what DocumentCookieMap keeps track of, so we can use that data structure on the parent side as well.  Each CookieServiceParent object needs to have one of these maps.  Every time we send cookies in step 2, we need to also update this data structure in the parent side similar to the child side.  When a document goes away in step 4, we need to send an asynchronous notification to the parent process to update the DocumentCookieMap there similarly as well.

6. When a cookie changes on the parent process side, we need to notify the child.  For this purpose, I suggest handling the "cookie-changed" and "private-cookie-changed" notifications in CookieServiceParent, and for each cookie consult our DocumentCookieMap to see whether the cookie change can be visible to the child process, and in that case notify the child process about the cookie change asynchronously.  Note that due to the delay between things such as the child process' Document destructor notifying the parent process, on the child side you should always be prepared to deal with cookie change notifications that can arrive without matching anything in our DocumentCookieMap.  Such notifications should be ignored.  Also note that before sending the notifications you should again apply the criteria in step 2 (for example, you should never send any notification about HTTP-only cookies.)

This should give us a fully working document.cookie getter in the new world.

7. The current setter is asynchronous already so there isn't much to be done for it.  However we must ensure that a cookie set via the setter is immediately readable from the getter before all of the asynchronous IPC is finished.  So we need to parse the string and update our hostTable on the child process side.

Since this is a complicated change that can potentially break things, it would be nice to hide it all behind a new preference.  The preference should be able to revert us to the existing code path.

Amy, I was thinking perhaps you may be interested to take on this project?  This is a super nice performance win that we're tracking for Quantum Flow.

Please let me know if the plan above makes sense and if you have questions.  I have tried to note all of the complexities that you may want to watch out for, so I hope this will be relatively straightforward...  Thanks!

	Amy Chung [:Amy] Assignee
	Comment 15 • 7 years ago

Hi Ehsan,
I will take a look at this bug and spend some time studying your suggestions.

Thanks again for your help!

	Jason Duell
	Comment 16 • 7 years ago

The plan in comment 14 looks good to me.  One nit: we're almost out of channel.loadFlag bits, so we should avoid using one.  I think it should be easy enough to add some new channel.sendE10sCookies() method or whatever.  Anything besides a loadFlag.

	(no longer active) Reporter
	Comment 17 • 7 years ago

(In reply to Jason Duell [:jduell] (needinfo me) from comment #16)
> The plan in comment 14 looks good to me.  One nit: we're almost out of
> channel.loadFlag bits, so we should avoid using one.  I think it should be
> easy enough to add some new channel.sendE10sCookies() method or whatever. 
> Anything besides a loadFlag.

Hmm since this is restricted to HTTP channels, how about a boolean attribute on nsIHttpChannel that is backed by a bool in HttpChannelBase?

	Jason Duell
	Comment 18 • 7 years ago

That sounds fine to me.

	Steve Fink [:sfink] [:s:]
	Comment 20 • 7 years ago

(In reply to :Ehsan Akhgari from comment #0)
> See this profile <https://clptr.io/2jkLOAZ> for example where we spend ~3.5
> *seconds* on Msg_GetCookieString sync IPC messages.

For the record, that profile shows 2.5 seconds on Msg_GetCookieString (not saying that's ok!), and 1 second on Msg_GetGraphicsFeatureStatus for setting up WebGL. Is there a bug for the latter?

	Andrew Overholt [:overholt]
	Comment 21 • 7 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #20)
> (In reply to :Ehsan Akhgari from comment #0)
> > See this profile <https://clptr.io/2jkLOAZ> for example where we spend ~3.5
> > *seconds* on Msg_GetCookieString sync IPC messages.
> 
> For the record, that profile shows 2.5 seconds on Msg_GetCookieString (not
> saying that's ok!), and 1 second on Msg_GetGraphicsFeatureStatus for setting
> up WebGL. Is there a bug for the latter?

A quick bugzilla search turns nothing up. Ehsan, maybe I didn't search for the right thing?

	(no longer active) Reporter
	Comment 22 • 7 years ago

(In reply to Steve Fink [:sfink] [:s:] from comment #20)
> (In reply to :Ehsan Akhgari from comment #0)
> > See this profile <https://clptr.io/2jkLOAZ> for example where we spend ~3.5
> > *seconds* on Msg_GetCookieString sync IPC messages.
> 
> For the record, that profile shows 2.5 seconds on Msg_GetCookieString (not
> saying that's ok!), and 1 second on Msg_GetGraphicsFeatureStatus for setting
> up WebGL. Is there a bug for the latter?

I have fixed that in bug 1331676.  (I usually try to file one bug per issue, so I end up filing several bugs for a single bug often times.)

	Amy Chung [:Amy] Assignee
	Comment 23 • 7 years ago

Hi Ehsan,
I have some questions from your suggestions as below:
1. What’s the timing to set the LOAD_DOCUMENT_NEEDS_COOKIES?
2. Do the matching cookies from nsHttpRequestHead? If the matching cookies exist, I have to set the load flag LOAD_DOCUMENT_NEEDS_COOKIES?

And I organized the steps of implementation as below:
1. Create LOAD_DOCUMENT_NEEDS_COOKIES In nsIChannel.idl.
2. Set LOAD_DOCUMENT_NEEDS_COOKIES to load flags  when getting the matching cookies nsHttpRequestHead 
   If load flags include LOAD_DOCUMENT_NEEDS_COOKIES & LOAD_DOCUMENT_URI, have to send the matching cookie to child process and init hash talbe in CookieServiceParent.cpp.
   a. Create SetMatchingCookies() in PHttpChannel.ipdl.
   b. Create RecvSetMatchingCookies() in HttpChannelChild.cpp.
3. Init the hash table in CookieServiceChild.cpp.
4. Create the update, modify, destroy hash table function  in CookieServiceChild.cpp
5. Same as CookieServiceParent.cpp.

Would you help me to answer my question and confirm my steps of implementation?

	(no longer active) Reporter
	Comment 24 • 7 years ago

(In reply to Amy Chung [:Amy] from comment #23)
> Hi Ehsan,
> I have some questions from your suggestions as below:
> 1. What’s the timing to set the LOAD_DOCUMENT_NEEDS_COOKIES?

If you mean when you should set it, I think we need it in places where we current set LOAD_DOCUMENT_URI for loading documents, which are:

<http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/docshell/base/nsDocShell.cpp#9233>
<http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/docshell/base/nsDocShell.cpp#11425>
<http://searchfox.org/mozilla-central/rev/b1044cf7c2000c3e75e8181e893236a940c8b6d2/dom/html/nsHTMLDocument.cpp#2346>

> 2. Do the matching cookies from nsHttpRequestHead? If the matching cookies
> exist, I have to set the load flag LOAD_DOCUMENT_NEEDS_COOKIES?

No, the idea here is that we want to set LOAD_DOCUMENT_NEEDS_COOKIES on the channels which should trigger Necko to send the cookies it has stored for the host for.  This means that we don't change how the Set-Cookie header is processed, just when we're about to send the OnStartRequest notification, we check to see if the LOAD_DOCUMENT_NEEDS_COOKIES load flag has been set, and if yes, we figure out which cookies the document can possibly see and send them to the content process before OnStartRequest is sent, so in the content process side as soon as we receive an OnStartRequest for a document that is being loaded, we should have its cookies cached and ready to use.

> And I organized the steps of implementation as below:
> 1. Create LOAD_DOCUMENT_NEEDS_COOKIES In nsIChannel.idl.
> 2. Set LOAD_DOCUMENT_NEEDS_COOKIES to load flags  when getting the matching
> cookies nsHttpRequestHead 

This part isn't accurate, please see my explanation above.

>    If load flags include LOAD_DOCUMENT_NEEDS_COOKIES & LOAD_DOCUMENT_URI,
> have to send the matching cookie to child process and init hash talbe in
> CookieServiceParent.cpp.
>    a. Create SetMatchingCookies() in PHttpChannel.ipdl.
>    b. Create RecvSetMatchingCookies() in HttpChannelChild.cpp.
> 3. Init the hash table in CookieServiceChild.cpp.
> 4. Create the update, modify, destroy hash table function  in
> CookieServiceChild.cpp
> 5. Same as CookieServiceParent.cpp.
> 
> Would you help me to answer my question and confirm my steps of
> implementation?

The rest of the steps sound good to me.  Thank you!

	Amy Chung [:Amy] Assignee
	Comment 25 • 7 years ago

Hi Ehsan,
I have some questions from your suggestions as below:
1. Does the method to figure out which cookies the document can possibly see as below:
   i.   Call GetCookie(URI, chan, getter_Copies(cookie)) and confirm the string cookie doesn't null.
   ii.  Send the cookie to HttpChannelChild.cpp by SendSetMatchingCookies().
   iii. Update the cookie to hash table by CookieServiceChild::UpdateDocumentHashTable().
2. The data structure of hash table as below:
   i.   Create struct DocumentCookieMap() in nsCookieService.cpp
        a. full URI path names, isSecure flags and refcounts
   ii.  Create a class DocumentCookieKey and DocumentCookieInfo() in nsCookieService.cpp
        a. Create DocumentCookieMap() in DocumentCookieKey. 
   iii. Create a hashtable in CookieServiceChild.cpp and CookieServerParent.cpp
        a. nsTHashTable<DocumentCookieInfo> DocumentHashTable;
 
Would you help me to answer my questions?

	(no longer active) Reporter
	Comment 26 • 7 years ago

Hi Amy,

(In reply to Amy Chung [:Amy] from comment #25)
> Hi Ehsan,
> I have some questions from your suggestions as below:
> 1. Does the method to figure out which cookies the document can possibly see
> as below:
>    i.   Call GetCookie(URI, chan, getter_Copies(cookie)) and confirm the
> string cookie doesn't null.
>    ii.  Send the cookie to HttpChannelChild.cpp by SendSetMatchingCookies().

Yes, so far I agree.  This is step 2 of comment 14.

>    iii. Update the cookie to hash table by
> CookieServiceChild::UpdateDocumentHashTable().

There's more to do here inisde RcvSetMatchingCookies, according to step 3 of comment 14, in addition to the above, you should do (b) and (c) under the "When receiving a cookie from..." section.

> 2. The data structure of hash table as below:
>    i.   Create struct DocumentCookieMap() in nsCookieService.cpp
>         a. full URI path names, isSecure flags and refcounts

Note that this is a hashmap where the key is the host name and the value is an array of tuples of (fullURIPathName, isSecure, refCount).  Using real class names, this will probably be nsClassHashtable<nsCStringHashKey, DocumentCookieInfoList>.  The reason is that you when a cookie changes for example, you want to quickly look up all of the tuples corresponding to the cookies for that host.  Also, this data structure needs to live on CookieServiceParent, because it tracks the list of the cookies for each content process, so each actor in the parent process needs to have its own map.

>    ii.  Create a class DocumentCookieKey and DocumentCookieInfo() in
> nsCookieService.cpp
>         a. Create DocumentCookieMap() in DocumentCookieKey. 

See above.

>    iii. Create a hashtable in CookieServiceChild.cpp and
> CookieServerParent.cpp
>         a. nsTHashTable<DocumentCookieInfo> DocumentHashTable;

Perhaps we are using different names for different things.  This is basically the data structures I had in my mind in comment 14.  It's easier to talk in terms of code.  :-)

struct DocumentCookieInfo {
  nsCString mPathName;
  bool mIsSecure;
  nsrefcnt mRefCnt; // (perhaps we should use a different name? since this is the name we use for XPCOM reference counting…)
};

typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;

typedef nsClassHashtable<nsCStringHashKey, DocumentCookieInfoList> DocumentCookieMap;

class CookieServiceParent… {
  // cookies that have been sent to the content process for this actor
  DocumentCookieMap mContentProcessCookies;
};

class CookieServiceChild… {
  // cookies that our parent process has told us about
  DocumentCookieMap mKnownCookies;
};

Hope this helps!

	(no longer active) Reporter
	Comment 27 • 7 years ago

(BTW the code above is a sample, it probably has mistakes and typos. :-)

	Nika Layzell [:nika] (ni? for response)
	Comment 28 • 7 years ago

Hey,

I'm working on a similar project for the permission manager in bug 1337056. In that bug I ran into the problem that documents which are loaded within <object> tags don't have the LOAD_DOCUMENT_URI flag set on them when they are being loaded, as whether or not they are a document depends on the MIME type of the response.

I work around this in parts 7/8/9 of bug 1337056. I imagine that once that lands you'll be able to use code very similar to what I wrote in part 9 for this bug as well.

Hopefully that's useful, and helps you not run into this problem as well :).

	Amy Chung [:Amy] Assignee
	Comment 29 • 7 years ago

(In reply to Michael Layzell [:mystor] from comment #28)
> Hey,
> 
> I'm working on a similar project for the permission manager in bug 1337056.
> In that bug I ran into the problem that documents which are loaded within
> <object> tags don't have the LOAD_DOCUMENT_URI flag set on them when they
> are being loaded, as whether or not they are a document depends on the MIME
> type of the response.
> 
> I work around this in parts 7/8/9 of bug 1337056. I imagine that once that
> lands you'll be able to use code very similar to what I wrote in part 9 for
> this bug as well.
> 
> Hopefully that's useful, and helps you not run into this problem as well :).

Hi Michael,
Thanks for your suggestions, that's really helpful to me.

	Amy Chung [:Amy] Assignee
	Comment 31 • 7 years ago

Hi Ehsan,
I have finished part of the function in this bug as below:
1. Created a new loadflag "LOAD_DOCUMENT_NEEDS_COOKIES".
2. Also set the "LOAD_DOCUMENT_NEEDS_COOKIES" when loadflag set to "LOAD_DOCUMENT_URI".
3. Defined the structure "DocumentCookieInfo", "DocumentCookieInfoList" and "DocuemtnCookieMap"
4. Confirmed the cookies which have to set to child process.
   i. Finished the ipc function "SetMatchingCookies()"
   ii. Created some functions to confirm which cookie have to set on nsCookiseService.cpp
5. Finished the update hash table function on child process and parent process.

[Question] 
1. In update hash table function, mRefDocCnt have to increase the length of documentCookieInfoList or once?
2. Would I create the variable "DocumentCookieMap mContentProcessCookies" in nsCookieService.cpp and not in CookieServiceParent.cpp? 

[Not finish]
1. Destroy function
   i.  Create it on nsICookieService.idl.
   ii. Implement it on nsCookieService.cpp and CookieServiceChild.cpp.
   iii.Call it on ~nsDocShell() and ~nsHTMLDocument().

2. Modify function
   i.  Create it on nsICookieService.idl.
   ii. Implement it on nsCookieService.cpp and CookieServiceChild.cpp.
   iii. Call it on nsCookieService::NotifyChanged().

Would you give me some suggestions?
Thanks!

	(no longer active) Reporter
	Comment 32 • 7 years ago

Comment on attachment 8847262 [details] [diff] [review]
implementation

Thanks so much, Amy!  I have asked Josh to provide feedback instead, since I have proposed the implementation plan, I'd like to get another pair of eyes looking over the work being done here.  :-)

	Amy Chung [:Amy] Assignee
	Comment 33 • 7 years ago

Hi Josh,
I have finished all functions as below:
[Created a new loadflag and new ipc set function]
1. Added a new loadflag LOAD_DOCUMENT_NEEDS_COOKIES on nsIChannel.idl.
2. Also set the "LOAD_DOCUMENT_NEEDS_COOKIES" when loadflag set to "LOAD_DOCUMENT_URI".
3. Created ipc function "setMatchingCookies".
4. When loadFlags include "LOAD_DOCUMENT_URI" and "LOAD_DOCUMENT_NEEDS_COOKIES", confirmed the cookies which got before the HttpChannelParent processed onStartRequest 
5. If the cookies meet the conditions from nsICookieService->GetCookieString(), call "SendSetMatchingCookie" to child process.
   And the conditions as below:
    a) If the cookie's host matches the final channel URI's using DomainMatches(),
   b) If the cookie's secure flag matches whether the final channel URI is https,
   c) If the cookie is not HTTP-only (since bug 1339129 is removing the access to HTTP-only cookies to the content process),
   d) If the cookie's path matches that of the final channel URI using PathMatches(),
   e) If the cookie has not expired yet.
   
[Created hash table]
1. Created struct DocumentCookieInfo , DocumentCookieInfoList and DocumentCookieMap.
2. Created hash table (DocumentCookieMap) on CookieServiceChild and nsCookieService.
3. Created the functions "updateCookieToTable" and "destrouCookieFromTable" which can process hash table on nsICookieService.idl.
4. Called nsICookieService->UpdateCookieToTable() when the situations as below:
   a) HttpChannelParent sent the matching cookies to HttpChannelChild.
   b) When new cookie set to db or the existing cookie be modified.
      i.  Called nsICookieService->UpdateCookieToTable() before called "NotifyChanged" on nsCookieService (Confirm !IsNeckoChild()).
      ii. Called nsICookieService->UpdateCookieToTable() when CookieServiceChild::Observe() got the new topics "cookie-doc-changed" and "private-doc-cookie-changed".
5. Called nsICookieService->DestroyCookieFromTable when called destructors ~docShell() and ~nsHTMLDocument().

[Question] 
1. In update hash table function, mRefDocCnt have to increase the length of documentCookieInfoList or once?
2. Would I create the variable "DocumentCookieMap mContentProcessCookies" in nsCookieService.cpp and not in CookieServiceParent.cpp (using IsNeckoChild() to distinguish between child process and parent process) ? 

Would you give me some suggestions, thanks!

	Amy Chung [:Amy] Assignee
	Comment 34 • 7 years ago

Hi Josh,
I tried to test the website: https://cswithandroid.withgoogle.com/course on gecko profiler, and confirmed the request header which included the cookie and the domain is "ssl.gstatic.com".
[Not imported the patch]
https://perf-html.io/public/ecff0f7083a45c5ee4dd6eeb2bab942387681264/calltree/?search=msg_get&thread=2

[imported the patch]
https://perf-html.io/public/6f29d65e6f28642a050aca8a82b7713c03114491/calltree/?search=msg_getCoo&thread=2

Would you gibe me some suggestions on the testing result?

Thanks!

	Josh Matthews [:jdm]
	Comment 35 • 7 years ago

The first profile does not appear to show any significant usage of document.cookie. Is that an unexpected result for that site?

	Markus Stange [:mstange]
	Comment 36 • 7 years ago

(In reply to Amy Chung [:Amy] from comment #34)
> [imported the patch]
> https://perf-html.io/public/6f29d65e6f28642a050aca8a82b7713c03114491/
> calltree/?search=msg_getCoo&thread=2

This search is a little misleading because it finds the string msg_getCoo in the URL of a perf.html tab that was open while you took the profile. E.g. it includes callstacks that contain the frame "PresShell::Paint (https://perf-html.io/public/620068628011d815fa9ceb5348afedf23484dadf/calltree/?search=Msg_GetCookie&thread=2)".
If you search for msg_getCookieString instead then you'll see zero samples in the first content process and a few samples in the second content process: https://perfht.ml/2ne9sDf

(In reply to Josh Matthews [:jdm] from comment #35)
> The first profile does not appear to show any significant usage of
> document.cookie. Is that an unexpected result for that site?

The first profile spends a total of 943ms waiting for document.cookie, out of a total run of about one minute. That seems rather significant to me.

	Josh Matthews [:jdm]
	Comment 37 • 7 years ago

Ah, I got confused by the time spent in PresShell::Paint and didn't read the profile correctly.

Amy, the patch doesn't appear to change the behaviour of CookieServiceChild::GetCookieStringInternal at all, which means that the document.cookie will still perform a sync IPC operation. I expect the profiles to look very similar until this is changed.

	Josh Matthews [:jdm]
	Comment 38 • 7 years ago

Comment on attachment 8849038 [details] [diff] [review]
implementation

Review of attachment 8849038 [details] [diff] [review]:
-----------------------------------------------------------------

I've requested enough changes that require significant design changes that I'm not going to keep reviewing the rest of the patch. It's not clear to me how much of the current changes are actually being executed right now, so I think it's worth fixing that before I review the next version. I think the big improvements will be:
* rewriting CookieServiceChild::GetCookieStringInternal and CookieServiceChild::SetCookieStringInternal to interact with a hashtable stored in CookeiServiceChild
* sending a parsed structure over IPDL, rather than individual cookie strings
* receiving IPC notifications from the parent about changes to the cookie database

::: docshell/base/nsDocShell.cpp
@@ +9222,5 @@
>  
>      // Mark the channel as being a document URI...
>      aOpenedChannel->GetLoadFlags(&loadFlags);
> +    loadFlags |= nsIChannel::LOAD_DOCUMENT_URI |
> +                 nsIChannel::LOAD_DOCUMENT_NEEDS_COOKIES;

We will need to check if the document is sandboxed here.

@@ +11391,5 @@
>    nsLoadFlags loadFlags = 0;
>    (void)aChannel->GetLoadFlags(&loadFlags);
>    loadFlags |= nsIChannel::LOAD_DOCUMENT_URI |
>                 nsIChannel::LOAD_CALL_CONTENT_SNIFFERS;
> +//  loadFlags |= nsIChannel::LOAD_DOCUMENT_NEEDS_COOKIES;

What's going on here?

::: dom/html/nsHTMLDocument.cpp
@@ +2347,5 @@
>  
>      nsLoadFlags loadFlags = 0;
>      channel->GetLoadFlags(&loadFlags);
> +    loadFlags |= nsIChannel::LOAD_DOCUMENT_URI |
> +                 nsIChannel::LOAD_DOCUMENT_NEEDS_COOKIES;

We will need to check if the document is sandboxed here.

::: netwerk/base/nsIChannel.idl
@@ +285,5 @@
>       * Set to force bypass of any service worker interception of the channel.
>       */
>      const unsigned long LOAD_BYPASS_SERVICE_WORKER = 1 << 25;
>  
> +    const unsigned long LOAD_DOCUMENT_NEEDS_COOKIES = 1 << 26;

We will want documentation for this new flag.

@@ +287,5 @@
>      const unsigned long LOAD_BYPASS_SERVICE_WORKER = 1 << 25;
>  
> +    const unsigned long LOAD_DOCUMENT_NEEDS_COOKIES = 1 << 26;
> +
> +    // nsICachingChannel load flags begin at bit 27.

This change needs to be made in nsICachingChannel.idl, but those flags go up to 31 right now. We may have a problem here.

::: netwerk/cookie/CookieServiceChild.cpp
@@ +68,5 @@
>    }
> +
> +  nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +  os->AddObserver(this, "cookie-doc-changed", true);
> +  os->AddObserver(this, "private-doc-cookie-changed", true);

I don't believe there is any code that emits these notifications in the content process in this patch. We will need IPC notifications on PCookieService.ipdl instead of observer service notifications.

::: netwerk/cookie/CookieServiceParent.cpp
@@ -63,3 @@
>  namespace mozilla {
>  namespace net {
> -

nit: revert this change.

::: netwerk/cookie/PCookieService.ipdl
@@ -24,5 @@
>   *
>   * @see nsICookieService
>   * @see nsICookiePermission
>   */
> -

nit: revert this change.

::: netwerk/cookie/nsCookieService.cpp
@@ +1973,4 @@
>    return NS_OK;
>  }
>  
> +

nit: revert this change.

@@ +2344,5 @@
> +            "private-doc-cookie-changed" : "cookie-doc-changed";
> +  } else {
> +    topic = mDBState == mPrivateDBState ?
> +            "private-cookie-changed" : "cookie-changed";
> +  }

I don't believe we want to send different notifications in certain circumstances. If I understand correctly, the goal of this change is to implement step 6, but these notifications are only observed in the parent process. Step 6 suggests adding cookie-changed/private-cookie-changed observers to CookieServiceParent; we can use that to send an IPC message instead.

@@ +3485,5 @@
>  
>    // create a stack-based nsCookieAttributes, to store all the
>    // attributes parsed from the cookie
>    nsCookieAttributes cookieAttributes;
> +  nsCookieAttributes cookieAttributes2;

Unused?

@@ +3848,5 @@
> +    UpdateCookieToTable(aHostURI, aChannel);
> +    NotifyChanged(aCookie, foundCookie ? u"changed" : u"added", true);
> +    return;
> +  }
> +  NotifyChanged(aCookie, foundCookie ? u"changed" : u"added", false);

I don't understand why these changes are necessary. Updating the list of documents that can know about cookies is only important when there is a channel performing a request. In cases where the cookie store is being updated, we don't need to update the list of documents at all, only query it to decide whether to notify the child process or not.

@@ +3984,5 @@
> +}
> +
> +
> +void
> +nsCookieService::GetMatchingCookies(const char          *aCookieHeader,

Instead of parsing the cookie header, we should make an API that retrieves the cookies that match directly from the cookie service's database.

::: netwerk/cookie/nsICookie.idl
@@ +94,5 @@
> +    [noscript, nostdcall, binaryname(GetOriginAttributes)]
> +    OriginAttributes binaryGetOriginAttributes();
> +
> +    [noscript, nostdcall, binaryname(SetOriginAttributes)]
> +    void binarySetOriginAttributes(in const_OriginAttributesRef aOriginAttrs);

Instead of making these changes, I would recommend casting from nsICookie* to nsCookie*.

::: netwerk/cookie/nsICookieService.idl
@@ +18,5 @@
> +    nsrefcnt mRefDocCnt;
> +  };
> +
> +  typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;
> +  typedef nsClassHashtable<nsCStringHashKey, DocumentCookieInfoList> DocumentCookieMap;

Why do these need to be declared in the IDL file?

@@ +205,5 @@
>    void setCookieStringFromHttp(in nsIURI aURI, in nsIURI aFirstURI, in nsIPrompt aPrompt, in string aCookie, in string aServerTime, in nsIChannel aChannel);
> +
> +  void updateCookieToTable(in nsIURI aHostURI, in nsIChannel aChannel);
> +
> +  void destroyCookiesFromTable(in nsIURI aHostrURI, in nsIChannel aChannel);

I think it would make more sense to cast the nsICookieService pointers into CookieServiceParent/CookieServiceChild and call methods on that, rather than exposing these through IDL.

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +61,5 @@
>  #include "nsIXULRuntime.h"
>  #include "nsICacheInfoChannel.h"
>  #include "nsIDOMWindowUtils.h"
>  #include "nsIThrottlingService.h"
> +#include "nsCookieService.h"

Why is this necessary?

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +369,5 @@
>    nsCString mAltDataType;
>  };
>  
>  mozilla::ipc::IPCResult
> +HttpChannelChild::RecvSetMatchingCookies(nsTArray<nsCString>&& cookiesList)

Rather than sending strings that need to be parsed, we should send a structure that can be used to call nsCookie::Create without any parsing.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1128,5 @@
>               "HttpChannelParent getting OnStartRequest from a different nsHttpChannel instance");
>  
> +  nsLoadFlags loadFlags;
> +  chan->GetLoadFlags(&loadFlags);
> +  if (loadFlags & nsIChannel::LOAD_DOCUMENT_NEEDS_COOKIES &&

We should only need to check LOAD_DOCUMENT_NEEDS_COOKIES here.

::: netwerk/protocol/http/PHttpChannel.ipdl
@@ +171,5 @@
>  
>    // Tell the child information of matched URL againts SafeBrowsing list
>    async SetClassifierMatchedInfo(ClassifierInfo info);
>  
> +  async SetMatchingCookies(nsCString[] cookiesList);

Add documentation like `// Provide the child with the list of existing cookies that can be accessed by a document loaded via this channel.`

@@ +181,5 @@
>    // Send__delete__() and complete the steps required to finish the redirect.
>    async FinishInterceptedRedirect();
>  
>    async SetPriority(int16_t priority);
> +

nit: revert this change.

::: netwerk/protocol/http/moz.build
@@ +126,5 @@
>  ]
> +
> +if CONFIG['NECKO_COOKIES']:
> +    LOCAL_INCLUDES += [
> +        '../../cookie',

Let's use /netwerk/cookie instead.

	Amy Chung [:Amy] Assignee
	Comment 39 • 7 years ago

Hi Josh,
My modifications as below:
1. If load flags include LOAD_DOCUMENT_URI && LOAD_DOCUMENT_NEEDS_COOKIES, call CookieServiceChild::GetCookieStringFromDocumentCookieMap().
2. Added mCookieValue and mCookieNmae on DocumentCookieInfo.
3. Modified UpdateCookieToTable().

Would you help me to review my patch?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 40 • 7 years ago

Comment on attachment 8849675 [details] [diff] [review]
implementation

Hi Josh,
Sorry for I didn't get the information of f-, I will modify my patch from your suggestions.
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 41 • 7 years ago

Hi Josh,
I have finished to modify the patch from your suggestions as below:
[Load Flags]
1. Created a load flags LOAD_DOCUMENT_NEEDS_COOKIE on nsIRequest.idl.
2. Added SandBoxed when set the loadFlags to nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE.

[rewrote CookieServiceChild::SetCookieString & CookieServiceChild::GetCookieString]
1. Added some field on 
2. Modified SetCookieString & GetCookieString to interact with a hashtable stored in CookeiServiceChild

[Sent a parsed structure over IPDL]
1. Created a function SetUpdateCookieToTable on PCookieService.ipdl.
2. Let CookieServiceParent to inherit the nsICookieService.
3. CookieServiceParent called SendSetUpdateCookieToTable on CookieServiceParent::UpdateCookieToTable.
4. When nsCookieService found the cookie have to set or modify, called mCookiServiceParent->UpdateCookieToTable.
5. Created CookieServiceParent::GetSingleton().

[Others]
1. Modified the function argument from string list to string.

[Not finish]
1. Have to confirm the function nsCookieService::ConfirmMatchingCookie().
2. network/protocol/http/moz.build.

Would you give me some suggestion on my patch?

Thanks!

	Amy Chung [:Amy] Assignee
	Comment 42 • 7 years ago

Hi Josh,
I found the problem about my implementation when Parent have to notify Child to update hashtable.
And the attachment is my new implementation for fixing the above problem:
[Implementation]
1. Call AddObserver to get notifies "cookie-changed" and "private-cookie-changed" on CookieServiceParent.
2. When get the notify, CookieServiceParent have to confirm the aTopic & mDocNeedsCookie(Boolean flag to confirm the one of load flags which includes nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE) then update hash table which in CookieServiceParent.
3. CookieServiceParent use ipc to notify child which have to update the DocumentCookieMap after CookieServiceParent updated the DocumentCookieMap.

[Data Structure]
1. Created attribute documentNeedsCookie on nsICookieService for confirming the one of LoadFlags which includes "nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE" on the channel.

	Josh Matthews [:jdm]
	Comment 43 • 7 years ago

Comment on attachment 8852874 [details] [diff] [review]
bug-1331680-tmp.patch

Review of attachment 8852874 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/CookieServiceParent.cpp
@@ +213,5 @@
> +  return NS_OK;
> +}
> +
> +NS_IMETHODIMP
> +CookieServiceParent::GetDocumentNeedsCookie(bool *aDocumentNeedsCookie)

I don't see how this will work; a CookieServiceParent represents many possible documents. What is the purpose of this flag?

	Josh Matthews [:jdm]
	Comment 44 • 7 years ago

Comment on attachment 8852155 [details] [diff] [review]
implementation

Review of attachment 8852155 [details] [diff] [review]:
-----------------------------------------------------------------

There is still a lot of code in this patch that I have not looked through closely because there are a number of problems I found that require significant changes. I am concerned by how much code is being duplicated for reasons that I do not understand. Here are some changes that will make it easier to review this patch:
* when sending cookies from the parent to the child, use an IPDL structure instead of a string. This should allow us to revert all of the changes to the parsing code, since the parent already stores parsed nsCookie values.
* avoid modifying the nsICookieService interface, and add APIs to nsCookieService or CookieServiceChild instead.
* remove many places that check the load flags and content policy type for channels. They should not be necessary, and then we do not need to create dummy channels in many places.

::: docshell/base/nsDocShell.cpp
@@ +5797,5 @@
>  
> +  nsCOMPtr<nsICookieService> service = do_GetService(NS_COOKIESERVICE_CONTRACTID);
> +  nsIChannel *channel = GetCurrentDocChannel();
> +  if (service && mLoadingURI && channel ) {
> +    service->DestroyCookiesFromTable(mLoadingURI, channel);

Note to self: we probably need to look at mOriginalURI here, per Ehsan's original comment.

@@ +8605,5 @@
> +    if (doc) {
> +      sandboxFlags = doc->GetSandboxFlags();
> +    }
> +  }
> +  aLoadFlags |= nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE;

This function seems to be confused; we don't do anything with the knowledge of sandboxing. In particular, unless the sandbox flags include the "allow-same-origin" value then we do not need to set LOAD_DOCUMENT_NEEDS_COOKIE.

::: dom/html/nsHTMLDocument.cpp
@@ +2413,5 @@
>      nsLoadFlags loadFlags = 0;
>      channel->GetLoadFlags(&loadFlags);
>      loadFlags |= nsIChannel::LOAD_DOCUMENT_URI;
> +    if (mSandboxFlags) {
> +      loadFlags |= nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE;

We should only be setting LOAD_DOCUMENT_NEEDS_COOKIE if there are no sandboxing flags or the "allow-same-origin" flag is set.

::: netwerk/base/nsIRequest.idl
@@ +131,5 @@
>       */
>      const unsigned long LOAD_HTML_OBJECT_DATA = 1 << 1;
>  
> +    /**
> +     * TODO: add comment to explain the function of this flag.

This comment isn't closed.

::: netwerk/cookie/CookieServiceChild.cpp
@@ +139,5 @@
> +  if (docCookieInfoList && key.Length() > 0) {
> +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> +      docCookieInfo = (*docCookieInfoList)[i];
> +      if (docCookieInfo.mPathName.Equals(sourcePath)) {
> +        if (!docCookieInfo.mCookieName.IsEmpty() || !docCookieInfo.mCookieValue.IsEmpty()) {

I think we should be able to assert these conditions.

@@ +147,5 @@
> +          if (!docCookieInfo.mCookieName.IsEmpty()) {
> +            aCookieString.Append(docCookieInfo.mCookieName.get());
> +          } else {
> +            aCookieString.Append(docCookieInfo.mCookieValue.get());
> +          }

I don't understand this logic. Also, shouldn't there be "=" separating names and values?

@@ +198,3 @@
>    // Synchronously call the parent.
> +    SendGetCookieString(uriParams, !!isForeign, attrs, &result);
> +  }

The point of these changes is to remove all synchronous IPC related to cookies. We don't need to check anything related to the channel that's being used; we should always have the necessary information available in the cookie map.

@@ +245,5 @@
> +  nsLoadFlags loadFlags;
> +  aChannel->GetLoadFlags(&loadFlags);
> +  if (loadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE) {
> +    UpdateCookieToTable(aHostURI, aChannel, aCookieString);
> +  }

We do not need to check anything about the channel here. We should always update the cookie map.

@@ +250,3 @@
>    // Synchronously call the parent.
>    SendSetCookieString(uriParams, !!isForeign, cookieString, serverTime,
>                        attrs);

This IPC message should be made asynchronous.

@@ +331,5 @@
> +CookieServiceChild::UpdateCookieToTable(nsIURI     *aHostURI,
> +                                        nsIChannel *aChannel,
> +                                        const char *aCookie)
> +{
> +  if (!aHostURI || !aChannel) {

There's a lot of duplicated code between the parent and the child implementation. Can we move this logic into the DocumentCookieMap type instead?

::: netwerk/cookie/CookieServiceParent.cpp
@@ +51,3 @@
>  }
>  
> +CookieServiceParent*

Let's return already_AddRefed<CookieServiceParent> instead.

@@ +66,5 @@
> +                             const char16_t  *aData)
> +{
> +  NS_ASSERTION(strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0,
> +                      "not a pref change topic!");
> +  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);

Why was this code added? This class did not add any preference observers.

@@ +172,5 @@
> +CookieServiceParent::GetCookieStringInternal(nsIURI *aHostURI,
> +                                             nsIChannel *aChannel,
> +                                             char **aCookieString)
> +{
> +  if (!mCookieService) {

As part of implementing nsICookieService, we appear to be duplicating a lot of code from CookieServiceChild. I don't understand why this is happening; I can't even find any code in this patch that calls these methods.

@@ +308,5 @@
> +  mContentProcessCookies.Get(key, &docCookieInfoList);
> +  nsCString baseDomain;
> +  aHostURI->GetAsciiHost(baseDomain);
> +  bool hostURIExist = false;
> +  if (docCookieInfoList && key.Length() > 0) {

Why do we have these checks for key length?

@@ +310,5 @@
> +  aHostURI->GetAsciiHost(baseDomain);
> +  bool hostURIExist = false;
> +  if (docCookieInfoList && key.Length() > 0) {
> +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> +      docCookieInfo = (*docCookieInfoList)[i];

docCookieInfo isn't a pointer, so this looks like it's copying the stored value. This means that modifying mRefDocCnt later will not update the value in the array.

@@ +313,5 @@
> +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> +      docCookieInfo = (*docCookieInfoList)[i];
> +      if (docCookieInfo.mPathName.Equals(baseDomain)) {
> +        hostURIExist = true;
> +        docCookieInfo.mRefDocCnt++;

We should probably break out of the loop for clarity.

@@ +326,5 @@
> +  if (!hostURIExist) {
> +    docCookieInfo.mPathName = baseDomain;
> +    docCookieInfo.mIsSecure = true;
> +    docCookieInfo.mRefDocCnt++;
> +    docCookieInfoList->InsertElementAt(docCookieInfoList->Length(), docCookieInfo);

Let's use AppendElement instead.

@@ +328,5 @@
> +    docCookieInfo.mIsSecure = true;
> +    docCookieInfo.mRefDocCnt++;
> +    docCookieInfoList->InsertElementAt(docCookieInfoList->Length(), docCookieInfo);
> +  }
> +  mContentProcessCookies.Put(key, docCookieInfoList);

Let's move this into the branch that creates a new list, sine it should not be necessary otherwise.

@@ +359,5 @@
> +  mContentProcessCookies.Get(key, &docCookieInfoList);
> +  nsCString sourcePath = nsCookieService::GetPathFromURI(aHostURI);
> +  if (docCookieInfoList && key.Length() > 0) {
> +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> +      docCookieInfo = (*docCookieInfoList)[i];

Same issue here about copying the value.

@@ +366,5 @@
> +          docCookieInfo.mRefDocCnt--;
> +        } else {
> +          docCookieInfoList->RemoveElementAt(i);
> +        }
> +      }

We should be breaking out of the loop here, or our loop index could be incorrect after removing an element.

@@ +371,5 @@
> +    }
> +  } else if (!docCookieInfoList || key.Length() <= 0) {
> +    return NS_ERROR_NOT_AVAILABLE;
> +  }
> +  mContentProcessCookies.Put(key, docCookieInfoList);

If the list is now empty, we should delete it and remove the hashtable entry to reclaim the memory we allocated.

::: netwerk/cookie/CookieServiceParent.h
@@ +7,5 @@
>  #define mozilla_net_CookieServiceParent_h
>  
>  #include "mozilla/net/PCookieServiceParent.h"
> +#include "nsICookieService.h"
> +#include "nsIobserver.h"

This should be nsIObserver.h.

@@ +17,5 @@
>  namespace mozilla {
>  namespace net {
>  
>  class CookieServiceParent : public PCookieServiceParent
> +                          , public nsICookieService

Why are we inheriting from and implementing this interface? I can't find any code that needs this.

@@ +29,3 @@
>    CookieServiceParent();
> +  static CookieServiceParent* GetSingleton();
> +  DocumentCookieMap mContentProcessCookies;

Let's not make this map public.

::: netwerk/cookie/PCookieService.ipdl
@@ +10,5 @@
>  
>  using mozilla::OriginAttributes from "mozilla/ipc/BackgroundUtils.h";
> +using struct mozilla::net::DocumentCookieInfo from "mozilla/net/DocumentCookieStruct.h";
> +using mozilla::net::DocumentCookieInfoList from "mozilla/net/DocumentCookieStruct.h";
> +using mozilla::net::DocumentCookieMap from "mozilla/net/DocumentCookieStruct.h";

Why are these necessary?

::: netwerk/cookie/moz.build
@@ +22,5 @@
>  if CONFIG['NECKO_COOKIES']:
>      EXPORTS.mozilla.net = [
>          'CookieServiceChild.h',
>          'CookieServiceParent.h',
> +        'DocumentCookieStruct.h',

This file is missing from this patch.

::: netwerk/cookie/nsCookieService.cpp
@@ +773,5 @@
>    }
>  
> +  // Get the nsCookieService instance directly, so we can call internal methods.
> +  mCookieServiceParent =
> +    already_AddRefed<CookieServiceParent>(CookieServiceParent::GetSingleton());

We should remove this cast when making the changes to CookieServiceParent::GetSingleton().

@@ +1978,4 @@
>    return NS_OK;
>  }
>  
> +

nit: revert this change.

@@ +3557,5 @@
>                               nsCookie                      *aCookie,
>                               int64_t                        aCurrentTimeInUsec,
>                               nsIURI                        *aHostURI,
>                               const char                    *aCookieHeader,
> +                             nsIChannel                    *aChannel,

This argument should not be necessary.

@@ +3767,5 @@
> +  if (aHostURI && aChannel && aCookieHeader) {
> +    nsLoadFlags loadFlags;
> +    aChannel->GetLoadFlags(&loadFlags);
> +    if (loadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE) {
> +      mCookieServiceParent->UpdateCookieToTable(aHostURI, aChannel, aCookieHeader);

There are a couple issues here - this method should not require a channel, because what we do with the cookie should not depend on whether there's a channel trying to load a document that triggered this update. I don't believe this added code should be necessary at all - the cookie service parent will receive a notification via the observer service instead.

@@ +3916,5 @@
> +nsCookieService::ConfirmMatchingCookie(nsCookie           *aCookie,
> +                                       nsIURI             *aHostURI,
> +                                       const char         *aCookieHeader,
> +                                       nsIChannel         *aChannel,
> +                                       nsCookieAttributes *aCookieAttributes,

aCookie and aCookieAttributes are local pointers. The caller can't observe any change to them, but this code is allocating memory and storing it in them, causing memory leaks. I don't believe this code is doing what it is supposed to.

@@ +4019,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  aKey.Append(spec);
> +  aKey.Append(NS_LITERAL_CSTRING("!"));
> +  aKey.AppendInt(aContentType);

I am concerned about this key. What is this code based on? Why do we need to include an nsContentPolicyType in it? The hash for hostTable in nsCookieService is based on the base domain and origin attributes; can we reuse nsCookieKey somehow instead for this new hashtable?

::: netwerk/cookie/nsCookieService.h
@@ +64,5 @@
> +                             nsrefcnt mRefDocCnt;
> +                               };
> +
> +    typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;
> +      typedef nsClassHashtable<nsCStringHashKey, DocumentCookieInfoList> DocumentCookieMap;*/

Let's remove this commented out code.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +378,5 @@
> +  nsCOMPtr<nsIURI> uri;
> +  nsresult result;
> +  result = GetURI(getter_AddRefs(uri));
> +  nsICookieService *cs = gHttpHandler->GetCookieService();
> +  cs->SetCookieString(uri, nullptr, cookie.get(), this);

I believe we only need to call UpdateCookieToTable here.

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1144,5 @@
> +     nsCOMPtr<nsIURI> newURI;
> +     chan->GetURI(getter_AddRefs(newURI));
> +     nsICookieService *cs = mHttpHandler->GetCookieService();
> +     nsCString cookies;
> +     cs->GetCookieString(newURI, chan, getter_Copies(cookies));

Rather than getting the cookie string and splitting it into an array of strings, I believe the following design would be cleaner and involve less string parsing:
* declare an IPDL structure that represents a cookie, containing the same fields as nsCookie
* add a GetScriptAccessibleCookiesForURL API to nsCookieService that retrieves an array of this new structure
* send the array in a single IPDL message
* make a nsCookie constructor that accepts an instance of the new IPDL structure

	Jason Duell
	Comment 45 • 7 years ago

re: comment 6 and comment 28:  we're going to add a IsDocumentChannel() method for determining whether a channel is for a document load.  See bug 1354349.  We should use that code in this bug.

	Amy Chung [:Amy] Assignee
	Comment 46 • 7 years ago

(In reply to Josh Matthews [:jdm] from comment #44)
> Comment on attachment 8852155 [details] [diff] [review]
> implementation
> 
> Review of attachment 8852155 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There is still a lot of code in this patch that I have not looked through
> closely because there are a number of problems I found that require
> significant changes. I am concerned by how much code is being duplicated for
> reasons that I do not understand. Here are some changes that will make it
> easier to review this patch:
> * when sending cookies from the parent to the child, use an IPDL structure
> instead of a string. This should allow us to revert all of the changes to
> the parsing code, since the parent already stores parsed nsCookie values.
> * avoid modifying the nsICookieService interface, and add APIs to
> nsCookieService or CookieServiceChild instead.
> * remove many places that check the load flags and content policy type for
> channels. They should not be necessary, and then we do not need to create
> dummy channels in many places.
> 
Thanks for your suggestions.

> ::: docshell/base/nsDocShell.cpp
> @@ +5797,5 @@
> >  
> > +  nsCOMPtr<nsICookieService> service = do_GetService(NS_COOKIESERVICE_CONTRACTID);
> > +  nsIChannel *channel = GetCurrentDocChannel();
> > +  if (service && mLoadingURI && channel ) {
> > +    service->DestroyCookiesFromTable(mLoadingURI, channel);
> 
> Note to self: we probably need to look at mOriginalURI here, per Ehsan's
> original comment.
>  
> @@ +8605,5 @@
> > +    if (doc) {
> > +      sandboxFlags = doc->GetSandboxFlags();
> > +    }
> > +  }
> > +  aLoadFlags |= nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE;
> 
> This function seems to be confused; we don't do anything with the knowledge
> of sandboxing. In particular, unless the sandbox flags include the
> "allow-same-origin" value then we do not need to set
> LOAD_DOCUMENT_NEEDS_COOKIE.
> 
> ::: dom/html/nsHTMLDocument.cpp
> @@ +2413,5 @@
> >      nsLoadFlags loadFlags = 0;
> >      channel->GetLoadFlags(&loadFlags);
> >      loadFlags |= nsIChannel::LOAD_DOCUMENT_URI;
> > +    if (mSandboxFlags) {
> > +      loadFlags |= nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE;
> 
> We should only be setting LOAD_DOCUMENT_NEEDS_COOKIE if there are no
> sandboxing flags or the "allow-same-origin" flag is set.
> 
Ok, I will confirm the sandboxed flags which include the "allow-same-origin".

> ::: netwerk/base/nsIRequest.idl
> @@ +131,5 @@
> >       */
> >      const unsigned long LOAD_HTML_OBJECT_DATA = 1 << 1;
> >  
> > +    /**
> > +     * TODO: add comment to explain the function of this flag.
> 
> This comment isn't closed.
> 
I already closed the comment.

> ::: netwerk/cookie/CookieServiceChild.cpp
> @@ +139,5 @@
> > +  if (docCookieInfoList && key.Length() > 0) {
> > +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> > +      docCookieInfo = (*docCookieInfoList)[i];
> > +      if (docCookieInfo.mPathName.Equals(sourcePath)) {
> > +        if (!docCookieInfo.mCookieName.IsEmpty() || !docCookieInfo.mCookieValue.IsEmpty()) {
> 
> I think we should be able to assert these conditions.
> 
Ok, I will do it.
> @@ +147,5 @@
> > +          if (!docCookieInfo.mCookieName.IsEmpty()) {
> > +            aCookieString.Append(docCookieInfo.mCookieName.get());
> > +          } else {
> > +            aCookieString.Append(docCookieInfo.mCookieValue.get());
> > +          }
> 
> I don't understand this logic. Also, shouldn't there be "=" separating names
> and values?
> 
Sorry I forgot to add "=" and values.
> @@ +198,3 @@
> >    // Synchronously call the parent.
> > +    SendGetCookieString(uriParams, !!isForeign, attrs, &result);
> > +  }
> 
> The point of these changes is to remove all synchronous IPC related to
> cookies. We don't need to check anything related to the channel that's being
> used; we should always have the necessary information available in the
> cookie map.
> 
> @@ +245,5 @@
> > +  nsLoadFlags loadFlags;
> > +  aChannel->GetLoadFlags(&loadFlags);
> > +  if (loadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE) {
> > +    UpdateCookieToTable(aHostURI, aChannel, aCookieString);
> > +  }
> 
> We do not need to check anything about the channel here. We should always
> update the cookie map.
> 
Ok, I understand.

> @@ +250,3 @@
> >    // Synchronously call the parent.
> >    SendSetCookieString(uriParams, !!isForeign, cookieString, serverTime,
> >                        attrs);
> 
> This IPC message should be made asynchronous.
> 
Ok, I will modify it.

> @@ +331,5 @@
> > +CookieServiceChild::UpdateCookieToTable(nsIURI     *aHostURI,
> > +                                        nsIChannel *aChannel,
> > +                                        const char *aCookie)
> > +{
> > +  if (!aHostURI || !aChannel) {
> 
> There's a lot of duplicated code between the parent and the child
> implementation. Can we move this logic into the DocumentCookieMap type
> instead?
> 
Ok, I will move UpdateCookieToTable to DocumentCookieStruct.h

> ::: netwerk/cookie/CookieServiceParent.cpp
> @@ +51,3 @@
> >  }
> >  
> > +CookieServiceParent*
> 
> Let's return already_AddRefed<CookieServiceParent> instead.
> 
I changed the method as below:
When CookieServiceParent receive the notification about modifying cookie, then call UpdateCookieToTable and ask CookieServiceChild to call UpdateCookieToTable too.

> @@ +66,5 @@
> > +                             const char16_t  *aData)
> > +{
> > +  NS_ASSERTION(strcmp(aTopic, NS_PREFBRANCH_PREFCHANGE_TOPIC_ID) == 0,
> > +                      "not a pref change topic!");
> > +  nsCOMPtr<nsIPrefBranch> prefBranch = do_QueryInterface(aSubject);
> 
> Why was this code added? This class did not add any preference observers.
> 
I will remove these code.

> @@ +172,5 @@
> > +CookieServiceParent::GetCookieStringInternal(nsIURI *aHostURI,
> > +                                             nsIChannel *aChannel,
> > +                                             char **aCookieString)
> > +{
> > +  if (!mCookieService) {
> 
> As part of implementing nsICookieService, we appear to be duplicating a lot
> of code from CookieServiceChild. I don't understand why this is happening; I
> can't even find any code in this patch that calls these methods.
> 
> @@ +308,5 @@
> > +  mContentProcessCookies.Get(key, &docCookieInfoList);
> > +  nsCString baseDomain;
> > +  aHostURI->GetAsciiHost(baseDomain);
> > +  bool hostURIExist = false;
> > +  if (docCookieInfoList && key.Length() > 0) {
> 
> Why do we have these checks for key length?
> 
Ok, I will remove it.

> @@ +310,5 @@
> > +  aHostURI->GetAsciiHost(baseDomain);
> > +  bool hostURIExist = false;
> > +  if (docCookieInfoList && key.Length() > 0) {
> > +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> > +      docCookieInfo = (*docCookieInfoList)[i];
> 
> docCookieInfo isn't a pointer, so this looks like it's copying the stored
> value. This means that modifying mRefDocCnt later will not update the value
> in the array.
> 
Ok, I understand.

> @@ +313,5 @@
> > +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> > +      docCookieInfo = (*docCookieInfoList)[i];
> > +      if (docCookieInfo.mPathName.Equals(baseDomain)) {
> > +        hostURIExist = true;
> > +        docCookieInfo.mRefDocCnt++;
> 
> We should probably break out of the loop for clarity.
> 
Ok, I will modify it.

> @@ +326,5 @@
> > +  if (!hostURIExist) {
> > +    docCookieInfo.mPathName = baseDomain;
> > +    docCookieInfo.mIsSecure = true;
> > +    docCookieInfo.mRefDocCnt++;
> > +    docCookieInfoList->InsertElementAt(docCookieInfoList->Length(), docCookieInfo);
> 
> Let's use AppendElement instead.
> 
Ok, I will modify it.

> @@ +328,5 @@
> > +    docCookieInfo.mIsSecure = true;
> > +    docCookieInfo.mRefDocCnt++;
> > +    docCookieInfoList->InsertElementAt(docCookieInfoList->Length(), docCookieInfo);
> > +  }
> > +  mContentProcessCookies.Put(key, docCookieInfoList);
> 
> Let's move this into the branch that creates a new list, sine it should not
> be necessary otherwise.
> 
Ok, I will move it into the else branch.

> @@ +359,5 @@
> > +  mContentProcessCookies.Get(key, &docCookieInfoList);
> > +  nsCString sourcePath = nsCookieService::GetPathFromURI(aHostURI);
> > +  if (docCookieInfoList && key.Length() > 0) {
> > +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> > +      docCookieInfo = (*docCookieInfoList)[i];
> 
> Same issue here about copying the value.
> 
Ok, I will modify it.

> @@ +366,5 @@
> > +          docCookieInfo.mRefDocCnt--;
> > +        } else {
> > +          docCookieInfoList->RemoveElementAt(i);
> > +        }
> > +      }
> 
> We should be breaking out of the loop here, or our loop index could be
> incorrect after removing an element.
> 
Ok, I will modify it.

> @@ +371,5 @@
> > +    }
> > +  } else if (!docCookieInfoList || key.Length() <= 0) {
> > +    return NS_ERROR_NOT_AVAILABLE;
> > +  }
> > +  mContentProcessCookies.Put(key, docCookieInfoList);
> 
> If the list is now empty, we should delete it and remove the hashtable entry
> to reclaim the memory we allocated.
> 
Ok, I will use IsEmpty() to confirm the list and remove this entry from hashtable. 

> ::: netwerk/cookie/CookieServiceParent.h
> @@ +7,5 @@
> >  #define mozilla_net_CookieServiceParent_h
> >  
> >  #include "mozilla/net/PCookieServiceParent.h"
> > +#include "nsICookieService.h"
> > +#include "nsIobserver.h"
> 
> This should be nsIObserver.h.
> 
I already modified it before, thanks.

> @@ +17,5 @@
> >  namespace mozilla {
> >  namespace net {
> >  
> >  class CookieServiceParent : public PCookieServiceParent
> > +                          , public nsICookieService
> 
> Why are we inheriting from and implementing this interface? I can't find any
> code that needs this.
> 
Because I created a function UpdateCookieToTable() on nsICookieService, I will move UpdateCookieToTable() to DocumentCookieStruct.h and remove the inheritance from CookieServiceParent.h.

> @@ +29,3 @@
> >    CookieServiceParent();
> > +  static CookieServiceParent* GetSingleton();
> > +  DocumentCookieMap mContentProcessCookies;
> 
> Let's not make this map public.
> 
Ok, I will modify it.

> ::: netwerk/cookie/PCookieService.ipdl
> @@ +10,5 @@
> >  
> >  using mozilla::OriginAttributes from "mozilla/ipc/BackgroundUtils.h";
> > +using struct mozilla::net::DocumentCookieInfo from "mozilla/net/DocumentCookieStruct.h";
> > +using mozilla::net::DocumentCookieInfoList from "mozilla/net/DocumentCookieStruct.h";
> > +using mozilla::net::DocumentCookieMap from "mozilla/net/DocumentCookieStruct.h";
> 
> Why are these necessary?
> 
Ok, I will use include "DocumentCookieStruct.h".

> ::: netwerk/cookie/moz.build
> @@ +22,5 @@
> >  if CONFIG['NECKO_COOKIES']:
> >      EXPORTS.mozilla.net = [
> >          'CookieServiceChild.h',
> >          'CookieServiceParent.h',
> > +        'DocumentCookieStruct.h',
> 
> This file is missing from this patch.
> 
Sorry for I didn't export this code on my patch.

> ::: netwerk/cookie/nsCookieService.cpp
> @@ +773,5 @@
> >    }
> >  
> > +  // Get the nsCookieService instance directly, so we can call internal methods.
> > +  mCookieServiceParent =
> > +    already_AddRefed<CookieServiceParent>(CookieServiceParent::GetSingleton());
> 
> We should remove this cast when making the changes to
> CookieServiceParent::GetSingleton().
> 
I already removed CookieServiceParent::GetSingleton().

> @@ +1978,4 @@
> >    return NS_OK;
> >  }
> >  
> > +
> 
> nit: revert this change.
> 
I will remove it.

> @@ +3557,5 @@
> >                               nsCookie                      *aCookie,
> >                               int64_t                        aCurrentTimeInUsec,
> >                               nsIURI                        *aHostURI,
> >                               const char                    *aCookieHeader,
> > +                             nsIChannel                    *aChannel,
> 
> This argument should not be necessary.
> 

> @@ +3767,5 @@
> > +  if (aHostURI && aChannel && aCookieHeader) {
> > +    nsLoadFlags loadFlags;
> > +    aChannel->GetLoadFlags(&loadFlags);
> > +    if (loadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE) {
> > +      mCookieServiceParent->UpdateCookieToTable(aHostURI, aChannel, aCookieHeader);
> 
> There are a couple issues here - this method should not require a channel,
> because what we do with the cookie should not depend on whether there's a
> channel trying to load a document that triggered this update. I don't
> believe this added code should be necessary at all - the cookie service
> parent will receive a notification via the observer service instead.
> 
I already modified to let CookieServiceParent receive the notification from the observer.
If CookieServiceParent get "cookie-changed" or "private-cookie-changed", it will call UpdateCookieToTable() and ask CookieServiceChild to call UpdateCookieToTable() too.

> @@ +3916,5 @@
> > +nsCookieService::ConfirmMatchingCookie(nsCookie           *aCookie,
> > +                                       nsIURI             *aHostURI,
> > +                                       const char         *aCookieHeader,
> > +                                       nsIChannel         *aChannel,
> > +                                       nsCookieAttributes *aCookieAttributes,
> 
> aCookie and aCookieAttributes are local pointers. The caller can't observe
> any change to them, but this code is allocating memory and storing it in
> them, causing memory leaks. I don't believe this code is doing what it is
> supposed to.
> 
> @@ +4019,5 @@
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  aKey.Append(spec);
> > +  aKey.Append(NS_LITERAL_CSTRING("!"));
> > +  aKey.AppendInt(aContentType);
> 
> I am concerned about this key. What is this code based on? Why do we need to
> include an nsContentPolicyType in it? The hash for hostTable in
> nsCookieService is based on the base domain and origin attributes; can we
> reuse nsCookieKey somehow instead for this new hashtable?
> 
Ok, I understand.

> ::: netwerk/cookie/nsCookieService.h
> @@ +64,5 @@
> > +                             nsrefcnt mRefDocCnt;
> > +                               };
> > +
> > +    typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;
> > +      typedef nsClassHashtable<nsCStringHashKey, DocumentCookieInfoList> DocumentCookieMap;*/
> 
> Let's remove this commented out code.
> 
Ok, I will remove it.

> ::: netwerk/protocol/http/HttpChannelChild.cpp
> @@ +378,5 @@
> > +  nsCOMPtr<nsIURI> uri;
> > +  nsresult result;
> > +  result = GetURI(getter_AddRefs(uri));
> > +  nsICookieService *cs = gHttpHandler->GetCookieService();
> > +  cs->SetCookieString(uri, nullptr, cookie.get(), this);
> 
> I believe we only need to call UpdateCookieToTable here.
> 
Ok, I will modify it.

> ::: netwerk/protocol/http/HttpChannelParent.cpp
> @@ +1144,5 @@
> > +     nsCOMPtr<nsIURI> newURI;
> > +     chan->GetURI(getter_AddRefs(newURI));
> > +     nsICookieService *cs = mHttpHandler->GetCookieService();
> > +     nsCString cookies;
> > +     cs->GetCookieString(newURI, chan, getter_Copies(cookies));
> 
> Rather than getting the cookie string and splitting it into an array of
> strings, I believe the following design would be cleaner and involve less
> string parsing:
> * declare an IPDL structure that represents a cookie, containing the same
> fields as nsCookie
I will create a new ipdl struct as "PCookie.ipdl" and discuss with you.

> * add a GetScriptAccessibleCookiesForURL API to nsCookieService that
> retrieves an array of this new structure
I will add this API.

> * send the array in a single IPDL message
> * make a nsCookie constructor that accepts an instance of the new IPDL
> structure


Thanks for your feedback.

	Amy Chung [:Amy] Assignee
	Comment 47 • 7 years ago

Hi Josh,
The implementation about crating ipdl struct to represent a cookie as below:
1. Create a ipdl struct naming CookieStruct on PCookieService.ipdl.
2. Add a new argument nsTArry<CookieStruct> in GetCookieStringInteranl().
3. Create a function naming ConfirmMatchingCookies() for confirming the cookie is matching the conditions on CookieServiceParent.
4. Move SetMatchCookie from PHttpChannel.ipdl to PCookieService.ipdl.
5. If the cookie matches the conditions, call SendSetMatchCookie() on CookieServiceParent.
6. CookieServiceChild call UpdateCookieToTable() when get ipc msg.
Would you give me your suggestions?

Thanks!

	Josh Matthews [:jdm]
	Comment 49 • 7 years ago

(In reply to Amy Chung [:Amy] from comment #47)
> 2. Add a new argument nsTArry<CookieStruct> in GetCookieStringInteranl().

I don't think this sounds like the best way to go about it. We should extract https://dxr.mozilla.org/mozilla-central/rev/c697e756f738ce37abc56f31bfbc48f55625d617/netwerk/cookie/nsCookieService.cpp#3209-3308 into a separate method that takes the AutoTArray<nsCookie*> as an argument; then we can transform this array into the CookieStruct values we need for IPDL.

> 4. Move SetMatchCookie from PHttpChannel.ipdl to PCookieService.ipdl.
> 5. If the cookie matches the conditions, call SendSetMatchCookie() on
> CookieServiceParent.

I'm not sure if these steps are necessary. It seems like it could be difficult to get the CookieServiceParent actor here.

Otherwise those steps sound good.

	Amy Chung [:Amy] Assignee
	Comment 50 • 7 years ago

Hi Josh,
In my view, the implementations can divide into four part as below:
1. Set LoadFlags.
2. Init Hash Table.
3. Update Hash Table.
4. Destroy Hash Table
I can separate my patch to four parts for reviewing clear.
Would you give me your suggestions? 

Thanks!

	Valentin Gosu [:valentin] (he/him)
	Comment 51 • 7 years ago

* Added CookieStruct to NeckoChannelParams.ipdlh
* Added GetCookieListInternal that appends cookies to a nsTArray<nsCookie*>
* Change GetCookieStringInternal to use GetCookieListInternal
* Add CookieList[] argument to PHttpChannel::OnStartRequest to pass it to the child
* Add nsCookieService::GetCookieStructList to get a list of CookieStructs to send to the child

MozReview-Commit-ID: GJkvEz7mWW0

	Nika Layzell [:nika] (ni? for response)
	Comment 52 • 7 years ago

The problem which I ran into with the permission manager work, might be relevant here for you as well. See bug 1355608 comment 15. Basically the problem is that a document loaded due to a service worker fetch interception doesn't go to the parent process at any point during the load. This is problematic for sending down cookies/permissions when the document is loaded, as it means that the document can be loaded before the data is requested.

I'm not sure what the best solution will be for this bug, but I thought I should let you know ahead of time. For the service worker work, we're planning to send down permissions for all registered service worker scopes at startup, and as the scopes are registered.

	Josh Matthews [:jdm]
	Comment 53 • 7 years ago

(In reply to Amy Chung [:Amy] from comment #50)
> Hi Josh,
> In my view, the implementations can divide into four part as below:
> 1. Set LoadFlags.
> 2. Init Hash Table.
> 3. Update Hash Table.
> 4. Destroy Hash Table
> I can separate my patch to four parts for reviewing clear.
> Would you give me your suggestions? 
> 
> Thanks!

If the result is smaller patches, I am in favour of this :)

	Amy Chung [:Amy] Assignee
	Comment 54 • 7 years ago

Hi Josh,
My modification as below:
[Set LoadFlags]
* nsDocShell.cpp
    * If CofirmBoxedForSettingLoadFlags return true, add loadflags LOAD_DOCUMENT_NEEDS_COOKIE when load flag set LOAD_DOCUMENT_URI.
* nsDocShell.h
    * Create a function "CofirmBoxedForSettingLoadFlags" about confirming the sandboxed flags include "SANDBOXED_ORIGIN".
* nsHTMLDocument.cpp
    * If sandboxed flags include "SANDBOXED_ORIGIN", add loadflags LOAD_DOCUMENT_NEEDS_COOKIE when load flag set LOAD_DOCUMENT_URI.
* nsIRequest.idl
    * Create load flag LOAD_DOCUMENT_NEEDS_COOKIE.

Would you give me your suggestions?

Thanks!

	Amy Chung [:Amy] Assignee
	Comment 55 • 7 years ago

Hi Josh,
My modification as below:
[Init Hash Table]
 - Data struct
   1. netwerk/cookie/DocumentCookieOperation.h
   2. netwerk/cookie/DocumentCookieoperation.cpp
      * Define DocumentCookieInfo、 DocumentCookieList and DocumentCookieMap.
      * Define the function about updating, destroy and get cookie from hash table.
      * Define GetBaseDomain and GetBaseDomainFromHost.
   3. netwerk/cookie/nsCookieKey
      * Define the key on DocumentCookieMap and DBState.
   4. netwerk/ipc/NeckoChannelParams.ipdlh
      * Create new ipdl strcut which naming cookie struct.
 - HttpChannel
   1. netwerk/protocol/http/HttpChannelChild.h
      * Add new argument cookiesList on RecvOnStartRequest()
   2. netwerk/protocol/http/HttpChannelChild.cpp
      * If cookiesList which in  RecvOnStartRequest() is not empty, call Init hash table from CookieServiceChild.
   3. netwerk/protocol/http/HttpChannelParent.cpp
      * Add a expression to confirm the cookies which is matching cookie.
   4. netwerk/protocol/http/PHttpChannel.ipdl
      * Add a argument cookiesList on OnStartRequest()
 - CookieService
   1. netwerk/cookie/CookieServiceChild.cpp
      * Init hash table
      * When recv the ipc msg about have to update cookie in hash table,
        call DocumentCookieOperation->UpdateHashTable()
      * Remove SendGetCookieStringInternal, use DocumentCookieOperation->GetCookieStringFromHashTable() instead.
   2. netwerk/cookie/CookieServiceChild.h
      * Create DocumentCookieOperation object naming mDocCookieOperation.
   3. netwerk/cookie/CookieServiceParent.cpp
      * Add observer on Constructor.
      * Create Observer for processing the notifies which already registered.
        * cookie-changed
        * private-cookie-changed
        * init-hash-table
        * destroy-hash-table
      * Remove observer on ActorDestroy()
   4. netwerk/cookie/CookieServiceParent.h
      * Inherit nsIObserver
      * Create DocumentCookieOperation object naming mDocCookieOperation.
   5. netwerk/cookie/nsCookieService.cpp
      * Create GetCookieStructList & GetCookieStructListInternal.
      * Remove GeBaseDomain & GetBaseDomainFromHost.
      * Create ConfirmMatchingCookie
      * Create new notify "init-hash-table" and "destroy-hash-table"
   6. netwerk/cookie/nsCookieService.h
      * Remove nsCookieKey class.
      * Remove mTLDService.
      * Create DocumentCookieOperation object naming mDocCookieOperation for calling GetBaseDomain.
   7. netwerk/cookie/moz.build
      * Add DocumentCookieOperation & nsCookieKey
   8. netwerk/ipc/NeckoParent.cpp
      * Use AddRef when calling CookieServiceParent Constructor
      * Use Release when calling CookieServiceParent Destructor

[Not Finish]  
   1. Sort DocumentCookieList by patch length when get cookie string.
      * Add new field "path" in DocumentCookieInfo.

Would you give me your suggestions?

Thanks!

	Amy Chung [:Amy] Assignee
	Comment 56 • 7 years ago

Hi Josh,
My modification as below:
* netwerk/cookie/nsCookieService.cpp
    * Implement xpcom interface DestroyHashTableFromDocument.
* netwerk/cookie/nsICookieService.idl
    * Add new interface destroyHashTableFromDocument
* netwerk/cookie/CookieServiceChild.cpp
    * Implement xpcom interface DestroyHashTableFromDocument.
* docshell/base/nsDocShell.cpp
    * Call Destroy hash table when this object be released.
* dom/html/nsHTMLDocument.cpp
    * Call Destroy hash table when this object be released.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 58 • 7 years ago

Hi Josh,
I found the part2 patch still too large, so I divided the function on init & update on hash table and get cookie from hash table into three part -- data struct, http channel and cookie service.
 [Data struct]
   1. netwerk/cookie/DocumentCookieOperation.h
   2. netwerk/cookie/DocumentCookieoperation.cpp
      * Define DocumentCookieInfo、 DocumentCookieList and DocumentCookieMap.
      * Define the function about updating, destroy and get cookie from hash table.
      * Define GetBaseDomain and GetBaseDomainFromHost.
   3. netwerk/cookie/nsCookieKey
      * Define the key on DocumentCookieMap and DBState.
   4. netwerk/ipc/NeckoChannelParams.ipdlh
      * Create new ipdl strcut which naming cookie struct.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 59 • 7 years ago

Hi Josh,
My modification as below:
[HttpChannel]
   1. netwerk/protocol/http/HttpChannelChild.h
      * Add new argument cookiesList on RecvOnStartRequest()
   2. netwerk/protocol/http/HttpChannelChild.cpp
      * If cookiesList which in  RecvOnStartRequest() is not empty, call Init hash table from CookieServiceChild.
   3. netwerk/protocol/http/HttpChannelParent.cpp
      * Add a expression to confirm the cookies which is matching cookie.
   4. netwerk/protocol/http/PHttpChannel.ipdl
      * Add a argument cookiesList on OnStartRequest().

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 60 • 7 years ago

Hi Josh,
My modification as below:
 [CookieService]
   1. netwerk/cookie/CookieServiceChild.cpp
      * Init hash table
      * When recv the ipc msg about have to update cookie in hash table,
        call DocumentCookieOperation->UpdateHashTable()
      * Remove SendGetCookieStringInternal, use DocumentCookieOperation->GetCookieStringFromHashTable() instead.
   2. netwerk/cookie/CookieServiceChild.h
      * Create DocumentCookieOperation object naming mDocCookieOperation.
   3. netwerk/cookie/CookieServiceParent.cpp
      * Add observer on Constructor.
      * Create Observer for processing the notifies which already registered.
        * cookie-changed
        * private-cookie-changed
        * init-hash-table
        * destroy-hash-table
      * Remove observer on ActorDestroy()
   4. netwerk/cookie/CookieServiceParent.h
      * Inherit nsIObserver
      * Create DocumentCookieOperation object naming mDocCookieOperation.
   5. netwerk/cookie/nsCookieService.cpp
      * Create GetCookieStructList & GetCookieStructListInternal.
      * Remove GeBaseDomain & GetBaseDomainFromHost.
      * Create ConfirmMatchingCookie
      * Create new notify "init-hash-table" and "destroy-hash-table"
   6. netwerk/cookie/nsCookieService.h
      * Remove nsCookieKey class.
      * Remove mTLDService.
      * Create DocumentCookieOperation object naming mDocCookieOperation for calling GetBaseDomain.
   7. netwerk/cookie/moz.build
      * Add DocumentCookieOperation & nsCookieKey
   8. netwerk/ipc/NeckoParent.cpp
      * Use AddRef when calling CookieServiceParent Constructor
      * Use Release when calling CookieServiceParent Destructor

Would you give me your suggestions?
Thanks!

	Josh Matthews [:jdm]
	Comment 62 • 7 years ago

Comment on attachment 8862104 [details] [diff] [review]
implementation part1--create & set load flag.

Review of attachment 8862104 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ -1,1 @@
> -/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

nit: revert this.

@@ +8587,5 @@
>  
>  } // namespace
>  
> +void
> +nsDocShell::ConfirmSandBoxedForSettingLoadFlags(nsLoadFlags &aLoadFlags)

Let's call this `RequireCookiesIfNecessary`.

@@ +8599,5 @@
> +      sandboxFlags = doc->GetSandboxFlags();
> +    }
> +  }
> +
> +  if (sandboxFlags & SANDBOXED_ORIGIN) {

We need to set the LOAD_DOCUMENT_NEEDS_COOKIE flag if:
* there are no sandbox flags (ie. SANDBOXED_NONE), or
* there are sandbox flags, but both SANDBOXED_ORIGIN and SANDBOXED_SCRIPT are not present (ie. the sandboxed iframe is not using a unique origin, and it is allowed to execute JS)

::: dom/html/nsHTMLDocument.cpp
@@ +2410,5 @@
>      channel->GetLoadFlags(&loadFlags);
>      loadFlags |= nsIChannel::LOAD_DOCUMENT_URI;
> +    // If sandboxed flags included SANBOXED_ORIGIN,
> +    // add LOAD_DOCUMENT_NEEDS_COOKIE to load flag
> +    if (mSandboxFlags & SANDBOXED_ORIGIN) {

This check will need to match the one in nsDocShell.cpp.

::: netwerk/base/nsIRequest.idl
@@ +131,5 @@
>       */
>      const unsigned long LOAD_HTML_OBJECT_DATA = 1 << 1;
>  
> +    /**
> +     * TODO: add comment to explain the function of this flag.

"This flag marks the request as belonging to a document that requires access to the document.cookies API."

	Josh Matthews [:jdm]
	Comment 63 • 7 years ago

Testcases that we need to verify work after these changes:
* the response for a document provides cookies; the page can observe those cookies using document.cookies
* the response for a document provides cookies, including some that are httponly; the page cannot observe the httponly cookies using document.cookies
* a page sets document.cookies then immediately gets the value of document.cookies; the new cookies should be observed
* a page sets cookies, then loads a sandboxed iframe that does not use allow-same-origin but uses allow-scripts; document.cookies in the iframe should not observe the original cookies, and setting document.cookies in the iframe should not be observable in the original page
* a page sets cookies, then loads a sandboxed iframe that uses allow-same-origin; document.cookies in the iframe should observe the original cookies, and setting document.cookies in the iframe should be observable in the original page
* a page is loaded with no cookies present, then performs a same-origin XHR that provides a response with cookies; document.cookies should observe the new cookies after the XHR is complete
* a page is loaded and has cookies set, then loads a same-origin iframe with sets more cookies, then removes the iframe; all of the cookies that were set are still visible from the original page using document.cookies

Reading through these patches, I am concerned that many of these testcases look like they will fail in the current implementation.

	Josh Matthews [:jdm]
	Comment 64 • 7 years ago

One other question - does this patch pass tests on the tryserver right now?

	Amy Chung [:Amy] Assignee
	Comment 65 • 7 years ago

No, I don't push my patch to try server yet.
I will test my patch on try server after finish the test cases.
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 66 • 7 years ago

Hi Josh,
I have pushed my patch to try server, I found some problems as below:
1. CookieServiceChild didn’t verify some rules about the cookie can save to db when server sets cookies.
    i.  Separate the part of confirming the cookies which match the rules to other static public function in nsCookieService, maybe naming ConfirmCookieCanSave()
    ii.  Use CookieStruct to replace nsCookieAttributes
    iii. CookieServiceChild update the cookie to hash table after call ConfirmCookieCanSave(). 
2. CookieServiceChild didn’t verify the cookie whether expired when server gets cookies.
    i. Confirm the expiry time.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 67 • 7 years ago

Hi,
I have modified my patch from Josh's suggestions and pass the test cases.
My modification as below:
1. Rename ConfirmSandBoxedForSettingLoadFlags to RequireCookiesIfNecessary.
2. Modified the rules of setting LOAD_DOCUMENT_NEEDS_COOKIE.
3. Added the comment of LOAD_DOCUMENT_NEEDS_COOKIE on nsIRequest.idl.

Would you give me your suggestion?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 68 • 7 years ago

Hi,
I have tested my testing web page and modified my patch as below:
[DocumentCookieInfo]
1. Modified the field of DocumentCookieInfo() to CookieStruct.
2. Added a constructor on DocuemntCookieInfo().

[DocumentCookieOperation]
1. Added a OperationFlag.
2. Modified UpdateCookieToTable.
3. Modified DestroyCookieFromTable

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 69 • 7 years ago

Hi,
I changed this patch from part 3 to part 4, and the code doesn't modify anything.
The last modifications as below:
[HttpChannel]
   1. netwerk/protocol/http/HttpChannelChild.h
      * Add new argument cookiesList on RecvOnStartRequest()
   2. netwerk/protocol/http/HttpChannelChild.cpp
      * If cookiesList which in  RecvOnStartRequest() is not empty, call Init hash table from CookieServiceChild.
   3. netwerk/protocol/http/HttpChannelParent.cpp
      * Add a expression to confirm the cookies which is matching cookie.
   4. netwerk/protocol/http/PHttpChannel.ipdl
      * Add a argument cookiesList on OnStartRequest().

Would you give me your suggestions?

	Amy Chung [:Amy] Assignee
	Comment 70 • 7 years ago

Hi,
The modifications as below:
1. Moved the parts of Implement xpcom interface DestroyHashTableFromDocument to part3 patch.
2. docshell/base/nsDocShell.cpp
    * Call Destroy hash table when this object be released.
   dom/html/nsHTMLDocument.cpp
    * Call Destroy hash table when this object be released.

Would you give me your suggestion?
Thanks!

	Junior [inactive]
	Comment 71 • 7 years ago

Comment on attachment 8865288 [details] [diff] [review]
implementation part1 -- created loadflags.

Review of attachment 8865288 [details] [diff] [review]:
-----------------------------------------------------------------

You have an unnecessary warning message at the beginning of your patch.

::: docshell/base/nsDocShell.cpp
@@ +8601,5 @@
> +  }
> +
> +  if (sandboxFlags & SANDBOXED_NONE ||
> +      (! (sandboxFlags & SANDBOXED_ORIGIN) &&
> +       ! (sandboxFlags & SANDBOXED_SCRIPTS))) {

nit: no space after !

::: dom/html/nsHTMLDocument.cpp
@@ +2409,5 @@
>      nsLoadFlags loadFlags = 0;
>      channel->GetLoadFlags(&loadFlags);
>      loadFlags |= nsIChannel::LOAD_DOCUMENT_URI;
> +    // If sandboxed flags included SANBOXED_ORIGIN,
> +    // add LOAD_DOCUMENT_NEEDS_COOKIE to load flag

Change the annotation to match your code, or remove it if you think it is unnecessary.

@@ +2411,5 @@
>      loadFlags |= nsIChannel::LOAD_DOCUMENT_URI;
> +    // If sandboxed flags included SANBOXED_ORIGIN,
> +    // add LOAD_DOCUMENT_NEEDS_COOKIE to load flag
> +    if (mSandboxFlags & SANDBOXED_NONE ||
> +        (!(mSandboxFlags &SANDBOXED_ORIGIN) &&

nit: space after &

	Junior [inactive]
	Comment 72 • 7 years ago

Comment on attachment 8865290 [details] [diff] [review]
implementation part2 -- data struct

Review of attachment 8865290 [details] [diff] [review]:
-----------------------------------------------------------------

I'd like to see another round

::: netwerk/cookie/DocumentCookieOperation.cpp
@@ +72,5 @@
> +// 'co.uk', or the empty string, aBaseDomain will be the exact host, and a
> +// leading dot will be treated as an error.
> +nsresult
> +DocumentCookieOperation::GetBaseDomainFromHost(const nsACString &aHost,
> +                                         nsCString        &aBaseDomain)

nit: indent

@@ +85,5 @@
> +  // get the base domain. this will fail if the host contains a leading dot,
> +  // more than one trailing dot, or is otherwise malformed.
> +  nsresult rv = mTLDService->GetBaseDomainFromHost(Substring(aHost, domain), 0, aBaseDomain);
> +  if (rv == NS_ERROR_HOST_IS_IP_ADDRESS ||
> +    rv == NS_ERROR_INSUFFICIENT_DOMAIN_LEVELS) {

nit: indent

@@ +127,5 @@
> +                                                      nsAutoCString          &aCookieString)
> +{
> +  DocumentCookieInfoList *docCookieInfoList;
> +  DocumentCookieInfo *docCookieInfo;
> +  // Create nsCookieKey

remove this annotation

@@ +130,5 @@
> +  DocumentCookieInfo *docCookieInfo;
> +  // Create nsCookieKey
> +  nsCookieKey key(aBaseDomain, aOriginAttrs);
> +  aDocCookieMap.Get(key, &docCookieInfoList);
> +  if (docCookieInfoList) {

bail-out first,
i.e.,
if (!docCookieInfoList) {
  return true;
}

@@ +184,5 @@
> +          docCookieInfo->mCookieStruct.lastAccessed() = aCookieStruct.lastAccessed();
> +          docCookieInfo->mCookieStruct.creationTime() = aCookieStruct.creationTime();
> +          docCookieInfo->mCookieStruct.isSession() = aCookieStruct.isSession();
> +          docCookieInfo->mCookieStruct.isSecure() = aCookieStruct.isSecure();
> +          docCookieInfo->mCookieStruct.isHttpOnly() = aCookieStruct.isHttpOnly();

could we simply use operation= ?

@@ +216,5 @@
> +DocumentCookieOperation::DestroyCookieFromTable(DocumentCookieMap &aDocCookieMap,
> +                                                nsIURI            *aHostURI,
> +                                                const OriginAttributes  &aOriginAttrs)
> +{
> +  DocumentCookieInfoList *docCookieInfoList = nullptr;

sync the style for the initial behavior
I suggest set them to nullptr like this

@@ +227,5 @@
> +  nsCookieKey key(baseDomain, aOriginAttrs);
> +  aDocCookieMap.Get(key, &docCookieInfoList);
> +  if (docCookieInfoList) {
> +    for (uint32_t i = 0; i < docCookieInfoList->Length(); i++) {
> +      docCookieInfo = &(docCookieInfoList->ElementAt(i));

move the declaration here

@@ +240,5 @@
> +        }
> +      }
> +   }
> +  } else if (!docCookieInfoList) {
> +    return false;

bail-out first

@@ +251,5 @@
> +  }
> +  return true;
> +}
> +} //net
> +} //mozilla

nit: space after //

::: netwerk/cookie/DocumentCookieOperation.h
@@ +16,5 @@
> +{
> +  DocumentCookieInfo()
> +  {}
> +
> +  DocumentCookieInfo(CookieStruct aCookieStruct)

any reason to call-by-value?

@@ +27,5 @@
> +     mCookieStruct.lastAccessed() = aCookieStruct.lastAccessed();
> +     mCookieStruct.creationTime() = aCookieStruct.creationTime();
> +     mCookieStruct.isSession() = aCookieStruct.isSession();
> +     mCookieStruct.isSecure() = aCookieStruct.isSecure();
> +     mCookieStruct.isHttpOnly() = aCookieStruct.isHttpOnly();

could we simply use operation= ?

@@ +29,5 @@
> +     mCookieStruct.isSession() = aCookieStruct.isSession();
> +     mCookieStruct.isSecure() = aCookieStruct.isSecure();
> +     mCookieStruct.isHttpOnly() = aCookieStruct.isHttpOnly();
> +  }
> +public:

It's a struct, default public

@@ +45,5 @@
> +  nsCOMPtr<nsIEffectiveTLDService> mTLDService;
> +  DocumentCookieOperation();
> +  NS_INLINE_DECL_REFCOUNTING(DocumentCookieOperation);
> +  typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;
> +  typedef nsClassHashtable<nsCookieKey, DocumentCookieInfoList> DocumentCookieMap;

possible a line-break here
I had hard time to find the definition of DocumentCookieMap

@@ +48,5 @@
> +  typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;
> +  typedef nsClassHashtable<nsCookieKey, DocumentCookieInfoList> DocumentCookieMap;
> +  bool UpdateCookieToTable(DocumentCookieMap &aDocCookieMap, const CookieStruct &aCookieStruct, const OriginAttributes &aOriginAttrs, OperationFlag aOperationFlag);
> +  bool DestroyCookieFromTable(DocumentCookieMap &aDocCookieMap, nsIURI *aHostURI, const OriginAttributes &aOriginAttrs);
> +  nsresult GetCookieStringFromHashTable(DocumentCookieMap &aDocCookieMap, const nsCString aBaseDomain, const OriginAttributes &aOriginAttrs, nsAutoCString &aCookieString);

nit: line-break for those with length apparently larger than 80
nit: &aBaseDomain
nit: GetCookieStringFromTable

@@ +58,5 @@
> +};
> +} //net
> +} //mozilla
> +
> +#endif //DocumentCookieStruct_h

nit: space after // for the three

::: netwerk/cookie/nsCookieKey.h
@@ +4,5 @@
> +#include "nsTHashtable.h"
> +
> +namespace mozilla {
> +namespace net {
> +class nsCookieKey : public PLDHashEntryHdr

You move the code from nsCookieService.h
Keep all the change of movement (i.e, those removal of nsCookieKey in nsCookieService.h) in the same patch.

Also the #include "nsCookieKey.h" to make this patch able to pass the build

@@ +66,5 @@
> +
> +} // net
> +} // mozilla
> +
> +#endif //mozilla_net_nsCookieKey_h

nit: space after //

	Junior [inactive]
	Comment 73 • 7 years ago

Comment on attachment 8865291 [details] [diff] [review]
implementation part4 -- http channel

Review of attachment 8865291 [details] [diff] [review]:
-----------------------------------------------------------------

Generally good.
Put some nits first and hold the f+ until part 3 uploaded since I need to know the changes of nsCookieService

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +49,5 @@
>  #include "nsIDOMWindowUtils.h"
>  #include "nsIEventTarget.h"
>  #include "nsStreamUtils.h"
>  #include "nsThreadUtils.h"
> +#include "mozilla/net/DocumentCookieOperation.h"

I know it's not alphabetical now, but I'll suggest to align with those "mozilla/net/*"

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1228,5 @@
> +  // Confirm it's document.cookie
> +  if (newLoadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE && isDocument) {
> +    nsCOMPtr<nsIURI> newURI;
> +    chan->GetURI(getter_AddRefs(newURI));
> +    //Get nsCookieService

remove this

@@ +1233,5 @@
> +    RefPtr<nsCookieService> cs = static_cast<nsCookieService *>(mHttpHandler->GetCookieService());
> +    NS_ASSERTION(mCookieService, "couldn't get nsICookieService");
> +    OriginAttributes attrs;
> +    NS_GetOriginAttributes(chan, attrs);
> +    nsCOMPtr<mozIThirdPartyUtil>  thirdPartyUtil;

nit: one space

@@ +1245,5 @@
> +    // Confirm the cookies which match the conditions to save to hash table.
> +    for (uint32_t i = 0; i < cookiesList.Length(); i++) {
> +      if (cs->ConfirmMatchingCookie(nullptr, newURI, &(cookiesList[i]), chan)) {
> +         if (mIPCClosed) {
> +           return NS_ERROR_UNEXPECTED;

move the logic above the loop

	Josh Matthews [:jdm]
	Comment 74 • 7 years ago

Comment on attachment 8865288 [details] [diff] [review]
implementation part1 -- created loadflags.

Review of attachment 8865288 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +8599,5 @@
> +      sandboxFlags = doc->GetSandboxFlags();
> +    }
> +  }
> +
> +  if (sandboxFlags & SANDBOXED_NONE ||

SANDBOXED_NONE is 0x0, so this condition will never be true. I guess it's unnecessary, since the other conditions will be true if there are no sandboxing flags present.

::: dom/html/nsHTMLDocument.cpp
@@ +2410,5 @@
>      channel->GetLoadFlags(&loadFlags);
>      loadFlags |= nsIChannel::LOAD_DOCUMENT_URI;
> +    // If sandboxed flags included SANBOXED_ORIGIN,
> +    // add LOAD_DOCUMENT_NEEDS_COOKIE to load flag
> +    if (mSandboxFlags & SANDBOXED_NONE ||

Could we add a function that encapsulates this check that both this code and nsDocShell could use?

	Josh Matthews [:jdm]
	Comment 75 • 7 years ago

Comment on attachment 8865290 [details] [diff] [review]
implementation part2 -- data struct

Review of attachment 8865290 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/DocumentCookieOperation.h
@@ +31,5 @@
> +     mCookieStruct.isHttpOnly() = aCookieStruct.isHttpOnly();
> +  }
> +public:
> +  CookieStruct mCookieStruct;
> +  nsrefcnt mRefDocCnt;

This seems like we're merging two different concepts here. A CookieStruct represents a single cookie. The reference count is supposed to be the count of the number of documents that need access to all of the cookies for a particular domain. I would not expect both of these values to be part of the same struct.

	Josh Matthews [:jdm]
	Comment 76 • 7 years ago

Comment on attachment 8865291 [details] [diff] [review]
implementation part4 -- http channel

Review of attachment 8865291 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1240,5 @@
> +    thirdPartyUtil->IsThirdPartyChannel(chan, newURI, &isForeign);
> +    bool isPrivate = chan && NS_UsePrivateBrowsing(chan);
> +    nsTArray<CookieStruct> cookiesList;
> +    // Get cookie struct list
> +    cs->GetCookieStructList(newURI, isForeign, false, attrs, isPrivate, cookiesList);

I suspect that this implementation does not properly separate private and non-private cookies. I can't confirm that right now because the changes to the cookie service are missing.

	Josh Matthews [:jdm]
	Comment 77 • 7 years ago

Comment on attachment 8865293 [details] [diff] [review]
implementation part5 -- Destroy cookie

Review of attachment 8865293 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/nsHTMLDocument.cpp
@@ +189,5 @@
>  nsHTMLDocument::~nsHTMLDocument()
>  {
> +  nsCOMPtr<nsICookieService> service = do_GetService(NS_COOKIESERVICE_CONTRACTID);
> +  if (service && mDocumentURI && mChannel) {
> +    service->DestroyHashTableFromDocument(mDocumentURI, mChannel);

We probably need to call this with mOriginalURI too. We should have a testcase that uses pushState() so that the URLs are different; when the document is destroyed then it should still end up removing the reference count for the appropriate cookie domain.

	Amy Chung [:Amy] Assignee
	Comment 78 • 7 years ago

Hi,
This test cases can divide to four parts as below:
1. document.cookie
   * Created observer, and added "cookie-changed" to observer.
   * Set cookie by "document.cookie"; the page can observe those cookies using document.cookies
   * Set cookie which includes "httponly" by "document.cookie"; the page cannot observe the httponly cookies using document.cookies.
   * Set document.cookies then immediately gets the value of document.cookies; the new cookies should be observed.
2. xhr
   * A page is loaded with no cookies present, then performs a same-origin XHR that provides a response with cookies; document.cookies should observe the new cookies after the XHR is complete.
3. iframe
   * Load a sandboxed iframe which does not use allow-same-origin but uses allow-scripts; document.cookies in the iframe should not observe.
   * Load a sandboxed iframe that uses allow-same-origin; document.cookies in the iframe should observe.
   * Loads a same-origin iframe with sets more cookies, then removes the iframe; all of the cookies that were set are still visible from the original page using document.cookies.
4. clear all cookies
   * Confirmed the observer can get the deleted notify when clearing all cookies.

Would you give me your suggestions?
Thanks!

	Junior [inactive]
	Comment 79 • 7 years ago

Comment on attachment 8865993 [details] [diff] [review]
test case -- part6

Review of attachment 8865993 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not through all the patch.
f- because of lack of decent comment and not covering all the cases in comment 63

::: netwerk/cookie/test/browser/browser.ini
@@ +6,5 @@
> +  test_1331680.html
> +  test_1331680_iframe.html
> +  user_agent.sjs
> +  test_1331680_xhr.html
> +  test_1331680_clear_cookies.html

alphabetical

::: netwerk/cookie/test/browser/browser_1331680.js
@@ +13,5 @@
> +  "http://example.com/browser/netwerk/cookie/test/browser/test_1331680_iframe.html";
> +const TEST_CLEAR_COOKIES_URL =
> +  "http://example.com/browser/netwerk/cookie/test/browser/test_1331680_clear_cookies.html";
> +
> +let cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);

is it unused?

@@ +16,5 @@
> +
> +let cm = Cc["@mozilla.org/cookiemanager;1"].getService(Ci.nsICookieManager2);
> +var testNum = 0;
> +var numOfCookies = 0;
> +var testItem;

nit: I suggest g- prefix for these global variables.
e.g., gTestNum

@@ +107,5 @@
> +    }
> +  }
> +};
> +
> +// opens `uri' in a new tab with the provided userContextId and focuses it.

Fix the annotation since you remove the userContextId parameter.
Also the function name and the following annotation

@@ +128,5 @@
> +function* test_set_cookie() {
> +  let {tab, browser} = yield* openTabInUserContext(TEST_URL);
> +  gBrowser.removeTab(tab);
> +}
> +

So, IIUC
The flow is
1) load the TEST_URL to the new tab
2) set document.cookie in TEST_URL
3) observed "cookie-changed" and check the cookie
4) close the tab and continue to next tab

Is it guarantee that 4) will triggered after 3)?
i.e., cookie-changed is sent sync

If not, I'm afraid of intermittent since the order would be changed.

@@ +132,5 @@
> +
> +function* test_xhr() {
> +  let {tab, browser} = yield* openTabInUserContext(TEST_XHR_URL);
> +  gBrowser.removeTab(tab);
> +}

Also, after the TEST_XHR_URL is loaded, the xhr is sent.
However, response is possibly on-the-air and close the tab.

@@ +137,5 @@
> +
> +function* confirm_iframe_cookies(cookieString, cookieName) {
> +  var if_name = cookieName + "=";
> +  var ca = cookieString.split(';');
> +  for(var i = 0; i <ca.length; i++) {

nit: space after |for| and <

@@ +168,5 @@
> +  gBrowser.removeTab(tab);
> +}
> +
> +add_task(function* test() {
> +  waitForExplicitFinish();

Seems |yield| already doing this.

@@ +169,5 @@
> +}
> +
> +add_task(function* test() {
> +  waitForExplicitFinish();
> +  // Add observer

Remove this

@@ +172,5 @@
> +  waitForExplicitFinish();
> +  // Add observer
> +  Services.obs.addObserver(observer, "cookie-changed");
> +
> +  // Start to test setting cookie.

The annotation should be informative.
And it's not the case for
 "the response for a document provides cookies; the page can observe those cookies using document.cookies"
The cookie here is from document.cookie instead of HTTP response

This annotation rule also applied to the following annotations.
I'd like to understand what you want to test after I read the annotation.

@@ +176,5 @@
> +  // Start to test setting cookie.
> +  testItem = TestItemsEnum.SetCookie;
> +  yield test_set_cookie();
> +
> +  // Start to test XHR ( set response header)

nit: no space after (

@@ +182,5 @@
> +  yield test_xhr();
> +
> +  // Start to test setting iframes.
> +  testItem = TestItemsEnum.Iframe;
> +  // Rest the number of tests.

ambiguous comment

@@ +192,5 @@
> +  testNum = 0;
> +  yield clear_cookies();
> +
> +  is(numOfCookies, 0, "Clear all cookies");
> +  // Remove observer.

remove this

::: netwerk/cookie/test/browser/test_1331680.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=643051

nit: change to correct bug number

@@ +8,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1331680">Mozilla Bug 643051</a>

same here

::: netwerk/cookie/test/browser/test_1331680_iframe.html
@@ +1,4 @@
> +<!DOCTYPE HTML>
> +<html>
> +<!--
> +https://bugzilla.mozilla.org/show_bug.cgi?id=643051

same here

@@ +8,5 @@
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<body>
> +<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1331680">Mozilla Bug 643051</a>

same here

@@ +14,5 @@
> +<div id="content" style="display: none">
> +<script type="application/javascript">
> +const ID = ["if_1", "if_2"];
> +function create_iframe(id, src, sandbox_flags) {
> +   var iframeEl = null;

nit: two space indentation for this file

::: netwerk/cookie/test/browser/user_agent.sjs
@@ +1,4 @@
> +
> +function handleRequest(request, response)
> +{
> +    // avoid confusing cache behaviors

same here

	Junior [inactive]
	Comment 80 • 7 years ago

Comment on attachment 8865293 [details] [diff] [review]
implementation part5 -- Destroy cookie

Review of attachment 8865293 [details] [diff] [review]:
-----------------------------------------------------------------

Suspend the feedback process for the similar reason with comment 73

::: docshell/base/nsDocShell.cpp
@@ +5794,5 @@
>                 "Unexpected item type in docshell");
>  
> +  nsCOMPtr<nsICookieService> service = do_GetService(NS_COOKIESERVICE_CONTRACTID);
> +  nsIChannel *channel = GetCurrentDocChannel();
> +  if (service && mLoadingURI && channel ) {

nit: no space before )

	Amy Chung [:Amy] Assignee
	Comment 81 • 7 years ago

Hi,
I have modified my patch from your suggestions as below:
1. Fixed nit.
2. Set RequireCookiesIfNecessary() to static for nsHTMLDocument can use.

Would you review my patch?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 82 • 7 years ago

Hi,
I have modified this patch from your suggestions as below:
1. Fixed nit.
2. Removed nsCookieKey from nsCookieService.h on this patch.
3. Create a structure naming DocumentCookieListInfo which includes DocumentCookieInfoList and RefCount.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 83 • 7 years ago

Hi,
This patch is WIP patch, hope this patch can help you to review the part 4 patch clearer.
[Finish]
Confirm the setting rules of cookie on child.

[To Do]
Fixed fail tests on try server.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ca9c855c71cf0479346fe22109a24d2068fce0c&selectedJob=97672928

	Junior [inactive]
	Comment 84 • 7 years ago

Comment on attachment 8866439 [details] [diff] [review]
implementation part1 -- created loadflags.

Review of attachment 8866439 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me. Thanks!

::: docshell/base/nsDocShell.cpp
@@ +8599,5 @@
>  
> +void
> +nsDocShell::RequireCookiesIfNecessary(nsLoadFlags      &aLoadFlags,
> +                                      uint32_t         &aSandboxFlags,
> +                                      nsIContentViewer *aContentViewer)

const

	Josh Matthews [:jdm]
	Comment 85 • 7 years ago

Comment on attachment 8865993 [details] [diff] [review]
test case -- part6

Review of attachment 8865993 [details] [diff] [review]:
-----------------------------------------------------------------

This test is currently relying on observing cookie notifications that occur in the parent process. I've left comments about places where we need to verify that the effects are visible in the content process too.

::: netwerk/cookie/test/browser/browser_1331680.js
@@ +11,5 @@
> +  "http://example.com/browser/netwerk/cookie/test/browser/test_1331680_xhr.html";
> +const TEST_IFRAME_URL =
> +  "http://example.com/browser/netwerk/cookie/test/browser/test_1331680_iframe.html";
> +const TEST_CLEAR_COOKIES_URL =
> +  "http://example.com/browser/netwerk/cookie/test/browser/test_1331680_clear_cookies.html";

This file is missing.

@@ +21,5 @@
> +
> +function confirm_cookie(isUpdate, data, cookieName, confirmName) {
> +  if (isUpdate) {
> +    numOfCookies++;
> +    ok(true, data == "added");

is(data, "added");

@@ +23,5 @@
> +  if (isUpdate) {
> +    numOfCookies++;
> +    ok(true, data == "added");
> +  } else {
> +    if (numOfCookies > 0)

ok(numCookies > 0);

@@ +25,5 @@
> +    ok(true, data == "added");
> +  } else {
> +    if (numOfCookies > 0)
> +      numOfCookies--;
> +    ok(true, data == "deleted");

is(data, "deleted");

@@ +48,5 @@
> +}
> +
> +function result_iframe_cookie(data, cookie) {
> +  switch (testNum) {
> +    case 0:

I don't understand why we don't observe the first cookie being set. As far as I know, the cookie service should still be able to observe it.

::: netwerk/cookie/test/browser/file_iframe_allow_same_origin.html
@@ +1,5 @@
> +<!DOCTYPE html>
> +<html>
> +<script>
> +document.cookie = "if2_1=123";
> +document.cookie = "if2_2=123";

We should verify that document.cookie can observe these new cookies and communicate that to browser_1331680.js somehow.

::: netwerk/cookie/test/browser/file_iframe_allow_scripts.html
@@ +1,4 @@
> +<!DOCTYPE html>
> +<html>
> +<script>
> +document.cookie = "if1=123";

We should verify that document.cookie can observe this change and communicate that back to browser_1331680.js somehow.

::: netwerk/cookie/test/browser/test_1331680_iframe.html
@@ +15,5 @@
> +<script type="application/javascript">
> +const ID = ["if_1", "if_2"];
> +function create_iframe(id, src, sandbox_flags) {
> +   var iframeEl = null;
> +   iframeEl      = document.createElement("iframe");

Let's merge this with the previous line.

@@ +27,5 @@
> +function delete_iframes(id) {
> +   var ifr = document.getElementById(id);
> +   ifr.onload = function() {
> +      ifr.parentNode.removeChild(ifr);
> +      delete window.frames.id;

What is this delete supposed to do?

::: netwerk/cookie/test/browser/test_1331680_xhr.html
@@ +10,5 @@
> +<script type="text/javascript">
> +  var xhr = new XMLHttpRequest();
> +  xhr.open("GET", 'user_agent.sjs ', false); // sync request
> +  xhr.send();
> +</script>

We need to verify that document.cookie can see the cookie that was set in the response for the XHR. We will also need to communicate that result back to browser_1331680.js somehow.

::: netwerk/cookie/test/browser/user_agent.sjs
@@ +3,5 @@
> +{
> +    // avoid confusing cache behaviors
> +    response.setHeader("Cache-Control", "no-cache", false);
> +    response.setHeader("Content-Type", "text/html", false);
> +    response.setHeader("Set-Cookie", "test=123", false);

We should set a second cookie that is httponly as part of this test, and verify that test_1331680_xhr.html cannot observe it through document.cookies.

@@ +15,5 @@
> +            nav: navigator.userAgent\
> +          };\
> +          self.parent.postMessage(msg, '*');\
> +        </script>\
> +      </body></html>"

All of the bits that are copied from the previous file should be removed, since they are confusing when trying to understand this test.

	Josh Matthews [:jdm]
	Comment 86 • 7 years ago

Comment on attachment 8866439 [details] [diff] [review]
implementation part1 -- created loadflags.

Review of attachment 8866439 [details] [diff] [review]:
-----------------------------------------------------------------

::: docshell/base/nsDocShell.cpp
@@ +8604,5 @@
> +{
> +  if (aContentViewer) {
> +    nsCOMPtr<nsIDocument> doc = aContentViewer->GetDocument();
> +    if (doc) {
> +      aSandboxFlags = doc->GetSandboxFlags();

This overrides the sandbox flags from nsDocShell, which is scary.

@@ +11496,5 @@
>    (void)aChannel->GetLoadFlags(&loadFlags);
>    loadFlags |= nsIChannel::LOAD_DOCUMENT_URI |
>                 nsIChannel::LOAD_CALL_CONTENT_SNIFFERS;
>  
> +  RequireCookiesIfNecessary(loadFlags, mSandboxFlags, mContentViewer);

I propose we do this instead:
if (SandboxFlagsImplyCookies(mSandboxFlags)) {
    loadFlags |= nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE;
}

where SandboxFlagsImplyCookies is just a small function that checks SANDBOXED_ORIGIN and SANDBOXED_SCRIPTS flags and returns a boolean.

	Junior [inactive]
	Comment 87 • 7 years ago

Comment on attachment 8866442 [details] [diff] [review]
implementation part2 -- data struct

Review of attachment 8866442 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cookie/DocumentCookieOperation.cpp
@@ +1,3 @@
> +#include "DocumentCookieOperation.h"
> +#include "nsIEffectiveTLDService.h"
> +#include "nsCookieService.h"

Do we need nsCookieService here?

@@ +3,5 @@
> +#include "nsCookieService.h"
> +
> +using mozilla::OriginAttributes;
> +
> +

nit: one empty line only

@@ +168,5 @@
> +{
> +  DocumentCookieListInfo *docCookieListInfo = nullptr;
> +  DocumentCookieInfo *docCookieInfo = nullptr;
> +
> +  // Create nsCookieKey

remove this

@@ +226,5 @@
> +}
> +
> +bool
> +DocumentCookieOperation::DestroyCookieFromTable(DocumentCookieMap &aDocCookieMap,
> +                                                nsIURI            *aHostURI,

const

@@ +234,5 @@
> +  DocumentCookieInfo *docCookieInfo = nullptr;
> +  bool requireHostMatch;
> +  nsCString baseDomain;
> +  GetBaseDomain(aHostURI, baseDomain, requireHostMatch);
> +  // Create nsCookieKey

nit: remove this

@@ +239,5 @@
> +  nsCookieKey key(baseDomain, aOriginAttrs);
> +  aDocCookieMap.Get(key, &docCookieListInfo);
> +  if (!docCookieListInfo) {
> +    return false;
> +  } else {

no need for |else|

::: netwerk/cookie/DocumentCookieOperation.h
@@ +5,5 @@
> +#include "nsHashKeys.h"
> +#include "nsTHashtable.h"
> +#include "nsCookieKey.h"
> +#include "nsAutoPtr.h"
> +#include "mozilla/net/NeckoChannelParams.h"

alphabetical please

@@ +17,5 @@
> +{
> +  DocumentCookieInfo()
> +  {}
> +
> +  DocumentCookieInfo(CookieStruct aCookieStruct)

|&aCookieStruct| if aCookieStruct is not possible to be nullptr
Sorry for not asking this before, is it able to use initialization list?
i.e., 
:mCookieStruct(aCookieStruct)

@@ +40,5 @@
> +  typedef nsTArray<DocumentCookieInfo> DocumentCookieInfoList;
> +  typedef
> +  nsClassHashtable<nsCookieKey, DocumentCookieListInfo> DocumentCookieMap;
> +
> +

nit: one empty line

@@ +52,5 @@
> +  DestroyCookieFromTable(DocumentCookieMap &aDocCookieMap, nsIURI *aHostURI,
> +                         const OriginAttributes &aOriginAttrs);
> +
> +  nsresult
> +  GetCookieStringFromHashTable(DocumentCookieMap &aDocCookieMap,

1. Any reason to use |HashTable| name here and |Table| in the previous two?
Make the same style if possible.

2. make the first argument const

@@ +66,5 @@
> +
> +  nsresult Init();
> +
> +protected:
> +  nsCOMPtr<nsIEffectiveTLDService> mTLDService;

I don't think of a reason to make it |protected|.
Correct me if I'm wrong

::: netwerk/cookie/nsCookieKey.h
@@ +35,5 @@
> +
> +  bool KeyEquals(KeyTypePointer other) const
> +  {
> +    return mBaseDomain == other->mBaseDomain &&
> +    mOriginAttributes == other->mOriginAttributes;

indentation

@@ +68,5 @@
> +} // net
> +} // mozilla
> +
> +#endif //mozilla_net_nsCookieKey_h
> +

nit: remove this empty line
nit: space after //

::: netwerk/ipc/NeckoChannelParams.ipdlh
@@ +189,5 @@
>    PFTPChannel;
>  };
>  
> +// For OnStartRequest Parent -> Child
> +

nit: remove the empty line

	Amy Chung [:Amy] Assignee
	Comment 88 • 7 years ago

Hi,
I have modified this patch from your suggestions as below:
1. Fixed nit.
2. Removed isPrivate, because the latest nsCookieService already removed the Private checking.
   The patch of removing Private checking as below:
https://bug1284579.bmoattachments.org/attachment.cgi?id=8864007

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 89 • 7 years ago

(In reply to Josh Matthews [:jdm] from comment #77)
> Comment on attachment 8865293 [details] [diff] [review]
> implementation part5 -- Destroy cookie
> 
> Review of attachment 8865293 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/html/nsHTMLDocument.cpp
> @@ +189,5 @@
> >  nsHTMLDocument::~nsHTMLDocument()
> >  {
> > +  nsCOMPtr<nsICookieService> service = do_GetService(NS_COOKIESERVICE_CONTRACTID);
> > +  if (service && mDocumentURI && mChannel) {
> > +    service->DestroyHashTableFromDocument(mDocumentURI, mChannel);
> 
> We probably need to call this with mOriginalURI too. We should have a
> testcase that uses pushState() so that the URLs are different; when the
> document is destroyed then it should still end up removing the reference
> count for the appropriate cookie domain.

Thanks, I will add a checking of OriginURI on this destructor and create a new test case for testing destroy function.

	Junior [inactive]
	Comment 90 • 7 years ago

Comment on attachment 8866619 [details] [diff] [review]
implementation part4 -- http channel

Review of attachment 8866619 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +49,5 @@
>  #include "nsIDOMWindowUtils.h"
>  #include "nsIEventTarget.h"
>  #include "nsStreamUtils.h"
>  #include "nsThreadUtils.h"
> +#include "mozilla/net/DocumentCookieOperation.h"

move this under |#include "mozilla/net/DNS.h"|

::: netwerk/protocol/http/HttpChannelParent.cpp
@@ +1225,5 @@
> +  mChannel->GetLoadFlags(&newLoadFlags);
> +  bool isDocument = false;
> +  chan->GetIsDocument(&isDocument);
> +  // Confirm it's document.cookie
> +  if (newLoadFlags & nsIRequest::LOAD_DOCUMENT_NEEDS_COOKIE && isDocument) {

it's a little crowded in the following code snippet.
Please add appropriate line break

@@ +1240,5 @@
> +    nsTArray<CookieStruct> cookiesList;
> +    // Get cookie struct list
> +    cs->GetCookieStructList(newURI, isForeign, false, attrs, cookiesList);
> +    // Confirm the cookies which match the conditions to save to hash table.
> +    if (!mIPCClosed) {

is it expected of not calling |requestHead->Exit();|?

	Josh Matthews [:jdm]
	Comment 91 • 7 years ago

Sorry for the silence. I spent a bunch of time reading through the patches (especially part 2 and part 4) and re-reading comment 14 and comment 26. I then asked Ehsan and Michael Layzell a bunch of questions, and we realized that we need to make some changes to the design. I am going to explain the design that should address the problems; I am in the middle of writing it up and making sure I understand all the complicated parts before proposing that you implement it.

	Amy Chung [:Amy] Assignee
	Comment 92 • 7 years ago

(In reply to Josh Matthews [:jdm] from comment #91)
> Sorry for the silence. I spent a bunch of time reading through the patches
> (especially part 2 and part 4) and re-reading comment 14 and comment 26. I
> then asked Ehsan and Michael Layzell a bunch of questions, and we realized
> that we need to make some changes to the design. I am going to explain the
> design that should address the problems; I am in the middle of writing it up
> and making sure I understand all the complicated parts before proposing that
> you implement it.

Thanks your help!
I have finished the test cases from comment 63 and fixed part2 and part3 patch.
I will upload the patches first, thanks!

	Amy Chung [:Amy] Assignee
	Comment 93 • 7 years ago

Hi,
I have modified as below:
1. Fixed the nits.
2. Added a variable names docListExist for deciding the DocumentCookieListInfo already puts to DocumentCookieMap or not.
3. Modified the function name GetCookieStringFromHashTable to GetCookieStringFromTable.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 94 • 7 years ago

Hi,
I have modified this patch as below:
1. Modified the RequireCookiesIfNecessary to SandboxFlagsImplyCookies.
   i. Modified the arguments of this function to uint32_t aSandboxFlags.
   ii.Return boolean value when mSandboxFlags not set to SANDBOXED_ORIGIN and
      SANDBOXED_SCRIPTS, return true.

Would you help me to review my patch, thanks!

	Amy Chung [:Amy] Assignee
	Comment 95 • 7 years ago

I have modified the WIP patch as below:
1. Added a new variable which names IsFromHttp on nsCookie.h.
   i.  Created a setting function names SetFromHttp().
   ii. Created a getting function names IsFromHttp().
2. Called nsCookie->SetFromHttp(aFromHttp) before the cookie which have to save to DB.
3. If the cookie which is from http, the CookieServiceParent have to notify the msg about the cookie must set to hash table to child.

[ToDo]
Fix the fail tests on try server.

	Amy Chung [:Amy] Assignee
	Comment 96 • 7 years ago

Hi,
I have fixed the patch as below:
1. Fixed nits.
2. Added "requestHead->Exit();" when the mIPCClosed is true.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 97 • 7 years ago

Hi,
I have modified the patch as below:
1. Added a condition "mOriginalURI" when nsHTMLDocument calls destructor.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 98 • 7 years ago

Hi,
Sorry for the wrong patch file.
I have modified this patch as below:
1. Modified the RequireCookiesIfNecessary to SandboxFlagsImplyCookies.
   i. Modified the arguments of this function to uint32_t aSandboxFlags.
   ii.Return boolean value when mSandboxFlags not set to SANDBOXED_ORIGIN and
      SANDBOXED_SCRIPTS, return true.

Would you help me to review my patch, thanks!

	Amy Chung [:Amy] Assignee
	Comment 99 • 7 years ago

Hi,
[Modification]
I have modified the patch as below:
1. Modified browser mochitest to general mochtest.
2. Added a communication between the iframe (id=if2) and test_1331680.html.
   i.  Used postMessage on file_iframe_allow_same_origin.html.
   ii. Set Listener on test_1331680.html.
3. Used the "document.cookie" to compare the cookie string which be get from file_1331680.js on test_1331680.html after called xhr.
   i.  file_1331680.js be called by loadChromeScirpt().
   ii. This compare can confirm the cookie strings be get from parent and child are same.

[Can't Modify]
1. If sandbox flags of iframe only set to allow_scripts, can't set cookie.
   Because the domain of iframe is not same to parent page, the behavior of set cookie contravenes the same-origin policy.
2. The XHR can set the cookie which set http-only, because the cookie set cookie through SetCookieStringFromHttp() in parent process.

[To-Do]
1. Have to create a test case for destroy function.

Would you give me your suggestions?
Thanks!

	Josh Matthews [:jdm]
	Comment 100 • 7 years ago

High level:

We want to remove the need for the content process to retrieve cookies from the parent process synchronously. We also do not want to duplicate all known cookies in each content process, because this is a privacy risk and it is unnecessary memory usage. We choose to send only the cookies that are accessible to a document right before that document is loaded. In order to avoid sending duplicate cookies we also track what documents are loaded in the parent process, so we only send cookies to the child when the child is loading a document and does not have cookies for that document's domain yet.

To simplify the model, the child process will need to notify the parent when a document is destroyed (nsDocument::Destroy), as well as when a document is created in a way that does not interact with the parent (such as about:blank (which inherits its principal from the document which created it, blob URIs) and navigations intercepted by a ServiceWorker). The parent will track the set of active domains for a particular content process, and when there are no documents in existence for a known domain the parent will instruct the content process to remove all cookies for that domain. 


Now, the low-level details:

We need two separate data structures; one for the child, and one for both the parent and child. The child will store a hashtable keyed on nsCookieKey with nsTArray<RefPtr<nsCookie>> values; this will be a member of CookieServiceChild.

The other data structure is a hashtable keyed on a string and stores nsTArray<DocumentCookieInfo> values, where DocumentCookieInfo looks like:
struct DocumentCookieInfo {
  nsCString mHostName;
  nsCString mPathName;
  bool mSecure;
  bool mCookiesPresent;
  nsrefcnt mActiveDocuments;
};

This second hashtable is a map of base domains, where each array of values is the list of documents loaded in the content process with a particular URL path. If the same document is loaded multiple times (ie. same base domain, and an entry exists in the array that contains identical mHostName, mSecure and mPathName values), the mActiveDocuments member will count how many instances of that document exist. This second hashtable will exist as a member in both CookieServiceParent and CookieServiceChild.

This means if a content process loads:
1) http://google.com/index.html
2) http://docs.google.com/
3) http://docs.google.com/
4) https://docs.google.com/
5) http://docs.google.com/error.html
6) http://developers.facebook.com/dev/welcome.html

then the hashtable would look like this:

{
"google.com" => [
  ("google.com", "index.html", false, 1),
  ("docs.google.com", "", false, 2),
  ("docs.google.com". "", true, 1),
  ("docs.google.com", "error.html”, false, 1)
],
"facebook.com" => [
  ("developers.facebook.com", "dev/welcome.html", false, 1)
],
}

Instead of HttpChannelParent::OnStartRequest, we will add the code that sends cookies to ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild. This code should use the ManagedPCookieServiceParent() API to retrieve the appropriate cookie service parent and call a method on that. Using the principal that is available in AboutToLoadHttpFtpWyciwygDocumentForChild, this CookieServiceParent method should:
* check if the channel is a document channel and has the LOAD_DOCUMENT_COOKIES load flag present
* check if there is a matching entry in the known documents hashtable
* if there is a matching entry, check if there is a matching DocumentCookieInfo structure for that entry
* if there is a matching structure, increment the number of active documents, and send a message to the content process with the principal, and no cookies.
* otherwise, add a new DocumentCookieInfo value to that entry, with an initial count of 1
* otherwise if there is no matching entry, insert a new hashtable entry containing a single DocumentCookieInfo value for the new document with an initial count of 1
* if a new value was added to the hashtable (either as part of a new entry or to an existing entry), retrieve a list of cookies that match the principal and send them to the child along with the principal using PCookieService. Cookies match the principal if the rules given in step 2 of comment 14 apply.

NOTE(mrl): We need to send a message even if the content process already has the cookies to make sure that the number of active documents is incremented.

When CookieServiceChild receives the message from the parent containing cookies and the document principal:
* check if there is a matching entry in the known documents hashtable
* if there is a matching entry, check if there is a matching DocumentCookieInfo structure for that entry
* if there is a matching structure, and mCookiesPresent is true, increment the number of active documents
* if there is a matching structure, and mCookiesPresent is false, set mCookiesPresent to true
* otherwise, add a new DocumentCookieInfo value to that entry, with an initial count of 1 and mCookiesPresent=true
* otherwise, if there is no matching entry, insert a new hash table entry containing a single DocumentCookieInfo value for the new document with an initial count of 1 and mCookiesPresent=true
* create an nsCookieKey from the principal, then create nsCookie values from the IPDL cookie values and add all of the provided cookies to the cookies hashtable, overwriting any existing matching cookies

In order to convert a principal into a hashtable string key, we should use the following steps:
* start with the principal's baseDomain 
* append the principal's originSuffix

When nsDocument::SetPrincipal is called, we need to do the following if we are running in the content process:
* if mChannel QIs to nsIHttpChannelInternal and nsIHttpChannelInternal::GetResponseSynthesized returns false, then 
   * Assert that a matching entry exists in the known document hashtable, with an mActiveDocuments value of at least 1
   * skip the remaining steps
* Otherwise call a method on CookieServiceChild that:
  * sends an IPDL message announcing a new document load, including the document's principal. This should be able to share code with the AboutToLoadHttpFtpWyciwygDocumentForChild codepath.
  * follow the same steps as when CookieServiceChild receives a message from the parent containing cookies - we need to update the known document hashtable with the new document. However, the new hashtable value should contain mCookiesPresent=false.

When nsDocument::Destroy is called and we are running in the content process, call a method on CookieServiceChild:
* retrieve an entry from the document info hashtable based on the document’s principal. Assert that this entry is present.
* using this entry, find values that match the document’s URL and original URL (if they are different)
* if the number of active documents is >1, decrease the count by 1
* otherwise, remove that DocumentCookieInfo value from the hashtable entry and purge appropriate cookies for that domain (see steps below)
* send an IPDL message to announce a document being destroyed, including the document's principal and the document’s URL and original URL (if different)
  * when the cookie service parent receives this message, it should find an existing entry in the document cookie info map using the principal from the message
  * using this entry, find values that match the URLs provided
  * if the value’s count is >1, it should be decreased by one. Otherwise the DocumentCookieInfo value should be removed.

In order to purge appropriate cookies for a domain:
* for each of the cookies that exist for the given domain
* for each value in the document hashtable entry for the given domain
* if the cookie path-matches against the value’s path, ignore the cookie
* if the cookie does not path-match against any of the known document paths, remove the cookie

In order for the content process to see updates to the cookie store after the initial response:
* make CookieServiceParent observe the cookie-changed and private-cookie-changed observer notifications
* when this notification occurs, compare the host and path of the modified cookie against the known document hashtable
* if there is no entry for the cookie’s host, ignore the cookie
* if the cookie does not domain-match or does not path-match against one of the DocumentCookieInfo values, ignore the cookie
* otherwise, send the cookie update information to the CookieServiceChild

When CookieServiceChild receives a cookie update:
* if there is no entry for the cookie’s host in the document hashtable, ignore the cookie
* if the cookie does not domain-match or does not path-match against one of the DocumentCookieInfo values, ignore the cookie
* otherwise, modify the stored cookie according to the update that was received

When CookieServiceChild::GetCookieStringInternal is invoked:
* if the network.cookie.ipc.sync preference (should default to false) is true, use the existing synchronous IPDL code path and return the result
* assert there is an entry in the known documents hashtable for the host URI
* if the entry has mCookiesPresent=false, use the existing synchronous IPDL code path and return the result
* otherwise, create an nsCookieKey value from the host URI and channel’s origin attributes and retrieve the set of cookies that need to be serialized

Since nsCookieService::AddInternal and nsCookieService::SetCookieInternal can fail in many interesting ways, we need to make sure that we cannot store cookies in the child that the parent will reject. We must move all of the checks in SetCookieInternal and AddInternal (except for the eviction code, which only makes in the parent) into code that can be shared between CookieServiceChild and nsCookieService. The cases that log a failure and immediately return without updating the cookie storage are the dangerous ones.

When CookieServiceChild::SetCookieStringInternal is invoked:
* if the network.cookie.ipc.sync preference is true, use the existing synchronous IPDL code path and return the result
* assert there is an entry in the known documents hashtable for the host URI
* if the entry in the known documents hashtable for the host URI has mCookiesPresent=false, use the existing synchronous IPDL code path and return the result
* check whether the cookie will result in updating the cookie storage (using the new shared code from nsCookieService)
* update the cookies hashtable with the new cookie values
* Send an async message to the parent process, notifying it of the cookie change.

To allow easier measurement of the impact of the sync IPC fallback code paths in SetCookieStringInternal and GetCookieStringInternal, extract them into separate Get/SetCookieStringInternalSyncIPC methods.

Additional testcases:
* ensure document has cookies available, then load about:blank in an iframe, and verify that document.cookies in the iframe can see the parent document’s cookies
* ensure a cross-origin domain has cookies present and a service worker registered. load an iframe that is intercepted by the service worker, and have the final document verify that document.cookies sees the original cookies.


Recommended implementation strategy:
* patch #1:
  - track documents in parent and child
  - document tracking data structure
  - nsDocument::SetPrincipal, nsDocument::Destroy (without cookie purging)
  - ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild (without sending cookies)
* patch #2:
  - sync IPC fallback changes in CookieServiceChild
  - at this point any e10s test using document.cookies should fail
* patch #3:
  - send matching cookies in ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild
  - purge matching cookies from nsDocument::Destroy
* patch #4:
  - shared code for nsCookieService/CookieServiceChild
  - make CookieServiceChild::SetCookieStringInternal reject cookies that would be rejected by the parent
* patch #5:
  - turn CookieServiceParent into an observer
  - send updates for cookie changes to child
* patch #6:
  - tests (be sure to verify that they pass network.cookie.ipc.sync=true, as well)

Please ask questions about anything that is not clear! I have attempted to cover all of the places I expect changes, but I may have forgotten or missed something.

	Josh Matthews [:jdm]
	Comment 101 • 7 years ago

Comment on attachment 8867881 [details] [diff] [review]
implementation part2 -- data struct

I am clearing the feedback requests on these patches given my previous comment. It should be much easier to review the work when it's split up in the order I suggested.

	Amy Chung [:Amy] Assignee
	Comment 102 • 7 years ago

Hi Josh,
Thanks for your suggestions about the design detail, and I have tried to write down the steps I understand and questions as below:
[Data Struct]
* Modified the DocumentCookieInfo.
* [CookieServiceChild]
    * Create nsTArray<RefPtr<nsCookie>> CookiesList.
    * Create a CookiesMap<nsCookieKey, CookiesList>

[Init hash table]
* [Parent Process]
    * Create a function names InitHashTableOnParent(nsIPrincipal) on CookieServiceParent.
    * If principal is available on ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild.
        * If the DocumentCookieMap exists the DocumentCookieInfo, the mActiveDocuments++
            * Because the CookieServiceParent doesn’t exist the entry, maybe only have to confirm the DocumentCookieInfo.
        * If the DocumentCookieInfo doesn’t exist, create a DocumentCookieInfo and let mActiveDocuments =1.
        * If the document have to update to DocumentCookieInfo, get cookies list and find the cookie which is the matching cookie then send to cookie service child.
* [Child Process]
    * If receive the nsCookie from parent.
        * If CookiesMap can get the CookiesLIst from nsCookieKey(nsIChannel.OriginAttributes, BaseDomain). && nsCookie.Name().equals(the nsCookie from CookiesList.Name())
            * If DomainMatch(nsCookie, DocumentCookieInfo.mHostName) && PathMatch(nsCookie, DocumentCookieInfo.mPathName)
                * If the DocumentCookieInfo->mCookiesPresent is true
                    * mActiveDocuments++
                * If the DocumentCookieInfo->mCookiesPresent is false
                    * Set mCookiesPresent to true.
                    * mActiveDocuments++
        * If not equal
            * Init a DocumentCookieInfo and set the domain & path from the entry.
            * mActiveDocuments = 1.
            * mCookiesPresent=true.
    * If not exist the entry.
        * Create a nsCookie and set the value from the cookie which sends from parent.
        * Create a DocumentCookieInfo and set the domain & path from the nsCookie.
            * mActiveDocuments = 1.
            * mCookiesPresent=true.
    * If exist the entry
        * Overwrite all cookies.
* [nsCookieKey]
    * The originAttributes from the principal.

[Remove]
* SetPrincipal
    * Query a nsIHttpChannelInternal from mChannel.
    * If nsIHttpChannelInternal::GetResponseSynthesized returns false
        * [CookieServiceParent]
            * If DocumentCookieInfo which get from nsCookieKey(nsIPrincipal.OriginAttributes, HostName)
                * If mActiveDocuments >= 1 in the existing DocumentCookieInfo which in CookieServiceParent.
                    * mActiveDocuments—
                    * If mActiveDocuments == 0
                        * Remove the DocumentCookieInfo.
            * Send a ipc msg to notify CookeServiceChild to remove the document.
                * [CookieServiceChild]
                * If mActiveDocuments >= 1in the existing DocumentCookieInfo which in CookieServiceChild.
                    * mActiveDocuments—
                    * If mActiveDocuments == 0
                        * Remove the DocumentCookieInfo.
    * Else If nsIHttpChannelInternal::GetResponseSynthesized returns true
        * Call the function InitHashTableOnParent(nsIPrincipal) on CookieServiceParent.
        * Same steps as above.
* Destroy
    * Create a function names DestroyDocumentFromHashTable(DocumentURI, OriginURI, nsIPrincipal) in CookieServiceParent.
        * If the DocumentCookieMap exists a DocumentCookieInfo whose domain equals DocumentURI or OriginURI
            * If mActiveDocuments > 1
                * mActiveDocuments—
            * If mActiveDocuments <= 1
                * Remove the DocumentCookieInfo.
            * Send the msg to CookieServiceChild.
    * Create a function names DestroyDocumentFromHashTable(DocumentURI, OriginURI, nsIPrincipal) in CookieServiceChild.
        * If get the msg about have to destroy document.
            * If the DocumentCookieMap exists a DocumentCookieInfo whose domain equals DocumentURI or OriginURI
                * If mActiveDocuments > 1
                    * mActiveDocuments—
                * If mActiveDocuments <= 1
                    * Remove the DocumentCookieInfo.
                * Purge appropriate cookies
                    * If the cookie’s domain equals the domain from DocumentURI or OriginURI.
                        * If PathMatch(cookie, the path from the DocumentCookieInfo) return false
                            * Remove the cookie.

[Update]
* [CookieServiceParent]
    * CookieServiceParent adds a observer for getting the “cookie-changed” or “private-cookie-changed” from nsCookieService.
        * If CookieServiceParent gets the notify which names “cookie-changed”  or “private-cookie-changed” 
            * If DomainMatches(nsCookie, DocumentCookieInfo->mHostNam) && PathMathes(nsCookie, DocumentCookieInfo->mPathName)
                * Send the nsCookie to CookieServiceChild.
      [CookieServiceChild]
    * CookieServiceChild gets the nsCookie form CookieServiceParent
        * If DomainMatches(nsCookie, DocumentCookieInfo->mHostNam) && PathMathes(nsCookie, DocumentCookieInfo->mPathName)
            * Store the nsCookie.
    * SetCookieString
        * If the network.cookie.ipc.sync preference is false
            * Get the nsIprincipal from loadInfo.
                * If DocumentCookieMap can get DocumentCookieInfo from nsCookieKey(nsIprincipal, GetBaseDomian(nsCookie->Host())).
                    * If mCookiesPresent=false
                        * Don’t store the nsCookie on CookieServiceChild.
                    * If mCookiesPresent=true
                        * If nsCookieService::SetCookieRules(cookieHeader, setCookie) and setCookie is true
                            * Store the nsCookie.
            * If the nsCookie which can’t store to cookie hashtable, the CookieServiceChild still have to send ipc msg to CookieServiceParent for saving the nsCookie to DB.
        * Else if the network.cookie.ipc.sync preference is true
            * CookieServiceChild have to send ipc msg to CookieServiceParent for saving the nsCookie to DB.

[Get Cookies]
*  [CookieServiceChild]
    * If the network.cookie.ipc.sync is true
        * Call the origin function SendGetCookieString().
    * else
        * Get the nsIprincipal from loadInfo.
            * If DocumentCookieMap can get DocumentCookieInfo from nsCookieKey(nsIprincipal, GetBaseDomian(nsCookie->Host())).
                * If mCookiesPresent=false
                    * Call the origin function SendGetCookieString().
                * If mCookiesPresent=true
                    * Get the nsCookie from cookie hash table.

[Questions]
1. Do every notifies which includes init hash table or destroy hash table always send from CookieServiceParent to CookieServiceChild?
2. How do I know the nsIPrincipal when CookieServiceParent get the “cookie-changed” or “private-cookie-changed”?
3. I have tried to separate the rules of setting cookie to a independent function which names “SetCookieRules”. If we want to confirm the secure cookie which definitely set from secure website, do we set the perf “network.cookie.leave-secure-alone” on CookieServiceChild?

Would you give me your suggestions?
Thanks!

	Josh Matthews [:jdm]
	Comment 103 • 7 years ago

The steps for "Init hash table" look ok, except that when we get the cookies (and a document principal) from the parent, we need to:
* check if there is an entry in the known document hashtable matching that principal
* if there is, check if any of the stored DocumentCookieInfo structures match the document principal's URL (host, path, and mIsSecure)

These steps do not involve comparing cookie names or doing domain/path matching. The known document hashtable stores the documents we know about; the cookie hashtable stores the cookies we know about. There is no connection between them.

The steps under SetPrincipal should be adding entries to the document hashmap, not removing them. Also, SetPrincipal and Destroy will be called in the child and then notify the parent.

Questions:
1. Sort of. Most HTTP requests for a document will go through AboutToLoadHttpFtpWyciwygDocumentForChild and require sending a message to the child from the parent. Some HTTP requests will not leave the child process (intercepted by a ServiceWorker, or about:blank) and will require sending a message from the child to the parent (which will then send a message from the parent to the child).
2. nsICookie includes a host attribute and originAttributes. You can use OriginAttributesDictionary::Init to extract it, then you can use OriginAttributesToSuffix (https://dxr.mozilla.org/mozilla-central/rev/b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/dom/base/ChromeUtils.h#82) to serialize it so it can be used in the key in the child.
3. I'm not sure exactly what you're asking. Our implementation has to support both leave-secure-alone=true and leave-secure-alone=false.

Does that make sense?

	Amy Chung [:Amy] Assignee
	Comment 104 • 7 years ago

(In reply to Josh Matthews [:jdm] from comment #103)
> The steps for "Init hash table" look ok, except that when we get the cookies
> (and a document principal) from the parent, we need to:
> * check if there is an entry in the known document hashtable matching that
> principal
> * if there is, check if any of the stored DocumentCookieInfo structures
> match the document principal's URL (host, path, and mIsSecure)
> 
> These steps do not involve comparing cookie names or doing domain/path
> matching. The known document hashtable stores the documents we know about;
> the cookie hashtable stores the cookies we know about. There is no
> connection between them.
> 
> The steps under SetPrincipal should be adding entries to the document
> hashmap, not removing them. Also, SetPrincipal and Destroy will be called in
> the child and then notify the parent.
> 
> Questions:
> 1. Sort of. Most HTTP requests for a document will go through
> AboutToLoadHttpFtpWyciwygDocumentForChild and require sending a message to
> the child from the parent. Some HTTP requests will not leave the child
> process (intercepted by a ServiceWorker, or about:blank) and will require
> sending a message from the child to the parent (which will then send a
> message from the parent to the child).
> 2. nsICookie includes a host attribute and originAttributes. You can use
> OriginAttributesDictionary::Init to extract it, then you can use
> OriginAttributesToSuffix
> (https://dxr.mozilla.org/mozilla-central/rev/
> b8e9b674033bcd1f3a4c59b9d0ee7619c1a17cc5/dom/base/ChromeUtils.h#82) to
> serialize it so it can be used in the key in the child.
> 3. I'm not sure exactly what you're asking. Our implementation has to
> support both leave-secure-alone=true and leave-secure-alone=false.
> 
> Does that make sense?

Thanks for your reply, but I still have some questions on the implementation as below:
1. Does the CookieServiceParent have to set the mCookiePresent after it gets the cookies list? In my view, If the tab reload again, the CookieServiceParent doesn’t need to send the cookies list to child, so the CookieServiceParent can decide whether or not to send the cookie to child by mCookiePresent.

2. When the CookieServiceChild is called SetCookieString(), why does CookieServiceChild need to use the existing synchronous IPDL code path when the mCookiePresent = false from the DocumentCookieInfo which stores in DocumentCookieMap? In my view, CookieServiceChild have to set mCookiePresent to true and store the cookie to the cookie hash table.

3. When the CookieServiceChild is called GetCookieString(), Should CookieServiceChild call the assert when the mCookiePresent = false?

Would you give me your suggestions?
Thanks!

	Josh Matthews [:jdm]
	Comment 105 • 7 years ago

Good questions!

1. The mCookiesPresent value is currently unused in CookieServiceParent in the design I described earlier. I don't think we should try to add special cases around it.

2. If CookieServiceChild sees mCookiesPresent=false, that means that it is still waiting to receive the initial set of cookies for that page. If we update the hashtable and set mCookiesPresent to true, not only will it later be overwritten by the initial set of cookies, but GetCookieString will also start using the non-sync IPC codepath before the set of initial cookies has been received.

3. I do not believe we can use the assert when mCookiePresent=false.

Does that make sense?

	Amy Chung [:Amy] Assignee
	Comment 106 • 7 years ago

Hi,
I have modified my patch as below:
[Data Struct]
* Modified the DocumentCookieInfo.
* [CookieServiceChild]
    * Create nsTArray<RefPtr<nsCookie>> CookiesList.
    * Create a CookiesMap<nsCookieKey, CookiesList>

[Init hash table]
* [Parent Process]
    * Create a function names InitHashTableOnParent(nsIPrincipal, nsIURI, nsIChannel) on CookieServiceParent.
    * If principal is available on ContentParent::AboutToLoadHttpFtpWyciwygDocumentForChild.
        * Call the new function ProcessHashTableOnCookieServiceParent(nsIChannel, nsIPrincipal, isUpdate=true).
          * On CookieServiceParent, call DocumentCookieOperation::UpdateCookieToHashTable(DocumentCookieMap, nsIURI, OriginAttributes).
          * Send ipc msg to Child.
* [Child Process]
    * If receive the msg about init hash table from parent.
        * call DocumentCookieOperation::UpdateCookieToHashTable(DocumentCookieMap, nsIURI, OriginAttributes).
* [SetPrincipal]
    * Query a nsIHttpChannelInternal from mChannel.
    * If nsIHttpChannelInternal::GetResponseSynthesized returns true
        * Call the function InitHashTableOnParent(nsIPrincipal, nsIURI, nsIChannel) on CookieServiceParent.
* [nsCookieKey]
    * The originAttributes from the principal.

[Remove]
* SetPrincipal
    * Query a nsIHttpChannelInternal from mChannel.
    * If nsIHttpChannelInternal::GetResponseSynthesized returns false
        * Call the new function ProcessHashTableOnCookieServiceParent(nsIChannel, nsIPrincipal, false).
        * [CookieServiceParent]
            * If DocumentCookieInfo which get from nsCookieKey(nsIPrincipal.OriginAttributes, HostName)
                * If mActiveDocuments >= 1 in the existing DocumentCookieInfo which in CookieServiceParent.
                    * mActiveDocuments—
                    * If mActiveDocuments == 0
                        * Remove the DocumentCookieInfo.
            * Send a ipc msg to notify CookeServiceChild to remove the document.
                * [CookieServiceChild]
                * If mActiveDocuments >= 1in the existing DocumentCookieInfo which in CookieServiceChild.
                    * mActiveDocuments—
                    * If mActiveDocuments == 0
                        * Remove the DocumentCookieInfo.
* Destroy
    * Create a function names DestroyDocumentFromHashTable(nsIPrincipal, DocumentURI) in CookieServiceParent.
        * If the DocumentCookieMap exists a DocumentCookieInfo whose domain equals DocumentURI or OriginURI
            * If mActiveDocuments > 1
                * mActiveDocuments—
            * If mActiveDocuments <= 1
                * Remove the DocumentCookieInfo.
            * Send the msg to CookieServiceChild.
    * Create a function names DestroyDocumentFromHashTable(DocumentURI, OriginURI, nsIPrincipal) in CookieServiceChild.
        * If get the msg about have to destroy document.
            * If the DocumentCookieMap exists a DocumentCookieInfo whose domain equals DocumentURI or OriginURI
                * If mActiveDocuments > 1
                    * mActiveDocuments—
                * If mActiveDocuments <= 1
                    * Remove the DocumentCookieInfo.

Would you give me your suggestions?
Thanks!

	Junior [inactive]
	Comment 107 • 7 years ago

Comment on attachment 8869169 [details] [diff] [review]
implementation part1

Review of attachment 8869169 [details] [diff] [review]:
-----------------------------------------------------------------

If the movement of nsCookieKey is required, could you move those change to patch 0?
That will speed up the review.

::: dom/ipc/ContentParent.cpp
@@ +89,5 @@
>  #include "mozilla/media/MediaParent.h"
>  #include "mozilla/Move.h"
>  #include "mozilla/net/NeckoParent.h"
> +#include "mozilla/net/PCookieServiceParent.h"
> +#include "mozilla/net/CookieServiceParent.h"

nit: move this line above #include "mozilla/net/NeckoParent.h"

::: dom/ipc/ContentParent.h
@@ +198,5 @@
>  
> +  static void ProcessHashTableOnCookieServiceParent(nsIChannel *aChnnel,
> +                                                    nsIURI *aHostURI,
> +                                                    nsIPrincipal *aPrincipal,
> +                                                    bool aIsUpdate);

const if possible

::: netwerk/cookie/CookieServiceChild.cpp
@@ +93,5 @@
> +{
> +  // Deserialize URI. Having a host URI is mandatory and should always be
> +  // provided by the child; thus we consider failure fatal.
> +  nsCOMPtr<nsIURI> hostURI = DeserializeURI(aHost);
> +  // Update cookie to hash table

nit: s/Update/Destroy

::: netwerk/cookie/CookieServiceParent.h
@@ +28,5 @@
> +                                 nsIChannel *aChannel);
> +
> +  nsresult DestroyDocumentFromHashTable(nsIPrincipal *aPrincipal,
> +                                        nsIURI *aHostURI,
> +                                        nsIChannel *aChannel);

const the parameters if possible

::: netwerk/cookie/DocumentCookieOperation.h
@@ +24,5 @@
> +   :mHostName(aHostName)
> +   ,mPathName(aPathName)
> +   ,mSecure(aSecure)
> +   ,mCookiesPresent(aCookiesPresent)
> +   ,mActiveDocuments(1)

nit: space between :/, and members

::: netwerk/cookie/PCookieService.ipdl
@@ +103,5 @@
>    async __delete__();
> +
> +child:
> +  async StartToInitHashTableOnChild(URIParams host,
> +                                    OriginAttributes attrs);

IMO |StartTo| prefix is a little redundant

::: netwerk/cookie/nsCookieService.cpp
@@ +4205,5 @@
>    return path;
>  }
>  
> +
> +

revert this

	Amy Chung [:Amy] Assignee
	Comment 108 • 7 years ago

Hi,
I have separated the movement of nsCookieKey to independent patch.
Would you give me your suggestions?

	Amy Chung [:Amy] Assignee
	Comment 109 • 7 years ago

Hi,
I have modified my patch as below:
1. Modified nits.
2. Separated the movement of nsCookieKey to other patch.

Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 110 • 7 years ago

Hi,
I have modified the patch as below:
[Get Cookies]
*  [all.js]
    * Created a new pref id names "network.cookie.ipc.sync" and default value is false.
*  [CookieServiceChild]
    * If the network.cookie.ipc.sync is true
        * Call the origin function SendGetCookieString().
    * else
        * Get the nsIprincipal from loadInfo.
            * If DocumentCookieMap can get DocumentCookieInfo from nsCookieKey(nsIprincipal, GetBaseDomian(nsCookie->Host())).
                * If mCookiesPresent=false
                    * Call the origin function SendGetCookieString().
                * If mCookiesPresent=true
                    * Get the nsCookie from cookie hash table.

Would you give me your suggestions on my patch?
Thanks!

[Question]
1. Part 0 - Part 2 don't include any modification on mCookiesPresent which default value is false.
In my view, the mCookiesPresent always get false and this return value will use the existing synchronous IPDL code path and return the result.
So I think document.cookie should get the cookies string through SendGetCookieString().
would you give me your suggestion on my option?

	Amy Chung [:Amy] Assignee
	Comment 111 • 7 years ago

(In reply to Josh Matthews [:jdm] from comment #105)
> Good questions!
> 
> 1. The mCookiesPresent value is currently unused in CookieServiceParent in
> the design I described earlier. I don't think we should try to add special
> cases around it.
Would you explain the reason about the mCookiePresent value is unused in CookieServiceParent?
Does any risk about the mCookiePresent is set in CookieServiceParent?
Whether it is for avoiding CookieServiceParent to miss to send any cookie to CookieServiceChild?
> 
> 2. If CookieServiceChild sees mCookiesPresent=false, that means that it is
> still waiting to receive the initial set of cookies for that page. If we
> update the hashtable and set mCookiesPresent to true, not only will it later
> be overwritten by the initial set of cookies, but GetCookieString will also
> start using the non-sync IPC codepath before the set of initial cookies has
> been received.
Ok, I understand.
> 
> 3. I do not believe we can use the assert when mCookiePresent=false.
Ok, I understand.
> 
> Does that make sense?

	Amy Chung [:Amy] Assignee
	Comment 112 • 7 years ago

Hi,
I have modified my patch as below:
[Init cookie hash table]
* [Parent Process]
    * If the document have to update to DocumentCookieInfo, get cookies list and find the cookie which is the matching cookie then send to cookie service child.
* [Child Process]
    * If receive the nsCookie from parent.
        * If CookiesMap can get the CookiesLIst from nsCookieKey(nsIChannel.OriginAttributes, BaseDomain). && nsCookie.Name().equals(the nsCookie from CookiesList.Name())
            * If DomainMatch(nsCookie, DocumentCookieInfo.mHostName) && PathMatch(nsCookie, DocumentCookieInfo.mPathName)
                * If the DocumentCookieInfo->mCookiesPresent is true
                    * mActiveDocuments++
                * If the DocumentCookieInfo->mCookiesPresent is false
                    * Set mCookiesPresent to true.
                    * mActiveDocuments++
        * If not equal
            * Init a DocumentCookieInfo and set the domain & path from the entry.
            * mActiveDocuments = 1.
            * mCookiesPresent=true.
    * If not exist the entry.
        * Create a nsCookie and set the value from the cookie which sends from parent.
        * Create a DocumentCookieInfo and set the domain & path from the nsCookie.
            * mActiveDocuments = 1.
            * mCookiesPresent=true.
    * If exist the entry
        * Overwrite the existing cookies.

[Remove]
* Destroy
    * Create a function names DestroyDocumentFromHashTable(DocumentURI, OriginURI, nsIPrincipal) in CookieServiceChild.
        * If get the msg about have to destroy document.
            * If the DocumentCookieMap exists a DocumentCookieInfo whose domain equals DocumentURI or OriginURI
                * Purge appropriate cookies
                    * If the DomainMathes
                        * If PathMatches(cookie, the path from the DocumentCookieInfo) return false
                            * Remove the cookie.


Would you give me your suggestions?
Thanks!

	Amy Chung [:Amy] Assignee
	Comment 113 • 7 years ago

Sorry for uploading a wrong attachment, re-uploaded a correct one.