STR: 1) Go to http://techcrunch.com/ and make sure Adblock Plus is disabled. 2) See some calls from http://amch.questionmarket.com/ domain. 3) Check their response headers in network tab, they have Set-Cookie:"CS1=1108335-12-1; expires=Fri, 03-Apr-2015 02:24:03 GMT; path=/; domain=.questionmarket.com ES=1042020-3"]PO-0; expires=Fri, 03-Apr-2015 02:24:03 GMT; path=/; domain=.questionmarket.com;" 4) This creates a cookie with domain as "amch.questionmarket.com" which is HTTP only Now if we have a devtool to list/edit cookies (bug 965872) then there is no way to link this cookie to the original page "techcrunch.com" Perform the same steps on Chrome, open devtools, Resources panel and see that it lists the cookies from "amch.questionmarket.com" under "techcrunch.com" host itself.

I don't think we need to change anything in the cookies database for this. You probably want to send a notification in the http channel code to associate a cookie with the load group that the channel belongs to, for example, from here: <http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/nsHttpChannel.cpp#1202>

While setting the http cookie received in the HTTP response headers, HttpBaseChannel:SetCookie [0] is called which is defined at [1, 2]. As per the method's signature, the second argument, aFirstURI neither is used anywhere in the method, nor is passed to the (only) method call of that method. Ehsan, if the http layer passes the load group's url (mostly the page url) as the second argument, can the method nsCookieService::SetCookieStringFromHttp then use that and send out some sort of notification about it ? Are there any security/performance/etc. issues involved ? [0] http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpBaseChannel.cpp#1317 [1] http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsICookieService.idl#177 [2] http://mxr.mozilla.org/mozilla-central/source/netwerk/cookie/nsCookieService.cpp#1666

(In reply to comment #2) > Ehsan, if the http layer passes the load group's url (mostly the page url) as > the second argument, can the method nsCookieService::SetCookieStringFromHttp > then use that and send out some sort of notification about it ? The URL is not what you want, since then you would be unable to differentiate between two tabs with the same URL, for example. You really want the nsILoadGroup that a channel belongs to.

okay, so I can get reference to the current nsILoadGroup from the HttpBaseChannel::SetCookie method. Is it okay to send out a notification via observer service with subject as the nsiloadgroup ?

Something like this works. It gives me loadGroup and the cookie . I don't know if this is the correct approach, or if it will have an impact on performance. Also, even after getting the loadGroup, how do I make sure that this is the correct load group ? The name property is sometimes null too.

Ok, after digging for a bit, I found a way to link the nsLoadGroup with the current window. Run the following in browser mode scratchpad and load a site like nytimes.com (with the patch applied). It will correctly match the cookies created via doubleclick.net etc. Services.obs.addObserver({observe:(a,b,c)=>{ console.log(a.QueryInterface(Ci.nsILoadGroup).name, c); var l = gBrowser.getBrowserForDocument(content.document).webNavigation.QueryInterface(Ci.nsIDocumentLoader) .loadGroup.QueryInterface(Ci.nsISupportsPriority) console.log(a == l) }}, "external-cookie", false)

Comment on attachment 8374689 [details] [diff] [review] wip Review of attachment 8374689 [details] [diff] [review]: ----------------------------------------------------------------- I'm not very familiar with this code, please ask for feedback from somebody on the Necko team. Thanks!

Comment on attachment 8374689 [details] [diff] [review] wip Honza, as you have some context about the issue, I am asking you for feedback, please feel free to transfer it to a more suitable person if you feel like.

Comment on attachment 8374689 [details] [diff] [review] wip discussed on irc

Made changes as requested on IRC. - passing nsIChannel instead of nsLoadGroup - named the topic to "http-on-response-set-cookie" - moved everything to a runnable and NS_DispatchToMainThread try : https://tbpl.mozilla.org/?tree=Try&rev=38871cbbe609

I think the lifetime of the cookie string as char* might be limited. You should carry it in nsString (member of the event) and do the conversion immediately before the event is posted. Otherwise looks good I think.

Converting from char * to nsString and then back to char16_t *

comment 11 was head on, try with previous patch has oranges on debug builds : https://tbpl.mozilla.org/?tree=Try&rev=38871cbbe609 but try with latest patch is green : https://tbpl.mozilla.org/?tree=Try&rev=aab1d362434c

Comment on attachment 8376451 [details] [diff] [review] patch v0.2 Review of attachment 8376451 [details] [diff] [review]: ----------------------------------------------------------------- Mostly good, just few details and we can go. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1339,5 @@ > + "http-on-response-set-cookie", > + mCookie.get()); > + } > + return NS_OK; > +} please inline this to the class (then use NS_IMETHOD instead of NS_IMETHODIMP) @@ +1360,5 @@ > + nsresult rv; > + rv = cs->SetCookieStringFromHttp(mURI, nullptr, nullptr, aCookieHeader, > + mResponseHead->PeekHeader(nsHttp::Date), > + this); > + if (rv == NS_OK) { if (NS_SUCCEEDED(rv)) @@ +1361,5 @@ > + rv = cs->SetCookieStringFromHttp(mURI, nullptr, nullptr, aCookieHeader, > + mResponseHead->PeekHeader(nsHttp::Date), > + this); > + if (rv == NS_OK) { > + NS_ConvertASCIItoUTF16 nsCookieHeader(aCookieHeader); The conversion should happen in the runnable. Have NS_ConvertASCIItoUTF16 as a member of your runnable. Pass aCookieHeader to the runnable. @@ +1362,5 @@ > + mResponseHead->PeekHeader(nsHttp::Date), > + this); > + if (rv == NS_OK) { > + NS_ConvertASCIItoUTF16 nsCookieHeader(aCookieHeader); > + NS_DispatchToMainThread(new CookieNotifierRunnable(this, nsCookieHeader)); Put the runnable to nsRefPtr<CookieNotifierRunnable> and then pass that refptr to NS_Dispatch...(). If dispatch fails, we leak the runnable with your code!

(In reply to Honza Bambas (:mayhemer) from comment #14) > Comment on attachment 8376451 [details] [diff] [review] > patch v0.2 > > Review of attachment 8376451 [details] [diff] [review]: > ----------------------------------------------------------------- > > Mostly good, just few details and we can go. > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > @@ +1339,5 @@ > > + "http-on-response-set-cookie", > > + mCookie.get()); > > + } > > + return NS_OK; > > +} > > please inline this to the class (then use NS_IMETHOD instead of > NS_IMETHODIMP) I am not sure that I understand what you mean here . (pretty new to C++ side) > @@ +1361,5 @@ > > + rv = cs->SetCookieStringFromHttp(mURI, nullptr, nullptr, aCookieHeader, > > + mResponseHead->PeekHeader(nsHttp::Date), > > + this); > > + if (rv == NS_OK) { > > + NS_ConvertASCIItoUTF16 nsCookieHeader(aCookieHeader); > > The conversion should happen in the runnable. Have NS_ConvertASCIItoUTF16 > as a member of your runnable. Pass aCookieHeader to the runnable. I thought in comment 11 you were suggesting that instead of passing char * to the runnable, I pass nsString instead. In the previous version of the patch, I was passing char * to the runnable only, right ?

(In reply to Girish Sharma [:Optimizer] from comment #15) > (In reply to Honza Bambas (:mayhemer) from comment #14) > > Comment on attachment 8376451 [details] [diff] [review] > > patch v0.2 > > > > Review of attachment 8376451 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > Mostly good, just few details and we can go. > > > > ::: netwerk/protocol/http/HttpBaseChannel.cpp > > @@ +1339,5 @@ > > > + "http-on-response-set-cookie", > > > + mCookie.get()); > > > + } > > > + return NS_OK; > > > +} > > > > please inline this to the class (then use NS_IMETHOD instead of > > NS_IMETHODIMP) > > I am not sure that I understand what you mean here . (pretty new to C++ side) do: class YourRunnable : public nsRunnable { NS_IMETHOD Run() { your code here.. return NS_OK; } }; > > > @@ +1361,5 @@ > > > + rv = cs->SetCookieStringFromHttp(mURI, nullptr, nullptr, aCookieHeader, > > > + mResponseHead->PeekHeader(nsHttp::Date), > > > + this); > > > + if (rv == NS_OK) { > > > + NS_ConvertASCIItoUTF16 nsCookieHeader(aCookieHeader); > > > > The conversion should happen in the runnable. Have NS_ConvertASCIItoUTF16 > > as a member of your runnable. Pass aCookieHeader to the runnable. > > I thought in comment 11 you were suggesting that instead of passing char * > to the runnable, I pass nsString instead. In the previous version of the > patch, I was passing char * to the runnable only, right ? Hmm.. right, I didn't realize. Anyway, the only goal here is to not let YourRunnable keep reference to the char * (since it will probably be an invalid pointer before the event fires (=Run() is called). So, my personal preference here is to pass char * to the constructor but let YourRunnable make a copy to ConvertASCIItoUTF16. So, it should be like: class YourRunnable : ... { YourRunnable(char const * cookie) : mCookie(cookie) { } NS_ConvertASCIItoUTF16 mCookie; };

Addressed review comments.

Comment on attachment 8379836 [details] [diff] [review] patch v0.3 Review of attachment 8379836 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with few small nits. ::: netwerk/protocol/http/HttpBaseChannel.cpp @@ +1333,5 @@ > + return NS_OK; > + } > + > +private: > + nsCOMPtr<nsIChannel> mChannel; One more small optimization: use nsRefPtr<HttpBaseChannel> for mChannel member. At the call to NotifyObservers you will have to do static_cast<nsIChannel*>(mChannel.get()), since C++ will not be able to find an unambiguous path to nsISupport base interface (multiple inheritance of HttpBaseChannel). nsCOMPtr<> calls QueryInterface that is not just cheap and should be avoided when not necessary. Here we know the class type and this code is private, so it's ok to do static_cast on the pointer. @@ +1353,5 @@ > nsICookieService *cs = gHttpHandler->GetCookieService(); > NS_ENSURE_TRUE(cs, NS_ERROR_FAILURE); > > + nsresult rv; > + rv = cs->SetCookieStringFromHttp(mURI, nullptr, nullptr, aCookieHeader, nsresult rv = cs->....

Fixed the nits. r+ as per last comment. Thank you Honza for your patience and helping me through this bug.

Dev doc needed for adding the following properties in the https://developer.mozilla.org/en-US/docs/Observer_Notifications page and everywhere else required. New notification since Firefox 30: http-on-response-set-cookie - This is fired only when a cookie is created due to the presence of SET-COOKIE header in the response header of any network request. This notification will come only after the "http-on-examine-response" is fired, but it will not always appear. topic : http-on-response-set-cookie subject: nsiChannel - The channel associated with the network request data: string - The raw cookie string which is used to create the cookie. This is the string which is present in the response header's SET-COOKIE header as is. Thanks.