Comment on attachment 8714917 [details] [diff] [review] cookie.patch Review of attachment 8714917 [details] [diff] [review]: ----------------------------------------------------------------- I don't know the cookies code well, and I can't review code in netwerk/. Please check the VCS logs to find someone for the cookies code and ask a netwerk peer too. ::: netwerk/cookie/nsICookieManager.idl @@ +16,1 @@ > interface nsICookieManager : nsISupports This shouldn't be necessary anymore. @@ +38,5 @@ > * strings. If the target cookie is a domain cookie, a leading > * dot must be present. > * @param aName The name specified in the cookie > * @param aPath The path for which the cookie was set > * @param aBlocked Indicates if cookies from this host should be permanently blocked This should list aUserContextId with a short description.

Comment on attachment 8714917 [details] [diff] [review] cookie.patch Smaug?

Comment on attachment 8714917 [details] [diff] [review] cookie.patch ehsan has, IIRC, played with cookies.

Comment on attachment 8714917 [details] [diff] [review] cookie.patch Review of attachment 8714917 [details] [diff] [review]: ----------------------------------------------------------------- nsICookieManager.remove() is by no way special. It is one of the many ways we delete a cookie. See all of the other call sites of RemoveCookieFromList(). This should be done for all of them. Also is there any reason why you would want to support deleting other users' cookies? That seems pretty questionable. It's definitely the wrong thing to do at least under the generic remove() name. If there is a use case for that, I think we should have a separate nsICookieManager API like removeFromOtherUser() or some such. But I'm uncomfortable adding an API like this, it's too big of a footgun. If the only use case here is doing the right thing in the face of multiple users, I would like to instead internally use the current user context ID, always. There's a couple of code-related issues below but I suspect looking at them may be unnecessary since I'm suggesting a completely different approach. :-) ::: netwerk/cookie/nsCookieService.cpp @@ +2370,4 @@ > bool aBlocked) > { > NeckoOriginAttributes attrs; > + attrs.mUserContextId = aUserContextId; You should add this to the other Remove() override too. ::: netwerk/cookie/nsICookieManager.idl @@ +45,5 @@ > + void remove(in AUTF8String aHost, > + in ACString aName, > + in AUTF8String aPath, > + in unsigned long aUserContextId, > + in boolean aBlocked); Have you checked how many add-ons use this?

The reason why I changed the remove() is because, simply, it doesn't work with containers. What we do in remove is: NeckoOriginAttributes attrs; return Remove(aHost, attrs, aName, aPath, aBlocked); Use-case: the user opens the cookie manager from about:profiles. He/she selects 1 cookie from 1 particular container and it tries to remove it: he/she cannot, because that cookie has a particular userContextId, but we are using a generic NeckoOriginAttributes, that doesn't have such userContextId flag set. Result: we delete the cookie with UserContextId == 0, instead the selected cookie. > If the only use case here is doing the right thing in the face of multiple > users, I would like to instead internally use the current user context ID, > always. I don't know what you mean with multiple users... but, the current way we delete cookies in RemoveCookieFromList works. We just don't have a correct remove method from the UI :)

This is a regression from bug 1165267, one that we have shipped. :(( If you go to the cookie viewer and delete the cookie, you'll end up deleting the cookie for the wrong user, or not do anything at all.

It is actually only a regression if you enable containers.

Comment on attachment 8714917 [details] [diff] [review] cookie.patch Review of attachment 8714917 [details] [diff] [review]: ----------------------------------------------------------------- OK, so now that I understand what this patch is doing, please ignore my comments before. :-) Just remove the uuid change. ::: netwerk/cookie/nsCookieService.cpp @@ +2370,4 @@ > bool aBlocked) > { > NeckoOriginAttributes attrs; > + attrs.mUserContextId = aUserContextId; You should add this to the other Remove() override too. ::: netwerk/cookie/nsICookieManager.idl @@ +45,5 @@ > + void remove(in AUTF8String aHost, > + in ACString aName, > + in AUTF8String aPath, > + in unsigned long aUserContextId, > + in boolean aBlocked); Have you checked how many add-ons use this?

Please don't make this code userContextId specific. Use full OriginAttributes structures instead.

jorgev, this patch changes nsICookieManager.remove() and it breaks several addons. Can we inform the addon developers about it?

(Without looking at the patch, could we make such interface changes that we don't end up breaking addons? And add a warning of using deprecated method?)

(In reply to Olli Pettay [:smaug] from comment #11) > (Without looking at the patch, could we make such interface changes that we > don't end up breaking addons? And add a warning of using deprecated method?) I can add a new method, but, talking with ehsan, that seems too fragile.

(In reply to Andrea Marchesini (:baku) from comment #10) > jorgev, this patch changes nsICookieManager.remove() and it breaks several > addons. > Can we inform the addon developers about it? We can include it in the compatibility communications, but we won't be able to include it in the automatic validation because "remove" is too generic of a test. I just need that the target milestone is set when this lands so I can include it in the right blog post.

Comment on attachment 8715451 [details] [diff] [review] cookie.patch Review of attachment 8715451 [details] [diff] [review]: ----------------------------------------------------------------- Generally speaking, if your patch contains the string "userContextId" or "appId", it is probably not handling OriginAttributes correctly. ::: browser/components/preferences/cookies.js @@ +71,5 @@ > _cookieEquals: function (aCookieA, aCookieB, aStrippedHost) { > return aCookieA.rawHost == aStrippedHost && > aCookieA.name == aCookieB.name && > + aCookieA.path == aCookieB.path && > + aCookieA.originAttributes.userContextId == aCookieB.originAttributes.userContextId; You should compare all attributes in the originAttributes. We might need to add a function on ChromeUtils for comparing two OriginAttributes with each other. @@ +285,5 @@ > var cookie = parent.cookies[i]; > if (item.rawHost == cookie.rawHost && > + item.name == cookie.name && > + item.path == cookie.path && > + item.originAttributes.userContextId == cookie.originAttributes.userContextId) { Same here.

Comment on attachment 8716933 [details] [diff] [review] cookie.patch Review of attachment 8716933 [details] [diff] [review]: ----------------------------------------------------------------- r=me with that fixed, or with the '!' explained. ::: browser/components/preferences/cookies.js @@ +71,5 @@ > _cookieEquals: function (aCookieA, aCookieB, aStrippedHost) { > return aCookieA.rawHost == aStrippedHost && > aCookieA.name == aCookieB.name && > + aCookieA.path == aCookieB.path && > + !ChromeUtils.compareOriginAttributes(aCookieA.originAttributes, Why the '!'. Below it looks like compareOriginAttributes returns true if the two attributes are the same, which is also what I would expect from an API like that. @@ +286,5 @@ > var cookie = parent.cookies[i]; > if (item.rawHost == cookie.rawHost && > + item.name == cookie.name && > + item.path == cookie.path && > + !ChromeUtils.compareOriginAttributes(item.originAttributes, Same here. ::: dom/base/ChromeUtils.cpp @@ +88,5 @@ > aAttrs = attrs; > } > > +/* static */ bool > +ChromeUtils::CompareOriginAttributes(dom::GlobalObject& aGlobal, Maybe name this IsOriginAttributesEqual to make it clear that this doesn't return -1, 0, 1 depending on which is "bigger". Which is what "compare"/"cmp" functions sometimes do. @@ +92,5 @@ > +ChromeUtils::CompareOriginAttributes(dom::GlobalObject& aGlobal, > + const dom::OriginAttributesDictionary& aA, > + const dom::OriginAttributesDictionary& aB) > +{ > + return aA.mAddonId == aB.mAddonId && Juse do |return aA == aB;|

> Juse do |return aA == aB;| We don't have operator== for WebIDL dictionaries.

backed out for build problems like https://treeherder.mozilla.org/logviewer.html#?job_id=22017228&repo=mozilla-inbound

> We don't have operator== for WebIDL dictionaries. We use operator!=/== everywhere else where we compare OAs. See for example [1]. I'm not sure if it's generated by the WebIDL compiler, if it's in one of the C++ subclasses that we use for OAs, or if it's the default operators provided by C++. But it works :) [1] http://mxr.mozilla.org/mozilla-central/source/caps/nsPrincipal.cpp#201

We have an operator for OriginAttributes but not for OriginAttributeDictionary.

Can you then do OriginAttributes(aA) == OriginAttributes(aB)?

It seems that we don't want to have a public CTOR for OriginAttributes class.

Use GenericOriginAttributes?

Comment on attachment 8723132 [details] [diff] [review] cookie2.patch >+NS_IMETHODIMP_(nsresult) Why not just NS_IMETHODIMP >+nsCookieService::RemoveNative(const nsACString &aHost, >+ const nsACString &aName, >+ const nsACString &aPath, >+ NeckoOriginAttributes* aOriginAttributes, >+ bool aBlocked) >+{ >+ if (NS_WARN_IF(!aOriginAttributes)) { >+ return NS_ERROR_FAILURE; >+ } Ah, I thought you'd let one to pass null here and if so, call Remove with a NeckoOriginAttributes created in this method. But if this works, fine. >+ >+ mozilla::NeckoOriginAttributes attrs; >+ so I thought this wasn't possible to use here. >+ >+// Stubs to make this test happy >+ >+mozilla::dom::OriginAttributesDictionary::OriginAttributesDictionary() {} ok, if this all links, fine.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab3a305b3e41 https://hg.mozilla.org/integration/mozilla-inbound/rev/ff0797af7e1e https://hg.mozilla.org/integration/mozilla-inbound/rev/21513aca36b4

https://hg.mozilla.org/mozilla-central/rev/ab3a305b3e41 https://hg.mozilla.org/mozilla-central/rev/ff0797af7e1e https://hg.mozilla.org/mozilla-central/rev/21513aca36b4

Backout: https://hg.mozilla.org/integration/b2g-inbound/rev/f88f85b0de5f

Why this backout?

gwagner, i guess this is a fallout from your merge to b2g-i (In reply to Pulsebot from comment #30) > Backout: > https://hg.mozilla.org/integration/b2g-inbound/rev/f88f85b0de5f can you take a look, no idea why pulsebot thinks this was a backout

I guess thats a bug in pulsebot. The merge contains the backout and the re-landing.

At least one add-on (CookieCuller, [1]) was using this API and is now broken. I'm assuming that's intentional and that the add-on needs to be updated, but heads-up in case it wasn't. [1] https://addons.mozilla.org/en-US/firefox/addon/cookieculler/

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #35) > At least one add-on (CookieCuller, [1]) was using this API and is now > broken. I'm assuming that's intentional and that the add-on needs to be > updated, but heads-up in case it wasn't. > > [1] https://addons.mozilla.org/en-US/firefox/addon/cookieculler/ Reddit Enhancement Suite, which is used for addon advocacy, also had to fix this [1]. Potentially a lot of addons will need to be updated for this change, so maybe we could add an entry in the release notes? [1] https://github.com/honestbleeps/Reddit-Enhancement-Suite/issues/2779

2 more add-ons affected by this change: * Remove Cookie(s) for Site: https://addons.mozilla.org/en-US/firefox/addon/remove-cookies-for-site/ * Web Developer: https://addons.mozilla.org/en-US/firefox/addon/web-developer/ They throw errors: NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsICookieManager.remove] and NS_ERROR_XPC_NOT_ENOUGH_ARGS: Not enough arguments [nsICookieManager2.remove]

Web Developer is very popular, need doc.

Is it time to nominate these for aurora uplift? Baku, can you also attach the final versions of the patch to this bug. I think there was some modifications between the patches here and what was pushed to nightly. Thanks!

(In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #39) > Is it time to nominate these for aurora uplift? These patches are currently in m-i/m-c only, right? If we uplift them we must land bug 1259169 to aurora as well otherwise we break add-ons there too. I prefer to keep it as it is and once bug 1259169 is fully reviewed and landed, we can uplift them together. What do you think?

(In reply to Andrea Marchesini (:baku) from comment #40) > (In reply to Tanvi Vyas - please needinfo [:tanvi] from comment #39) > > Is it time to nominate these for aurora uplift? > > These patches are currently in m-i/m-c only, right? If we uplift them we > must land bug 1259169 to aurora as well otherwise we break add-ons there > too. I prefer to keep it as it is and once bug 1259169 is fully reviewed and > landed, we can uplift them together. What do you think? Sorry baku, I'm getting the bugs mixed up. I've commented in bug 1259169. That's the one we need to land and uplift to aurora 47. This bug is already landed in 47 right? This is what caused bug 1259169?

Tanvi if we uplift the patches in bug 1259169 to beta (46) does that affect your issue here?

I don't think anything needs to be done here. This patch landed in 47 and should stay that way.

Comment on attachment 8716933 [details] [diff] [review] cookie.patch >- cookieMgr.remove(cookie.host, cookie.name, cookie.path, false); >+ cookieMgr.remove(cookie.host, cookie.name, cookie.path, >+ cookie.originAttributes, false); Removing an enumerated cookie seems to be pretty popular. Is it too late to ask for (or write a patch to provide) a dedicated method to do this?

> Removing an enumerated cookie seems to be pretty popular. Is it too late to > ask for (or write a patch to provide) a dedicated method to do this? We already did it.