Consider not doing sync IPC for document.cookie getter/setter

NEW
Assigned to

Status

()

Core
Networking: Cookies
3 months ago
6 hours ago

People

(Reporter: Ehsan, Assigned: Amy)

Tracking

(Blocks: 2 bugs)

unspecified
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(platform-rel +)

Details

(Whiteboard: [necko-active][platform-rel-Linkedin][qf:p1])

Attachments

(9 attachments, 5 obsolete attachments)

79.55 KB, patch
Details | Diff | Splinter Review
16.74 KB, patch
Details | Diff | Splinter Review
18.10 KB, patch
Details | Diff | Splinter Review
6.04 KB, patch
Details | Diff | Splinter Review
9.80 KB, patch
Amy
: feedback?
jdm
Details | Diff | Splinter Review
15.04 KB, patch
Amy
: feedback?
jdm
Details | Diff | Splinter Review
11.63 KB, patch
Amy
: feedback?
jdm
Details | Diff | Splinter Review
64.05 KB, patch
Amy
: feedback?
jdm
Details | Diff | Splinter Review
100.86 KB, patch
Details | Diff | Splinter Review
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.
Flags: needinfo?(mcmanus)
Flags: needinfo?(josh)
Flags: needinfo?(jduell.mcbugs)
(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?
Flags: needinfo?(mcmanus)
(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.
> 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.
Flags: needinfo?(jduell.mcbugs)
(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.
Flags: needinfo?(cpeterson)
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.  :-)
>  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.
Sounds like we could skip sending http-only cookies to the child (and https-only ones if a domain was only loaded with http://).
(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.
Flags: needinfo?(cpeterson)

Comment 9

3 months 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.
Flags: needinfo?(josh)
Whiteboard: [necko-would-take]
(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.
Flags: needinfo?(cpeterson)
(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.
Flags: needinfo?(cpeterson)
Depends on: 1334509
(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.
Flags: needinfo?(ehsan)
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.
Depends on: 1339129
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!
Flags: needinfo?(ehsan) → needinfo?(amchung)
Blocks: 1287730
(Assignee)

Comment 15

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

Thanks again for your help!
Flags: needinfo?(amchung)
(Assignee)

Updated

2 months ago
Assignee: nobody → amchung
(Assignee)

Updated

2 months ago
Whiteboard: [necko-would-take] → [necko-active]
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.
(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?
That sounds fine to me.
platform-rel: --- → +
Whiteboard: [necko-active] → [necko-active][platform-rel-Linkedin]

Updated

2 months ago
Duplicate of this bug: 1306928
(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?
(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?
Flags: needinfo?(ehsan)
(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.)
Flags: needinfo?(ehsan)
(Assignee)

Comment 23

2 months 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?
Flags: needinfo?(ehsan)
(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!
Flags: needinfo?(ehsan)
(Assignee)

Comment 25

2 months 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?
Flags: needinfo?(ehsan)
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!
Flags: needinfo?(ehsan)
(BTW the code above is a sample, it probably has mistakes and typos. :-)
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 :).
Whiteboard: [necko-active][platform-rel-Linkedin] → [necko-active][platform-rel-Linkedin][qf:p1]
(Assignee)

Comment 29

2 months 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.
Duplicate of this bug: 1194761
(Assignee)

Comment 31

2 months ago
Created attachment 8847262 [details] [diff] [review]
implementation

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!
Attachment #8847262 - Flags: feedback?(ehsan)
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.  :-)
Attachment #8847262 - Flags: feedback?(ehsan) → feedback?(josh)
(Assignee)

Comment 33

a month ago
Created attachment 8849038 [details] [diff] [review]
implementation

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!
Attachment #8847262 - Attachment is obsolete: true
Attachment #8847262 - Flags: feedback?(josh)
Attachment #8849038 - Flags: feedback?(josh)
(Assignee)

Comment 34

a month 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!
Flags: needinfo?(josh)
The first profile does not appear to show any significant usage of document.cookie. Is that an unexpected result for that site?
Flags: needinfo?(josh)
(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.
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.
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.
Attachment #8849038 - Flags: feedback?(josh) → feedback-
(Assignee)

Comment 39

a month ago
Created attachment 8849675 [details] [diff] [review]
implementation

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!
Attachment #8849038 - Attachment is obsolete: true
Attachment #8849675 - Flags: feedback?(josh)
(Assignee)

Comment 40

a month 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!
Attachment #8849675 - Flags: feedback?(josh)
(Assignee)

Comment 41

a month ago
Created attachment 8852155 [details] [diff] [review]
implementation

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!
Attachment #8849675 - Attachment is obsolete: true
Attachment #8852155 - Flags: feedback?(josh)
(Assignee)

Comment 42

a month ago
Created attachment 8852874 [details] [diff] [review]
bug-1331680-tmp.patch

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.
Flags: needinfo?(josh)
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?
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
Attachment #8852155 - Flags: feedback?(josh) → feedback-

Updated

26 days ago
Flags: needinfo?(josh)
Depends on: 1354349
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.
(Assignee)

Comment 46

18 days 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.
(Assignee)

Comment 47

14 days 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!
Flags: needinfo?(josh)
Duplicate of this bug: 1266275
(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.
Flags: needinfo?(josh)
(Assignee)

Comment 50

10 days 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!
Flags: needinfo?(josh)
Created attachment 8859465 [details] [diff] [review]
Add CookieStruct IPDL representation and send a list of CookieStructs to the child in OnStartRequest

* 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
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.
(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 :)
Flags: needinfo?(josh)
(Assignee)

Comment 54

2 days ago
Created attachment 8862104 [details] [diff] [review]
implementation part1--create & set load flag.

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!
Attachment #8862104 - Flags: feedback?(josh)
(Assignee)

Updated

2 days ago
Attachment #8862104 - Attachment description: implementation--create & set load flag. → implementation part1--create & set load flag.
(Assignee)

Comment 55

2 days ago
Created attachment 8862117 [details] [diff] [review]
implementation part2 -- init & update on hash table, and get cookie from hahs table

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!
Attachment #8862117 - Flags: feedback?(josh)
(Assignee)

Comment 56

2 days ago
Created attachment 8862119 [details] [diff] [review]
implementation part5 --- Destroy cookie

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!
Attachment #8862119 - Flags: feedback?(josh)
(Assignee)

Comment 57

2 days ago
Created attachment 8862121 [details] [diff] [review]
implementation -- full version
(Assignee)

Comment 58

2 days ago
Created attachment 8862378 [details] [diff] [review]
implementation part2 -- data struct

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!
Attachment #8862117 - Attachment is obsolete: true
Attachment #8862117 - Flags: feedback?(josh)
Attachment #8862378 - Flags: feedback?(josh)
(Assignee)

Updated

2 days ago
Attachment #8862119 - Attachment description: implementation part3 --- Destroy cookie → implementation part5 --- Destroy cookie
(Assignee)

Comment 59

2 days ago
Created attachment 8862380 [details] [diff] [review]
implementation part3 -- http channel

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!
Attachment #8862380 - Flags: feedback?(josh)
(Assignee)

Updated

2 days ago
Attachment #8862380 - Attachment description: implementation part2 -- http channel → implementation part3 -- http channel
(Assignee)

Comment 60

2 days ago
Created attachment 8862382 [details] [diff] [review]
implementation part4 -- cookie service

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!
Attachment #8862382 - Flags: feedback?(josh)
(Assignee)

Comment 61

2 days ago
Created attachment 8862384 [details] [diff] [review]
implementation -- full version
Attachment #8862121 - Attachment is obsolete: true
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."
Attachment #8862104 - Flags: feedback?(josh) → feedback-
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.
You need to log in before you can comment on or make changes to this bug.