User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/48.0.2564.116 Chrome/48.0.2564.116 Safari/537.36 Steps to reproduce: On an ubuntu 14.04 machine , I added my twitter account, the API authentication failed due to internet failure, then I tried to delete my account from the list by selecting the account and clicking on delete . Actual results: The account did not get deleted from the list. Expected results: The account should have got deleted from the list .

Moving to the right component.

The nsICookieManager::remove method uses 5 arguments for removing the cookie instead of 4 arguments in previous version .

Comment on attachment 8728619 [details] [diff] [review] the code in " twitter.js " file , located in /chat/protocols/twitter had used less arguments for Session.cookies.remove method , which is used to delete cookies when deleting an account , causing t Review of attachment 8728619 [details] [diff] [review]: ----------------------------------------------------------------- ::: chat/protocols/twitter/twitter.js @@ +893,5 @@ > // during the same session (bug 954308). > let cookies = Services.cookies.getCookiesFromHost("twitter.com"); > while (cookies.hasMoreElements()) { > let cookie = cookies.getNext().QueryInterface(Ci.nsICookie2); > + Services.cookies.remove(cookie.host, cookie.name, cookie.path,cookie.originAttributes, false); Thought I'd do a drive by review! nit: Please add a space after the comma. Even better: this line is now quite long, could you break it up? eg: Services.cookies.remove(cookie.host, cookie.name, cookie.path, cookie.originAttributes, false); Please refer to https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style for our coding style guidelines.

I have edited the mdn docs related to nsICookieManager::remove method , which is used to remove cookies . The twitter account was not getting deleted as to delete account we need to delete cookies and to delete cookies the nsICookieManager::remove was being called with fewer arguments than required. A new jsVal relating to cookie origins was added in the nsICookieManager::remove method , so this was a problem occuring due to code regression.

Comment on attachment 8729980 [details] [diff] [review] Deleting account , requires deleting the cookies. The method nsICookieManager::remove() is called to remove cookies. In twitter.js the method was called with fewer arguments ( 4 instead of 5 ) Review of attachment 8729980 [details] [diff] [review]: ----------------------------------------------------------------- Code looks good! Please follow the instructions [1] on how to prepare a patch with the correct header and formatting for checkin. (Basically, run mach mercurial-setup, check your hg config, add a commit message in the right format and then use hg export or hg bzexport to submit the patch.) [1] https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F ::: chat/protocols/twitter/twitter.js @@ +893,5 @@ > // during the same session (bug 954308). > let cookies = Services.cookies.getCookiesFromHost("twitter.com"); > while (cookies.hasMoreElements()) { > let cookie = cookies.getNext().QueryInterface(Ci.nsICookie2); > + Services.cookies.remove(cookie.host, cookie.name, cookie.path, Nit: Please avoid adding trailing whitespace. (Most editors have a setting you can turn on that will automatically avoid this.)

I have edited the mdn docs related to nsICookieManager::remove method , which is used to remove cookies . The twitter account was not getting deleted as to delete account we need to delete cookies and to delete cookies the nsICookieManager::remove was being called with fewer arguments than required. A new jsVal relating to cookie origins was added in the nsICookieManager::remove method , so this was a problem occuring due to code regression.

Comment on attachment 8729986 [details] [diff] [review] Deleting account , requires deleting the cookies. The method nsICookieManager::remove() is called to remove cookies. In twitter.js the method was called with fewer arguments ( 4 instead of 5 ) Review of attachment 8729986 [details] [diff] [review]: ----------------------------------------------------------------- Your commit message Bug 1255177- Modified Services.cookies.remove() to use appropriate arguments isn't right - you didn't modify the remove function. It would be more precise e.g. like this: Bug 1255177 - Add originAttributes argument to Services.cookies.remove calls in chat/.

I have edited the mdn docs related to nsICookieManager::remove method , which is used to remove cookies . The twitter account was not getting deleted as to delete account we need to delete cookies and to delete cookies the nsICookieManager::remove was being called with fewer arguments than required. A new jsVal relating to cookie origins was added in the nsICookieManager::remove method , so this was a problem occuring due to code regression.

I have edited the mdn docs related to nsICookieManager::remove method , which is used to remove cookies . The twitter account was not getting deleted as to delete account we need to delete cookies and to delete cookies the nsICookieManager::remove was being called with fewer arguments than required. A new jsVal relating to cookie origins was added in the nsICookieManager::remove method , so this was a problem occuring due to code regression.

modified the commit message to be more appropriate.

Comment on attachment 8729988 [details] [diff] [review] Deleting account , requires deleting the cookies. The method nsICookieManager::remove() is called to remove cookies. In twitter.js the method was called with fewer arguments ( 4 instead of 5 ) Review of attachment 8729988 [details] [diff] [review]: ----------------------------------------------------------------- Thanks!

This patch does not apply on the current c-c tip (abort: bad hunk #1 @@ -889,18 +889,17 @@ Account.prototype = {). Please check and attach a rebased patch!

ok will do that ASAP and submit a new patch.

https://hg.mozilla.org/comm-central/rev/330bcc2089a8007bfde3313475dc7ce1b313a3ef Bug 1255177 - Add originAttributes argument to Services.cookies.remove calls in chat/. r=aleth

Fixed the broken patch file. Did you edit it by hand? Always a bad idea ;)

We should uplift this to 47, which is when the blocker landed.

Comment on attachment 8729988 [details] [diff] [review] Deleting account , requires deleting the cookies. The method nsICookieManager::remove() is called to remove cookies. In twitter.js the method was called with fewer arguments ( 4 instead of 5 ) [Approval Request Comment] Regression caused by (bug #): 1245184 User impact if declined: see bug title Risk to taking this patch (and alternatives if risky): can't get more broken than it is.

Fixed patch file, as landed.

It was my first patch, and there were other unnecessary files which were coming in the patch, so I had edited it by hand (a very bad practice, sorry ). I was trying to make a fresh patch, but my build had broken ( now it is fixed ). Thanks for fixing my patch. Also I ran a simple search for "cookies.remove" on dxr in c-c, pointing out many such files involving obsolete usage for this same method. Shouldn't we fix them also ?

(In reply to sourab.reddy2k14 from comment #19) > Also I ran a simple search for "cookies.remove" on dxr in c-c, pointing out > many such files involving obsolete usage for this same method. Shouldn't we > fix them also ? Well spotted. Yes you are right, and that's already being done in bug 1256153 and bug 1251368.

Aurora (TB 47): https://hg.mozilla.org/releases/comm-aurora/rev/05bc4f17ba6e