In bug 1245184 we changed nsICookieManager::remove(). But that broke many addons. We want to support the old prototype of ::remove() for 1 or 2 releases to give times to addon developers to update their code.

tanvi, I tried but I cannot really write |for (uint32_t i = 0; i <= 4; ++i) { ... }|. The patch I submitted works with the default userContextId. If we want to support all the other values, we should implement something else. What about: nsIContainerService? that gives the number of the default userContextId, maybe the name, localized too.

(In reply to Andrea Marchesini (:baku) from comment #2) > tanvi, I tried but I cannot really write |for (uint32_t i = 0; i <= 4; ++i) > { ... }|. Why is that? > The patch I submitted works with the default userContextId. If we can't get all user contexts, we can stick with the default. > If we want to > support all the other values, we should implement something else. What > about: nsIContainerService? that gives the number of the default > userContextId, maybe the name, localized too. Are you proposing this because you are worried that there are contexts other than 0, 1, 2, 3, and 4? Is this so that we don't accidentally miss a cookie set with context=5? Since this is just temporary, I think the 0 through 4 loop is okay to use.

> Are you proposing this because you are worried that there are contexts other > than 0, 1, 2, 3, and 4? Is this so that we don't accidentally miss a cookie > set with context=5? Since this is just temporary, I think the 0 through 4 > loop is okay to use. because as much as we try, temporary in gecko means a lot of time :) I'm ok with what you propose but we must be 100% that we remove this hack in 12 weeks time. Now, let's see what the reviewer says about this.

Comment on attachment 8734018 [details] [diff] [review] remove.patch Review of attachment 8734018 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cookie/nsCookieService.cpp @@ +2369,5 @@ > JS::HandleValue aOriginAttributes, > bool aBlocked, > JSContext* aCx) > { > + if (aOriginAttributes.IsUndefined()) { This is not the right way to figure out whether the argument was passed or not, you want to mark the function as [optional_argc] and check how many arguments have been passed in. @@ +2378,5 @@ > + nsContentUtils::ReportToConsole(nsIScriptError::warningFlag, > + NS_LITERAL_CSTRING("Cookie Manager"), > + nullptr, > + nsContentUtils::eNECKO_PROPERTIES, > + "nsICookieManagerRemoveDeprecated"); Hmm, so what is the actual behavior that you want here besides just warning? ::: netwerk/cookie/nsICookieManager.idl @@ +47,5 @@ > * dot must be present. > * @param aName The name specified in the cookie > * @param aPath The path for which the cookie was set > + * @param aOriginAttributes The originAttributes of this cookie. This > + * attribute is optional to do not break addons. In Nit: to avoid breaking add-ons. @@ +48,5 @@ > * @param aName The name specified in the cookie > * @param aPath The path for which the cookie was set > + * @param aOriginAttributes The originAttributes of this cookie. This > + * attribute is optional to do not break addons. In > + * 1 or 2 releases it will be mandatory. How will we remember? :-) @@ +56,5 @@ > [implicit_jscontext] > void remove(in AUTF8String aHost, > in ACString aName, > in AUTF8String aPath, > + [optional] in jsval aOriginAttributes, How does this help with backwards compat? remove() used to accept 4 arguments. I think what you meant to do here was to also move aOriginAttributes to be the last argument? ::: netwerk/locales/en-US/necko.properties @@ +44,5 @@ > # %1$S is the deprected API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: '%1$S' deprecated, please use '%2$S' > + > +# LOCALIZATION NOTE (nsICookieManagerRemoveDeprecated): don't localize nsICookieManager::remove() and originAttributes. > +nsICookieManagerRemoveDeprecated="Warning: 'nsICookieManager::remove()' is changed. Update your code and pass the correct originAttributes.' This will probably prevent the patch from being uplifted to Aurora. Also, I don't think this is helpful in its current form, since it's not clear how remove() is changed and how to pass the correct originAttributes. Perhaps you should link to an MDN article? Also, less importantly, you don't need to add "Warning" here IMO since the console message is already a warning. And please get rid of the "::" C++ism too!

Comment on attachment 8735800 [details] [diff] [review] remove.patch Review of attachment 8735800 [details] [diff] [review]: ----------------------------------------------------------------- Clearing the review request for now. Can you please respond to my two questions below? Thanks! ::: netwerk/cookie/nsCookieService.cpp @@ +2375,2 @@ > { > + MOZ_ASSERT(aArgc >= 4); Huh. AFAIK argc here is the number of *optional* arguments, not all arguments, so it should either be 0 or 1, which means this assertion should always fail. Can you explain why it doesn't? ::: netwerk/cookie/nsICookieManager.idl @@ +65,5 @@ > nsresult removeNative(in AUTF8String aHost, > in ACString aName, > in AUTF8String aPath, > + in boolean aBlocked, > + in NeckoOriginAttributesPtr aOriginAttributes); This change is technically unneeded, but I guess it doesn't hurt... ::: netwerk/locales/en-US/necko.properties @@ +44,5 @@ > # %1$S is the deprected API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: '%1$S' deprecated, please use '%2$S' > + > +# LOCALIZATION NOTE (nsICookieManagerRemoveDeprecated): don't localize nsICookieManager.remove() and originAttributes. > +nsICookieManagerRemoveDeprecated="'nsICookieManager.remove()' is changed. Update your code and pass the correct originAttributes. Read more on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager' What are your thoughts on how this should be backported to Aurora with this string change?

> Can you explain why it doesn't? Fixed. > What are your thoughts on how this should be backported to Aurora with this > string change? Well.. I don't know. I need to backport this to Aurora. Maybe it's ok to have a non localized message for aurora and one localized for central?

Comment on attachment 8738072 [details] [diff] [review] remove.patch Sorry, I won't have time to review this before Apr 14, and perhaps not for a while after it either. Redirecting.

(In reply to Andrea Marchesini (:baku) from comment #8) > > What are your thoughts on how this should be backported to Aurora with this > > string change? > > > Well.. I don't know. I need to backport this to Aurora. Maybe it's ok to > have a non localized message for aurora and one localized for central? It's okay to have a non-localized message for Aurora. Its better to have non-localized message than to break the addon. We need to get this reviewed and uplifted as soon as possible.

Tracking for Firefox 47 because without this we will break some addons. See bug 1257484.

backed out for assertion failures like https://treeherder.mozilla.org/logviewer.html#?job_id=25410107&repo=mozilla-inbound

https://treeherder.mozilla.org/logviewer.html#?job_id=25411124&repo=mozilla-inbound seems to be related too

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

Backed out for various test failures/crashes. Looks like the same ones as last time. Please test your patches more thoroughly before pushing and breaking trees. https://hg.mozilla.org/integration/mozilla-inbound/rev/89c24de1de01 https://treeherder.mozilla.org/logviewer.html#?job_id=25466493&repo=mozilla-inbound https://treeherder.mozilla.org/logviewer.html#?job_id=25465991&repo=mozilla-inbound

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb000abdd76a&selectedJob=19226475 Sorry for this backout and push loop. My tree was out of sync with inbound.

Comment on attachment 8738072 [details] [diff] [review] remove.patch Review of attachment 8738072 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/locales/en-US/necko.properties @@ +44,5 @@ > # %1$S is the deprected API; %2$S is the API function that should be used. > APIDeprecationWarning=Warning: '%1$S' deprecated, please use '%2$S' > + > +# LOCALIZATION NOTE (nsICookieManagerRemoveDeprecated): don't localize nsICookieManager.remove() and originAttributes. > +nsICookieManagerRemoveDeprecated="'nsICookieManager.remove()' is changed. Update your code and pass the correct originAttributes. Read more on MDN: https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager' This string has several issues: there's an opening stray ", there a trailing stray ', it shouldn't use en-US in the link to MDN, and it should use “” for the code. nsICookieManagerRemoveDeprecated=“nsICookieManager.remove()” is changed. Update your code and pass the correct originAttributes. Read more on MDN: https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager Any chance to land a quick fix without a new string ID?

Comment on attachment 8739942 [details] [diff] [review] remove2.patch Review of attachment 8739942 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, thanks Andrea.

Hi, I understand the reason of this change, but it really make things worse I think. If I'm not mistaken, we have now: ESR or older Gecko: remove(aHost, aName, aPath, aBlocked); Stable: remove(aHost, aName, aPath, aOriginAttributes, aBlocked); Beta - Nightly: remove(aHost, aName, aPath, aBlocked, aOriginAttributes); Future (Nightly+1?): remove(aHost, aName, aPath, aOriginAttributes, aBlocked); As an add-on developer, how I am suppose to call the function with the right parameter, when I support not only Firefox, from ESR to Nightly, but also older Gecko version found in addons like Palemoon etc. Actually I test if typeof(cookie.originAttributes) == "object" to call the method for the current stable. But in Beta and nightly, now, the domain is always added to the block list. This is really annoying. Do you have a solution for my problem ? -- https://addons.mozilla.org/addon/cookiekeeper/

[fix mistakes in the previous comment] Hi, I understand the reason of this change, but it really makes things worse I think. If I'm not mistaken, we have now: ESR or older Gecko: remove(aHost, aName, aPath, aBlocked); Stable: remove(aHost, aName, aPath, aOriginAttributes, aBlocked); Beta - Nightly: remove(aHost, aName, aPath, aBlocked, aOriginAttributes); Future (Nightly+1?): remove(aHost, aName, aPath, aOriginAttributes, aBlocked); As an add-on developer, how I am suppose to call the function with the right parameters, when I support not only Firefox, from ESR to Nightly, but also older Gecko version found in browsers like Palemoon etc. Actually I test if typeof(cookie.originAttributes) == "object" to call the method for the current stable. But in Beta and nightly, now, the domain is always added to the block list. This is really annoying. Do you have a solution for my problem ? -- https://addons.mozilla.org/addon/cookiekeeper/

> If I'm not mistaken, we have now: You are right. But I'm planning to uplift this patch to aurora and beta so that the new setup will be: Stable - ESR - older Gecko: remove(aHost, aName, aPath, aBlocked); Aurora - Beta - Nightly: remove(aHost, aName, aPath, aBlocked, aOriginAttributes);

Comment on attachment 8738072 [details] [diff] [review] remove.patch Review of attachment 8738072 [details] [diff] [review]: ----------------------------------------------------------------- I would like to uplift this patch in order to be back compatible with stable releases. I can provide a new version of this patch without localization if this is needed.

This patch includes a string. I strongly suggest to create a separate patch without string changes (hard-coded English message) for uplift.

(In reply to Andrea Marchesini (:baku) from comment #29) > > Stable - ESR - older Gecko: remove(aHost, aName, aPath, aBlocked); > Aurora - Beta - Nightly: remove(aHost, aName, aPath, aBlocked, > aOriginAttributes); This mean that soon, only Firefox 45.* will have remove(aHost, aName, aPath, aOriginAttributes, aBlocked) ? Also the documentation at https://developer.mozilla.org/docs/Mozilla/Tech/XPCOM/Reference/Interface/nsICookieManager need to be updated. With a note on the changes / revert too if possible. To exactly know what we have on each version.

Approval Request Comment [Feature/regressing bug #]: bug 1245184 [User impact if declined]: addons will have a non-compatible nsICookieManager::remove() method [Risks and why]: The code is relative simple, and we should take this patch. [String/UUID change made/needed]: none

Tracking to keep an eye on last minute uplift

I see that this would be good for addon developers in the next couple of releases, but I'm uneasy about uplifting this so late in beta. Can you suggest a 2nd reviewer? And, is there some way we can test this at the last minute after beta 11 lands on Friday?

Comment on attachment 8740104 [details] [diff] [review] remove.patch Add-on compat issue, Aurora47+

Comment on attachment 8740104 [details] [diff] [review] remove.patch Review of attachment 8740104 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

That didn't answer any of my questions. How will we test this before the 46 release, and who will do that testing? We can uplift this to beta 11 and give it a try. Kev, jorge, what do you think?

has problems to apply renamed 1259169 -> remove.patch applying remove.patch patching file devtools/server/actors/storage.js Hunk #1 FAILED at 845 Hunk #2 FAILED at 888 2 out of 2 hunks FAILED -- saving rejects to file devtools/server/actors/storage.js.rej patch failed, unable to continue (try -v) patch failed, rejects left in working directory errors during apply, please fix and qrefresh remove.patch btw can we avoid here having 2 patches with the same name "remove.patch" that could result in confusion/errors :)

Patch rebased for m-a.

https://hg.mozilla.org/releases/mozilla-aurora/rev/949d0760be73

My concern is that we feel like we're fixing addon compatibility here, but since this comes a week before we release (effectively) it may actually break compat for developers who already fixed their addons and tested them earlier in beta.

It looks like the patch would only add an optional argument to the existing function, so that wouldn't break backward compatibility. It would help in forward compatibility because it gives developers more time to use the new API in advance. So I'm favor of uplifting this change.

Comment on attachment 8740104 [details] [diff] [review] remove.patch Great, let's do it, this can still land for beta 11.

I'm hitting a lot of conflicts trying to uplift the aurora version of the patch. Could we get a beta version of the patch?

Comment on attachment 8740104 [details] [diff] [review] remove.patch Sorry, my fault! We don't need this patch for beta. Beta is still using the old remove(). The patch is to make the 'new' remove() compatible with that we have in beta and in release.

(In reply to Andrea Marchesini (:baku) from comment #47) > Comment on attachment 8740104 [details] [diff] [review] > remove.patch > > Sorry, my fault! We don't need this patch for beta. Beta is still using the > old remove(). The patch is to make the 'new' remove() compatible with that > we have in beta and in release. Per baku, this patch isn't necessary for beta/Firefox 46. The new remove() method that is what is breaking addons didn't land until Firefox 47 (https://bugzilla.mozilla.org/show_bug.cgi?id=1245184). So I'm removing the Firefox 46 tracking flags.

Jorge, do we want to write a blog post about this compatibility change for addon developers? With this patch we have a console warning message. Is it enough?

It's mentioned in the compatibility blog post: https://blog.mozilla.org/addons/2016/04/07/compatibility-for-firefox-47/. The console warning should be enough for the transition, thanks.

We could add a release note with a link to the post for 47.

Added to Fx47 (beta) release notes.

Hi, Andrea, this came up on our radar for Fx47. Could you please elaborate a bit on what QE can do here to help? We're not sure exactly how to confirm the patch pushed for this bug. Thank you.

This patch makes nsICookieManager::remove() back compatible with what we had in previous Firefox releases. It means that we can delete cookies without passing originAttributes as parameter. We did that because otherwise we would break several addons. In order to test it, you can see if nsICookieManager::remove() works without originAttributes and if the selected cookies are correctly removed from the UI, or programmatically.

I have tested this issue using "Remove Cookies for Site" add-on that was last updated in 2011 to check if it would erase the cookies on Firefox 47.0b9 on a Windows 10 x64 machine. I have used the add-on to erase cookies on the first 20 Alexa top sites and all worked fine, except for Bing.com. On Bing the cookies are erased but as soon as I go to check them in about:preferences#privacy they are right back. I expect that this is intended. Andrea, would you consider this as being VERIFIED FIXED?

Yes. I consider it as VERIFIED FIXED. Thanks!

Verified as fixed in Firefox 48.0b2 (20160620091522) on Windows 7 x64 and Windows 10 x64.