1214071 - Add APIs get/removeCookiesWithOriginAttributes() in nsICookieManager2.idl

Status: RESOLVED FIXED

(Core :: Networking: Cookies, defect, P1)

Description

[Ethan Tseng [:ethan]]

This is a follow-up of bug 1165267 (Use OriginAttributes for nsCookieService).

We shall add two new interfaces in nsICookieManager2.idl to replace the original get/removeCookiesForApp().
- getCookiesForOriginAttributes()
- removeCookiesForOriginAttributes()

Comment 1

[Ethan Tseng [:ethan]]

This pair of functions should only be used while uninstalling apps. They are not so critical for now. Set priority as P4.

Comment 2

[Ethan Tseng [:ethan]]

Attachment: bug-1214071-wip.patch

I think we cannot remove the original get/removeCookiesForApp() functions because they are used by extensions. But we could add two new functions for app uninstallation, which is done in dom/apps/Webapps.jsm.

Bobby, could you provide feedback on this IDL change?

Comment 3

[Bobby Holley (:bholley)]

Comment on attachment 8672900 [details] [diff] [review]
bug-1214071-wip.patch

Review of attachment 8672900 [details] [diff] [review]:
-----------------------------------------------------------------

Looks like only the b2g simulator uses {get,remove}CookiesForApp:

https://mxr.mozilla.org/addons/search?string=getCookiesForApp
https://mxr.mozilla.org/addons/search?string=removeCookiesForApp

So I'd say we should just remove those two methods.

Comment 4

[Ethan Tseng [:ethan]]

(In reply to Bobby Holley (:bholley) from comment #3)
> Comment on attachment 8672900 [details] [diff] [review]
> Looks like only the b2g simulator uses {get,remove}CookiesForApp:
> So I'd say we should just remove those two methods.

Thanks Bobby for your prompt response!

I have another question. I don't know how to pass jsval in C++.
Could you give me some hint, such as sample code somewhere?

Comment 5

[Bobby Holley (:bholley)]

If you need to call this from C++, you should have a separate API entry point that doesn't go through XPIDL.

A good example here is the way we create codebase principals. nsIScriptSecurityManager::createCodebasePrincipal is the script-accessible side, and that forwards to BasePrincipal::CreateCodebasePrincipal, which C++ callers use directly.

Comment 6

[Ethan Tseng [:ethan]]

(In reply to Bobby Holley (:bholley) from comment #5)
> A good example here is the way we create codebase principals.
> nsIScriptSecurityManager::createCodebasePrincipal is the script-accessible
> side, and that forwards to BasePrincipal::CreateCodebasePrincipal, which C++
> callers use directly.

Thank you Bobby!
I'll follow your way to do this.

Comment 7

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO]

make this blocks Bug 1233136 as removeCookiesForOriginAttributes should also consider pattern matching in clear-origin-data.

Comment 8

[Ethan Tseng [:ethan]]

While working on this bug, we found these two lines are not necessary:
https://dxr.mozilla.org/mozilla-central/source/dom/apps/Webapps.jsm#704
  permMgr.removePermissionsWithAttributes(JSON.stringify(attrs));
  Services.cookies.removeCookiesForApp(localId, false);

nsPermissionManager and nsCookieService both observe the "clear-origin-data" event, which call their RemovePermissionsWithAttributes() and RemoveCookiesForApps() respectively.

I commented out these two lines and ran TreeHerder:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9b4624f44466

I think we can remove these two lines of codes.

Fabrice, do you have any concern about this?

Comment 9

[[:fabrice] Fabrice Desré]

That seems fine to me, go ahead!

Comment 10

[Ethan Tseng [:ethan]]

Attachment: Add RemoveCookiesWithAttributes in CookieManager

This patch adds RemoveCookiesWithAttributes in nsCookieService.
Basic manual tests was executed.
Need to write some automation test cases.

Comment 11

[Ethan Tseng [:ethan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f7f66deb405

Comment 12

[Tanvi Vyas[:tanvi]]

Ethan, can you flag this bug for review?  Thanks!

Comment 13

[Ethan Tseng [:ethan]]

(In reply to Tanvi Vyas [:tanvi] from comment #12)
> Ethan, can you flag this bug for review?  Thanks!

Hi Tanvi, sorry I didn't set aside time to write the test patch yet.
And I suppose to raise the review flag when I have test case.
Does this bug block anything on you? Is it at high stake now?
If so, maybe I can try to land the patch first and add test cases as a follow-up.

Comment 14

[Tanvi Vyas[:tanvi]]

(In reply to Ethan Tseng [:ethan] from comment #13)
> (In reply to Tanvi Vyas [:tanvi] from comment #12)
> > Ethan, can you flag this bug for review?  Thanks!
> 
> Hi Tanvi, sorry I didn't set aside time to write the test patch yet.
> And I suppose to raise the review flag when I have test case.
> Does this bug block anything on you? Is it at high stake now?
> If so, maybe I can try to land the patch first and add test cases as a
> follow-up.

Hi Ethan,

I'm not sure how or if this blocks the user context id (containers) project, since I'm not familiar with this code.  I am needinfo'ing baku to look at this in more detail.

I have also needinfo'ed Dave since I believe he had some comments on the patch.

Comment 15

[Andrea Marchesini [:baku]]

As far as I can tell, we are not blocked by this. CookieManager is working correctly for containers in desktop and we don't have needs for such methods.

Comment 16

[Ethan Tseng [:ethan]]

(In reply to Andrea Marchesini (:baku) from comment #15)
> As far as I can tell, we are not blocked by this. CookieManager is working
> correctly for containers in desktop and we don't have needs for such methods.

Thank you Andrea!
Then I'll stick to my original plan to finish this bug with a test patch. Thanks!

Comment 17

[Dave Huseby [:huseby]]

I don't think this patch is needed because:

1) appId is going away.
2) the existing cookie isolation code seems to be doing the right thing already.

Unless I'm missing something, I don't think this change is needed.

Comment 18

[[:fabrice] Fabrice Desré]

(In reply to Dave Huseby [:huseby] from comment #17)
> I don't think this patch is needed because:
> 
> 1) appId is going away.

From where exactly?

Comment 19

[Ethan Tseng [:ethan]]

(In reply to Dave Huseby [:huseby] from comment #17)
> I don't think this patch is needed because:
> 1) appId is going away.
> 2) the existing cookie isolation code seems to be doing the right thing
> already.
> Unless I'm missing something, I don't think this change is needed.

As I know, appId is not gone. It is encapsulated in OriginAttribute.
Although Firefox Desktop does not need this patch (seems it uses other ways to clean up cookies), I think this patch is still needed for potential use in the future.

Comment 20

[Ethan Tseng [:ethan]]

Attachment: Add RemoveCookiesWithAttributes in CookieManager

Unbitrotted the patch.

Jonathan, thanks for taking this bug!

Comment 21

[Tanvi Vyas[:tanvi]]

Assume we have pass the following OriginAttributes into the get/removeCookiesForOriginAttributes() method:
usercontext id = 1
appid = 5

Will this get/remove all cookies with appid = 5?  Or all cookies with appid =5 && user context =1?  I assume the later.

Comment 22

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi] OOO]

(In reply to Tanvi Vyas - behind on needinfos [:tanvi] from comment #21)
> Assume we have pass the following OriginAttributes into the
> get/removeCookiesForOriginAttributes() method:
> usercontext id = 1
> appid = 5
> 
> Will this get/remove all cookies with appid = 5?  Or all cookies with appid
> =5 && user context =1?  I assume the later.

Yes, it will be the later one. (appId=5 && userContext=1)
It will use OriginAttributesPattern.Matches to do the matching.
http://lxr.mozilla.org/mozilla-central/source/caps/BasePrincipal.h#156

Comment 23

[Tim Huang[:timhuang]]

The devtools might need this because right now Devtools depends on the getCookiesFromHost of the nsICookieManager2 to acquire cookies for a specific host. However, the getCookiesFromHost only use default context id to access cookies which will cause devtools cannot get correct cookies when user context id involves. So devtools should use getCookiesFromOriginAttributes to acquire cookies with correct user context id.

Comment 24

[Jonathan Hao (inactive) [:jhao]]

Hi Ethan,

In the summary and previous comments, the API's name is "get/removeCookiesForOriginAttributes()", but in your patch, its name is removeCookiesWithAttributes().

Is this intentional?

Comment 25

[Ethan Tseng [:ethan]]

(In reply to Jonathan Hao [:jhao] from comment #24)
> Hi Ethan,
> In the summary and previous comments, the API's name is
> "get/removeCookiesForOriginAttributes()", but in your patch, its name is
> removeCookiesWithAttributes().
> Is this intentional?

Yes, it was intentional.
The reason is because nsIPermissionManager uses the name |removePermissionsWithAttributes|,
and I just wanted to be consistent with it.
However, I think |Attributes| alone is too general so I decided to use |OriginAttributes|.

If there is no objection, let's use |get/removeCookiesWithOriginAttributes|.
Thanks!

Comment 26

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for get/removeCookiesWithOriginAttributes() in nsICookieManager2

This is a test for removeCookiesWithOriginAttributes().

By this test, I found a bug in Ethan's patch.  

> for (auto iter = mDBState->hostTable.Iter(); !iter.Done(); iter.Next()) {
>     nsCookieEntry* entry = iter.Get();
> 	
>     if (!aPattern.Matches(entry->mOriginAttributes)) {
>       continue;
>     }
> 	
>     // Pattern matches. Delete all cookies within this nsCookieEntry.
>     const nsCookieEntry::ArrayType& cookies = entry->GetCookies();
> 	
>     for (nsCookieEntry::IndexType i = 0; i < cookies.Length(); ++i) {
>       nsCookie *cookie = cookies[i];
> 	
>       nsAutoCString host;
>       cookie->GetHost(host);
> 	
>       nsAutoCString name;
>       cookie->GetName(name);
> 	
>       nsAutoCString path;
>       cookie->GetPath(path);
> 	
>       // Remove the cookie.
>       Remove(host, entry->mOriginAttributes, name, path, false);
>     }
> 	
> }

Removing elements in a data structure while traversing it can cause problems. In fact, I created three cookies in my test, and the second one is not removed because of this bug.

I'll fix this bug, and also add an implementation of GetCookiesWithOriginAttributes() in later patches.

Comment 27

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=81742b64fc79

Comment 28

[Jonathan Hao (inactive) [:jhao]]

Attachment: Replace Get/RemoveCookiesForApp() with Get/RemoveCookiesWithOriginAttributes()

I fixed the bug mentioned in Comment 26, and added GetCookiesWithOriginAttributes().

Comment 29

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for Get/RemoveCookiesWithOriginAttributes()

Browser tests for Get/RemoveCookiesWithOriginAttributes().

Comment 30

[Ethan Tseng [:ethan]]

(In reply to Jonathan Hao [:jhao] from comment #26)
> By this test, I found a bug in Ethan's patch.  
> Removing elements in a data structure while traversing it can cause
> problems. In fact, I created three cookies in my test, and the second one is
> not removed because of this bug.
Nice catch!
Thanks, Jonathan!

Comment 31

[Jonathan Hao (inactive) [:jhao]]

Attachment: Replace get/removeCookiesForApp() with get/removeCookiesWithOriginAttributes()

Hi Ehsan, could you review these patches? Thanks.

Comment 32

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for get/removeCookiesWithOriginAttributes()

Also this one. Thanks.

Comment 33

[(no longer active)]

Comment on attachment 8749526 [details] [diff] [review]
Replace get/removeCookiesForApp() with get/removeCookiesWithOriginAttributes()

Review of attachment 8749526 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good but I'd like to get an answer about the Remove() question below before r+ing...

::: netwerk/cookie/nsCookieService.cpp
@@ +4347,5 @@
>      NS_WARNING("No DBState! Profile already closed?");
>      return NS_ERROR_NOT_AVAILABLE;
>    }
>  
> +  if (aPattern.mAppId.WasPassed() && aPattern.mAppId.Value() != NECKO_UNKNOWN_APP_ID) {

I think you meant to check for equality here, not inequality, otherwise you're changing the meaning of this check!

@@ +4420,5 @@
> +      nsresult rv = Remove(host, entry->mOriginAttributes, name, path, false);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      // If the array size remains the same after removal, something wrong happens
> +      // Return to avoid infinite loops

Shouldn't Remove() above return a failure code in this case?  I'd like to understand why this check is needed -- did you hit this issue while testing?

::: netwerk/cookie/nsICookieManager2.idl
@@ -132,2 @@
>     */
> -  void removeCookiesForApp(in unsigned long appId,

This is used by a bunch of b2g add-ons such as the simulators: <https://mxr.mozilla.org/addons/search?string=removeCookiesForApp&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hitlimit=&tree=addons>  I'm hoping that we won't need to update those consumers...

Comment 34

[Jonathan Hao (inactive) [:jhao]]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #33)
> @@ +4420,5 @@
> > +      nsresult rv = Remove(host, entry->mOriginAttributes, name, path, false);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      // If the array size remains the same after removal, something wrong happens
> > +      // Return to avoid infinite loops
> 
> Shouldn't Remove() above return a failure code in this case?  I'd like to
> understand why this check is needed -- did you hit this issue while testing?

Ehsan, thanks for your review.

In Remove(), if we can't find a cookie with the specified (host, origin attributes, name, path), it does not return a failure code, and the outside loop won't terminate. Nonetheless, if removeCookiesWithOriginAttributes() works correctly, this cannot happen because we always pass an existing cookie's information to Remove().

If you think the check is redundant, I can remove it.

Comment 35

[Jonathan Hao (inactive) [:jhao]]

(In reply to :Ehsan Akhgari (busy, don't ask for review please) from comment #33)
> ::: netwerk/cookie/nsICookieManager2.idl
> @@ -132,2 @@
> >     */
> > -  void removeCookiesForApp(in unsigned long appId,
> 
> This is used by a bunch of b2g add-ons such as the simulators:
> <https://mxr.mozilla.org/addons/
> search?string=removeCookiesForApp&find=&findi=&filter=%5E%5B%5E%5C0%5D*%24&hi
> tlimit=&tree=addons>  I'm hoping that we won't need to update those
> consumers...

Hi Fabrice,
Do we need to update those b2g add-ons?

Comment 36

[[:fabrice] Fabrice Desré]

Hi Jonathan,

No you don't have to update these add-ons.

Comment 37

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=89f0f1e8df62

Comment 38

[Jonathan Hao (inactive) [:jhao]]

Attachment: Tests for Get/RemoveCookiesWithOriginAttributes()

Update commit message.

Comment 39

[Jonathan Hao (inactive) [:jhao]]

Attachment: Replace Get/RemoveCookiesForApp() with Get/RemoveCookiesWithOriginAttributes()

Thanks, Fabrice.

Ehsan, please review again. Thanks.

Comment 40

[Jonathan Hao (inactive) [:jhao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=99aeb8c834a0

Comment 41

[Jonathan Hao (inactive) [:jhao]]

Attachment: Replace Get/RemoveCookiesForApp() with Get/RemoveCookiesWithOriginAttributes()

I saw a build failure on Windows, so I fixed a return type.

Comment 42

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7b6472147fdf
https://hg.mozilla.org/integration/mozilla-inbound/rev/29944095078e

Comment 43

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7b6472147fdf
https://hg.mozilla.org/mozilla-central/rev/29944095078e