1165267 - Use OriginAttributes for nsCookieService

Status: RESOLVED FIXED

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

Description

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

We implemented cookie for apps on B2G in Bug 756648 and Bug 786299.
Now for the new security model we will update origin in Bug 1163254. so for nsICookieManager2 and nsICookieService should use the origin, instead of using appId/isBrowser.

For the new security model it also mentions 'cookie jar id', which should use the nsIPrincipal.originAttribute. But let's see if we need another bug for this.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

I'm a bit confused as to what the goal is here. with nsICookieService, I don't see any reference to appId/isBrowser, and in nsICookieManager2, I see: 

nsISimpleEnumerator getCookiesForApp(in unsigned long appId, in boolean onlyBrowserElement);
and
void removeCookiesForApp(in unsigned long appId, in boolean onlyBrowserElement);

Both of which use "onlyBrowserElement" which uses different semantics than originAttributes usually has (as false is fuzzier). I'm not sure if we would actually want these to be replaced with an originAttributes argument.

Comment 2

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

(In reply to Michael Layzell [:mystor] from comment #1)
> I'm a bit confused as to what the goal is here. with nsICookieService, I
> don't see any reference to appId/isBrowser,

Sorry, should be nsCookieService.cpp/.h

But for nsICookieService,
I am wondering maywe we should convert nsIURI to nsIPrincipal.

> and in nsICookieManager2, I see: 
> 
> nsISimpleEnumerator getCookiesForApp(in unsigned long appId, in boolean
> onlyBrowserElement);
> and
> void removeCookiesForApp(in unsigned long appId, in boolean
> onlyBrowserElement);
> 
> Both of which use "onlyBrowserElement" which uses different semantics than
> originAttributes usually has (as false is fuzzier). I'm not sure if we would
> actually want these to be replaced with an originAttributes argument.

I think these arguments should be converted to cookieJar attribute in nsIPrincipal.
Sorry for not updating the comments.

Comment 3

[Jason Duell]

I'm not aware of a final specification of how the details of cookies will work in this new model.

  https://wiki.mozilla.org/FirefoxOS/New_security_model#Origins_and_cookie_jars_-_bug_1153435

That document has a lot of open questions about various important edge cases: have we sorted out the answers yet?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

We have, sorry that hasn't gotten documented yet.

The short of the story is:
We have a new struct called OriginAttributes which is a generalized form of what we currently use "appid+browserflag" for. Encapsulating this in the OriginAttributes struct will make it easier to modify the contents of this struct in the future.

When storing data, key it on the contents of the OriginAttributes. The easiest way to do that is to stringify the OriginAttributes using the OriginAttributes.CreateSuffix() function.

Or, even better, if you have an nsIPrincipal, you can simply grab nsIPrincipal.originSuffix and use that as additional key in your storage API.

For APIs that store things keyed on origins you can even use nsIPrincipal.origin as the full key. That property will return a string which includes the traditional scheme+host+port, but also includes the OriginAttributes.

But since cookies and http cache aren't keyed on the origin, this doesn't apply for those parts of the code.


Hopefully this should be simpler than the current model since you don't have to worry about what appids and browserflags are. Simply treat OriginAttributes, or its stringified form, as an opaque extra key.

Comment 5

[Jason Duell]

We need to make sure that other use cases that are using appids for namespacing don't get broken by this.  IIRC we use a "fake" appid for fetching Safebrowsing lists from Google, for instance (to avoid sending all the user's google.com cookies are part of the request).  That will need some fake/unique key to keep working.

Comment 6

[Ethan Tseng [:ethan]]

I'll take this bug.

Honza, I NI you just in order to let you know I am working on this.
You could cancel the NI request when you are aware of it. :)

Comment 7

[Ethan Tseng [:ethan]]

[Blocking Requested - why for this release]:
This bug blocks bug 1153435, which is 2.5+, so I guess this should be 2.5+ as well.

Paul,
Should we set all sub bugs of bug 1153435 as 2.5 blockers?

Comment 8

[Honza Bambas (:mayhemer)]

Go ahead :)

Comment 9

[Paul Theriault [:pauljt] (no longer reading bugmail)]

(In reply to Ethan Tseng [:ethan] from comment #7)
> [Blocking Requested - why for this release]:
> This bug blocks bug 1153435, which is 2.5+, so I guess this should be 2.5+
> as well.
> 
> Paul,
> Should we set all sub bugs of bug 1153435 as 2.5 blockers?

Yes I think so.

Comment 10

[Ethan Tseng [:ethan]]

Modify the bug title due to design change.

nsIPrincipal::cookieJar was removed in bug 1182347, and according to comment 4, we will use nsIPrincipal::originAttributes instead of |appId + inBrowserElement| as cookie key.

Comment 11

[Ethan Tseng [:ethan]]

Attachment: bug-1165267-wip.patch

*** Work Note ***
1. Upload a WIP patch.
   It just removes |appId| and |inBrowserElement| from hash keys of nsCookieKey.
   (we should stop always using different cookie jars for different apps).
   The result is the cookie hash key is only |baseDomain|, which is the situation before
   bug 756648 was landed.

2. This patch breaks the following test cases, which need to be addressed.
   - netwerk/test/unit/test_cookiejars.js
   - netwerk/test/unit/test_cookiejars_safebrowsing.js
   - netwerk/test/unit_ipc/test_cookiejars_wrap.js

3. Eventually we should completely remove |appId| and |inBrowserElement| from cookies.
   However in 2.5 we still have to store these two fields in cookie DB (SQLite).

Comment 12

[Ethan Tseng [:ethan]]

*** Work Note ***
As NSec design, addId and inBrowserElement should no longer be used as cookie key.
However, we cannot completely remove addId and inBrowser from nsCookieService implementation because they are required for two APIs:
 - nsICookieManager2::getCookiesForApp()
 - nsICookieManager2::removeCookiesForApp()

Actually GetCookiesForApp() is only used by RemoveCookiesForApp(), which is only called by Webapps.jsm while uninstalling an app. In the long run, maybe we should remove or change these two APIs to reflect the design change of web apps.

Comment 13

[Ethan Tseng [:ethan]]

Fixed typo from comment 12 (typo appId as addId).

*** Work Note ***
As NSec design, appId and inBrowserElement should no longer be used as cookie key.
However, we cannot completely remove appId and inBrowser from nsCookieService implementation because they are required for two APIs:
 - nsICookieManager2::getCookiesForApp()
 - nsICookieManager2::removeCookiesForApp()

Actually GetCookiesForApp() is only used by RemoveCookiesForApp(), which is only called by Webapps.jsm while uninstalling an app. In the long run, maybe we should remove or change these two APIs to reflect the design change of web apps.

Comment 14

[Ethan Tseng [:ethan]]

Attachment: bug-1165267.patch

The latest patch.
It replaces appId and inBrowserElement by nsIPrincipal.originAttributes.

Comment 15

[Ethan Tseng [:ethan]]

When I was working on this bug last week, I thought we should change the behavior of cookies, which means we only use baseDomain as hash key for cookie jars.
But after discussion with Yoshi and Henry, I found my idea was wrong (point 1 in my comment 11).
Just as Jonas said in comment 4, we should replace appId + inBrowserElement by nsIPrincipal.originAttributes.
Thus, the cookie behavior remains the same. This bug is just an internal refactoring to make appId and inBrowserElement transparent to cookies.

However, this patch does not change the DB schema yet. Cookie DB still stores these two fields.

Comment 16

[Henry Chang [:hchang]]

Comment on attachment 8653401 [details] [diff] [review]
bug-1165267.patch

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

Overall looks good to me except that the behavior of RemoveCookieForApps is changed. Thanks
!

::: netwerk/cookie/nsCookieService.cpp
@@ +3980,5 @@
>      nsCookieEntry* entry = iter.Get();
>  
> +    // FIXME: Is this the right way to compare?
> +    mozilla::OriginAttributes originAttr(aAppId, aOnlyBrowserElement);
> +    if (entry->mOriginAttr != originAttr) {

This is not the original purpose.

::: netwerk/cookie/nsCookieService.h
@@ +290,5 @@
>    void                            GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, uint32_t aAppId, bool aInBrowserElement, bool aIsPrivate, nsCString &aCookie);
>      nsresult                      SetCookieStringCommon(nsIURI *aHostURI, const char *aCookieHeader, const char *aServerTime, nsIChannel *aChannel, bool aFromHttp);
> +  void                            SetCookieStringInternal(nsIURI *aHostURI, bool
> +      aIsForeign, nsDependentCString &aCookieHeader, const nsCString
> +      &aServerTime, bool aFromHttp, uint32_t aAppId, bool aInBrowserElement,

"uint32_t aAppId, bool aInBrowserElement" is no longer needed. So is its implementation part.

Comment 17

[Ethan Tseng [:ethan]]

Attachment: bug-1165267.patch

Update the patch to address issues from Henry's feedback (comment 16).

This patch is not tested yet. I will perform unit test and add more patch if necessary.

Comment 18

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

Comment on attachment 8653401 [details] [diff] [review]
bug-1165267.patch

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

There are several parts should be done in this bug AFAICT, so I list it below
1. utils functions
- There should be a function called NS_GetOriginAttributes and its behavior will like NS_GetAppInfo.
- same for NS_GetAppInfoFromClearDataNotification

2. GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, uint32_t aAppId, bool aInBrowserElement, bool aIsPrivate, nsCString &aCookie);
needs to be 
  GetCookieStringInternal(nsIURI *aHostURI, bool aIsForeign, bool aHttpBound, OriginAttributes attrs, bool aIsPrivate, nsCString &aCookie);

So this in turn will need to change its callers like 
- CookieServiceParent::RecvGetCookieString
- CookieServiceParent::GetAppInfoFromParams

3. Likewise for SetCookieStringInternal

4. interfaces inherited from nsICookieManager2.idl
- get/setCookiesForApp

5. Removal
- it should listen clear-origin-data instead of webapps-clear-data
- nsCookieService::Remove should take OriginAttributes instead.
- nsCookieService::RemoveCookiesForApp
- nsCookieService::GetCookiesForApp
- nsCookieService::AppClearDataObserverInit should be renamed to origin-centric

6. Database migration

::: netwerk/cookie/nsCookieService.cpp
@@ +50,5 @@
>  #include "nsIAppsService.h"
>  #include "mozIApplication.h"
>  #include "mozIApplicationClearPrivateDataParams.h"
>  #include "nsIConsoleService.h"
> +#include "nsIScriptSecurityManager.h"

This can be removed. See below

@@ +68,5 @@
>   * useful types & constants
>   ******************************************************************************/
>  
>  static nsCookieService *gCookieService;
>  

during line 91~92
#define IDX_APP_ID 10
#define IDX_BROWSER_ELEM 11
should be changed to IDX_ORIGIN_ATTRIBUTES or IDX_ORIGIN_SUFFIX.

@@ +493,5 @@
>          break;
>  
>        CookieDomainTuple *tuple = mDBState->hostArray.AppendElement();
>        row->GetUTF8String(IDX_BASE_DOMAIN, tuple->key.mBaseDomain);
> +      uint32_t appId = static_cast<uint32_t>(row->AsInt32(IDX_APP_ID));

We don't need IDX_APP_ID anymore, so you should read originSuffix here, and convert it to OriginAttributes.

nsAutoCString suffix;
row->GetUTF8String(IDX_ORIGIN_ATTRIBUTES, suffix);
OriginAttributes attrs;
attrs.PopulateFromSuffix(suffix);
tuple->key.mOriginAttributes = attrs;

@@ +1696,5 @@
> +  rv = secMan->GetChannelResultPrincipal(aChannel, getter_AddRefs(principal));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  const mozilla::OriginAttributes &originAttr =
> +    mozilla::BasePrincipal::Cast(principal)->OriginAttributesRef();

You don't need nsIScriptSecurityManager to do this.

If you have an util called NS_GetOriginAttributes;

OriginAttributes attrs;
if (aChannel) {
  NS_GetOriginAttributes(aChannel, attrs);
}

@@ +1716,5 @@
>                                           nsDependentCString &aCookieHeader,
>                                           const nsCString    &aServerTime,
>                                           bool                aFromHttp,
>                                           uint32_t            aAppId,
>                                           bool                aInBrowserElement,

aAppId/aInBrowserElement not needed.

@@ +2314,3 @@
>    NS_ASSERT_SUCCESS(rv);
>    rv = mDefaultDBState->stmtReadDomain->BindInt32ByName(
> +    NS_LITERAL_CSTRING("inBrowserElement"), aKey.mOriginAttr.mInBrowser ? 1 : 0);

Replace thess with OriginSuffix.

@@ +2417,5 @@
>      // Make sure we haven't already read the data.
>      stmt->GetUTF8String(IDX_BASE_DOMAIN, baseDomain);
>      appId = static_cast<uint32_t>(stmt->AsInt32(IDX_APP_ID));
>      inBrowserElement = static_cast<bool>(stmt->AsInt32(IDX_BROWSER_ELEM));
> +    mozilla::OriginAttributes originAttr(appId, inBrowserElement);

Use the PopulateFromSuffix mentioned before to get OriginAttributes.

@@ +4154,4 @@
>    NS_ASSERT_SUCCESS(rv);
>  
>    rv = params->BindInt32ByName(NS_LITERAL_CSTRING("inBrowserElement"),
> +                               aKey.mOriginAttr.mInBrowser ? 1 : 0);

Same here.

::: netwerk/cookie/nsCookieService.h
@@ +66,2 @@
>      : mBaseDomain(baseDomain)
> +    , mOriginAttr(originAttr)

We usually call it mOriginAttributes.

Comment 19

[Ethan Tseng [:ethan]]

Comment on attachment 8653401 [details] [diff] [review]
bug-1165267.patch

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

Yoshi, thanks for your review and advice. Quite helpful!

I have a major question.
You said to store originSuffix instead of appId and inBrowser in Database.
If I understand correctly, you were suggesting to change the DB schema of cookies?
Is that correct?

::: netwerk/cookie/nsCookieService.cpp
@@ +493,5 @@
>          break;
>  
>        CookieDomainTuple *tuple = mDBState->hostArray.AppendElement();
>        row->GetUTF8String(IDX_BASE_DOMAIN, tuple->key.mBaseDomain);
> +      uint32_t appId = static_cast<uint32_t>(row->AsInt32(IDX_APP_ID));

Are you suggesting to change the DB schema of cookie?

@@ +1696,5 @@
> +  rv = secMan->GetChannelResultPrincipal(aChannel, getter_AddRefs(principal));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  const mozilla::OriginAttributes &originAttr =
> +    mozilla::BasePrincipal::Cast(principal)->OriginAttributesRef();

Yes. I just realized that Necko should avoid knowing about script security manager.
I will change this code snippet to use NS_GetOriginAttributes, which shall be added into nsNetUtil.h/cpp.

@@ +1716,5 @@
>                                           nsDependentCString &aCookieHeader,
>                                           const nsCString    &aServerTime,
>                                           bool                aFromHttp,
>                                           uint32_t            aAppId,
>                                           bool                aInBrowserElement,

I already removed them in the updated patch. :)

::: netwerk/cookie/nsCookieService.h
@@ +66,2 @@
>      : mBaseDomain(baseDomain)
> +    , mOriginAttr(originAttr)

Sure. I'll rename it.

Comment 20

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

(In reply to Yoshi Huang[:allstars.chh] from comment #18)
> 6. Database migration
Yes, as mentioned in (6) we need to update the schema version and do the migration.

Comment 21

[Ethan Tseng [:ethan]]

(In reply to Yoshi Huang[:allstars.chh] from comment #20)
> (In reply to Yoshi Huang[:allstars.chh] from comment #18)
> > 6. Database migration
> Yes, as mentioned in (6) we need to update the schema version and do the
> migration.

Thank you for confirming this.
I will fix all the issues in your comment 18.

Comment 22

[Henry Chang [:hchang]]

This is probably where the assertion failure comes from: https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.cpp#407

Using |GetChannelURIPrincipal| to get the principal from a channel which has null loadcontext will create a principal with unknown app id.

Comment 23

[Ethan Tseng [:ethan]]

(In reply to Henry Chang [:henry] from comment #22)
> This is probably where the assertion failure comes from:
> https://dxr.mozilla.org/mozilla-central/source/caps/nsScriptSecurityManager.
> cpp#407
> Using |GetChannelURIPrincipal| to get the principal from a channel which has
> null loadcontext will create a principal with unknown app id.

Thanks Henry for providing a clue!
I'll figure out how to fix this issue. :)

Comment 24

[Ethan Tseng [:ethan]]

Attachment: Part 1: Add util functions NS_GetOriginAttributes.

This patch is temporary and needs to be updated after bug 1165466 is landed.

Comment 25

[Ethan Tseng [:ethan]]

Attachment: Part 1: Add util functions NS_GetOriginAttributes.

Minor update.

Comment 26

[Ethan Tseng [:ethan]]

Attachment: Part 2: Replace appId and inBrowser by originAttributes.

This patch replaces appId + inBrowserElement by originAttributes.
It fixes most issues addressed in comment 18, except for:
4. Interfaces inherited from nsICookieManager2.idl
5. Removal
6. Database migration

I'll fix these in newer parts of patches.

Comment 27

[Ethan Tseng [:ethan]]

Attachment: Part 2: Replace appId and inBrowser by originAttributes.

Polish the patch.

Comment 28

[Ethan Tseng [:ethan]]

Comment on attachment 8655766 [details] [diff] [review]
Part 2: Replace appId and inBrowser by originAttributes.

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

Yoshi, I plan to do DB migration and Observer change (listen clear-origin-data) in a another part of patch.
I am already working on it now.
Could you help to review this part 2 patch first?

Comment 29

[Ethan Tseng [:ethan]]

Comment on attachment 8655766 [details] [diff] [review]
Part 2: Replace appId and inBrowser by originAttributes.

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

Well, I just realize it's better to request a formal review after I complete all parts and make them pass full tests.
So, cancel r? and request for feedback instead.

Comment 30

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

Comment on attachment 8654799 [details] [diff] [review]
Part 1: Add util functions NS_GetOriginAttributes.

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

Honza is also doing this in Bug 1165269

Comment 31

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

Comment on attachment 8655766 [details] [diff] [review]
Part 2: Replace appId and inBrowser by originAttributes.

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

::: netwerk/cookie/CookieServiceParent.cpp
@@ +138,5 @@
>    if (!valid) {
>      return false;
>    }
>  
> +  // TODO: Bug 1165466 - Use OriginAttributes in nsILoadContext.

The TODO should be put in GetAppInfoFromParams

@@ +181,5 @@
>    // aIsPrivate argument as nsCookieService::SetCookieStringInternal deals
>    // with aIsForeign before we have to worry about nsCookiePermission trying
>    // to use the channel to inspect it.
>    nsCOMPtr<nsIChannel> dummyChannel;
>    CreateDummyChannel(hostURI, appId, isInBrowserElement,

CreateDummyChannel should be updated as well.

@@ +185,5 @@
>    CreateDummyChannel(hostURI, appId, isInBrowserElement,
>                       isPrivate, getter_AddRefs(dummyChannel));
>  
>    // NB: dummyChannel could be null if something failed in CreateDummyChannel.
> +  mozilla::OriginAttributes originAttr(appId, isInBrowserElement);

GetAppInfoFromParams should be updated, and appId/isBrowserElement should be replaced with OriginAttributes.

::: netwerk/cookie/nsCookieService.cpp
@@ +493,5 @@
>  
>        CookieDomainTuple *tuple = mDBState->hostArray.AppendElement();
>        row->GetUTF8String(IDX_BASE_DOMAIN, tuple->key.mBaseDomain);
> +      uint32_t appId = static_cast<uint32_t>(row->AsInt32(IDX_APP_ID));
> +      bool inBrowserElement = static_cast<bool>(row->AsInt32(IDX_BROWSER_ELEM));

you shouldn't store appId/inBrowser anymore.
I presume you'd like to fix this in another part.
I don't know your plan,
but if there is another part, these changes should be in that part, not here.

@@ +1610,5 @@
>    if (aChannel) {
>      NS_GetAppInfo(aChannel, &appId, &inBrowserElement);
>    }
> +  OriginAttributes attrs = NS_GetOriginAttributes(aChannel, appId,
> +                                                  inBrowserElement);

You should use Honza's NS_GetOriginAttributes.
Your version doesn't look quite right.

@@ +2404,5 @@
>      // Make sure we haven't already read the data.
>      stmt->GetUTF8String(IDX_BASE_DOMAIN, baseDomain);
>      appId = static_cast<uint32_t>(stmt->AsInt32(IDX_APP_ID));
>      inBrowserElement = static_cast<bool>(stmt->AsInt32(IDX_BROWSER_ELEM));
> +    OriginAttributes originAttr(appId, inBrowserElement);

nit, originAttrs

@@ +3959,2 @@
>    }
>  

You need to change to listen to clear-origin-data in this patch,
otherwise the modifications here doesn't make any sense.

Comment 32

[Ethan Tseng [:ethan]]

(In reply to Yoshi Huang[:allstars.chh](OOO 9.4 ~ 9.20) from comment #30)
> Comment on attachment 8654799 [details] [diff] [review]
> Part 1: Add util functions NS_GetOriginAttributes.
> Review of attachment 8654799 [details] [diff] [review]:
> -----------------------------------------------------------------
> Honza is also doing this in Bug 1165269

Thanks for your information.
I'll make a temporary patch by copying the util function from Honza's patch, just in order to test cookie codes.

Comment 33

[Ethan Tseng [:ethan]]

Attachment: Part 0: Add util functions NS_GetOriginAttributes.

A temporary patch of adding util function NS_GetOriginAttributes.
This utility will be included in bug 1165269.

Comment 34

[Ethan Tseng [:ethan]]

Attachment: Part 1: Add interfaces to nsICookieManager2.idl

Adds two new functions to nsICookieManager2.idl:
- getCookiesForOriginAttributes
- removeCookiesForOriginAttributes

This patch is intended only for feedback on new interfaces, not for review.
Its implementation will be done in a real patch.

Ehsan, could you provide feedback on this IDL change?

Comment 35

[Ethan Tseng [:ethan]]

Attachment: Replace appId and inBrowser by originAttributes

This patch resolved most issues addressed by Yoshi in comment 18 and comment 31.
I think it's pretty close to completion.
I performed the existing unit tests locally (mochitests in netwerk/test/unit/).
And I need to do more tests to verify the patch, especially the part of DB migration.

Comment 36

[Ethan Tseng [:ethan]]

Comment on attachment 8657740 [details] [diff] [review]
Replace appId and inBrowser by originAttributes

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

Jason, I need to do more verification for my patch. In the meantime, could you help to take a look and provide feedback for me?

Comment 37

[(no longer active)]

(In reply to Ethan Tseng [:ethan] from comment #34)
> Ehsan, could you provide feedback on this IDL change?

Please ask bholley.  Thanks.

Comment 38

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) from comment #37)
> (In reply to Ethan Tseng [:ethan] from comment #34)
> > Ehsan, could you provide feedback on this IDL change?
> Please ask bholley.  Thanks.

Thanks for this info.

I think we could do this IDL change later because:
1. We can still accomplish this bug without this change.
2. There are not many consumers of this pair of interfaces.

I will file a follow-up to track this issue.

Comment 39

[Ethan Tseng [:ethan]]

Attachment: Replace appId and inBrowser by originAttributes

=== Patch update ===
1. Fix some nits and polish the patch.
2. This patch passed xpcshell unit tests in necko and cookies.
   - netwerk/test/unit/*
   - netwerk/test/unit_ipc/*
   - netwer/cookie/test/unit/*
   - netwer/cookie/test/unit_ipc/*
3. Database migration is manually tested and verified.


=== Description about code change ===
I found it's not easy to split the patch into different parts, so I consolidate changes into a single patch. The code change is summarized here.

Summary:
This patch does not change cookie behavior.
It is an internal refactoring to hide the concept of appId/inBrowser from cookies by using nsIPrincipal::originAttributes.

Major code changes:
1. Replace members mAppId and mInBrowserElement by mOriginAttributes in nsCookieKey.
2. Change parameters in several functions.
   - nsCookieService::GetCookieStringInternal()
   - nsCookieService::SetCookieStringInternal()
   - nsCookieService::Remove()
3. Change CookieServiceParent::GetAppInfoFromParams() as GetOriginAttributesFromParams().
4. Listen to "clear-origin-data" event instead of "webapps-clear-data" in nsCookieService.
5. Upgrade cookie SQLite database.
   - Remove appId and inBrowserElement columns
   - Add a new column originAttributes

Comment 40

[Ethan Tseng [:ethan]]

Attachment: cookies.sqlite.org

The cookie SQLite file before my patch (COOKIES_SCHEMA_VERSION = 5).

Comment 41

[Ethan Tseng [:ethan]]

Attachment: cookies.sqlite

The cookie SQLite file after applying my patch (COOKIES_SCHEMA_VERSION = 6).

Comment 42

[Ethan Tseng [:ethan]]

=== Work Note ===

How did I verify cookie database migration?
1. Build and run Firefox Nightly without my patch.
2. Navigate to http://www.amazon.com.
3. Copy the file cookies.sqlite as cookies.sqlite.org (attachment 8659704 [details])
4. Apply the patch (attachment 8659701 [details] [diff] [review]).
5. Build and run Firefox Nightly; navigate to http://www.amazon.com again.
6. Open the file cookies.sqlite (attachment 8659705 [details])
7. Compare the columns between cookies.sqlite and cookie.sqlite.org.
8. Verify appId and inBrowser columns are removed; originAttributes is added.
   Verify the values of rest columns remain the same.

Comment 43

[Ethan Tseng [:ethan]]

Comment on attachment 8659701 [details] [diff] [review]
Replace appId and inBrowser by originAttributes

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

Hi Jason,

You had written and reviewed the codes of cookies, and you also know about new security model.
Thus I think you are quite suitable to review my patch.
Could you please help to review it?

Comment 44

[Jason Duell]

I hate to distract you Honza, but you've been doing the OriginAttributes conversion of some other code, so you know it better than I do.  Can you take this review?

Comment 45

[Honza Bambas (:mayhemer)]

If there is no need of any cookies code deep knowledge, then yes.

Comment 46

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #45)
> If there is no need of any cookies code deep knowledge, then yes.
No. We didn't alter logic of cookies.
Thanks in advance. :)

Comment 47

[Honza Bambas (:mayhemer)]

Comment on attachment 8659701 [details] [diff] [review]
Replace appId and inBrowser by originAttributes

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

r+ with nits fixed.  Before landing some questions should be answered first.

::: netwerk/cookie/CookieServiceParent.cpp
@@ +26,5 @@
>  
>  // Ignore failures from this function, as they only affect whether we do or
>  // don't show a dialog box in private browsing mode if the user sets a pref.
>  void
> +CreateDummyChannel(nsIURI* aHostURI, OriginAttributes aAttrs, bool aIsPrivate,

not passed by reference?

@@ +71,5 @@
>    aIsPrivate = false;
>  
> +  // TODO: Bug 1165466 - Use originAttributes in nsILoadContext.
> +  // After this bug is landed, we should get originAttributes through
> +  // aLoadContext.

so, should this wait for that bug?  if not, is there a bug filed to fix this later?  or, should that be fixed in that bug? (I would be in favor of the first option here.)

::: netwerk/cookie/nsCookieService.cpp
@@ +579,5 @@
> +    MOZ_ASSERT(!nsCRT::strcmp(aTopic, TOPIC_CLEAR_ORIGIN_DATA));
> +
> +    MOZ_ASSERT(XRE_IsParentProcess());
> +    OriginAttributes attrs;
> +    MOZ_ALWAYS_TRUE(attrs.Init(nsAutoString(aData)));

nsDependentString

@@ +586,5 @@
>        = do_GetService(NS_COOKIEMANAGER_CONTRACTID);
>      MOZ_ASSERT(cookieManager);
> +
> +    // TODO: We should add a new interface RemoveCookiesForOriginAttributes in
> +    // nsICookieManager2 and use it instead of RemoveCookiesForApp.

a bug filed?  something tells me it should rather be done in this bug tho.  anyway, I'm ok with leaving it for a followup.

@@ +1193,5 @@
> +          "(baseDomain, originAttributes, name, value, host, path, expiry,"
> +          " lastAccessed, creationTime, isSecure, isHttpOnly) "
> +          "SELECT baseDomain, originAttributes, name, value, host, path, expiry,"
> +          " lastAccessed, creationTime, isSecure, isHttpOnly "
> +          "FROM moz_cookies_old"));

better would be to have a registred scalar function taking appid/inbrowser and spitting out the corresponding suffix.  you will save the loop above and do the update in a single shot.

@@ +1202,5 @@
> +             "DROP TABLE moz_cookies_old"));
> +        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
> +
> +        COOKIE_LOGSTRING(LogLevel::Debug,
> +          ("Upgrade database to schema version 6"));

Upgraded ?

@@ +1705,5 @@
>    bool isForeign = true;
>    mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
>  
> +  // Get originAttributes.
> +  OriginAttributes attrs(NECKO_NO_APP_ID, false);

isn't OriginAttributes attrs() just enough?  I'd rather leave it up to the class to make itself set to default.

@@ +1778,5 @@
>    bool isForeign = true;
>    mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
>  
> +  // Get originAttributes.
> +  OriginAttributes attrs(NECKO_NO_APP_ID, false);

same here

@@ +2168,5 @@
>                          const nsACString &aName,
>                          const nsACString &aPath,
>                          bool             aBlocked)
>  {
> +  OriginAttributes attrs(NECKO_NO_APP_ID, false);

and here

@@ +4056,5 @@
>    for (auto iter = mDBState->hostTable.Iter(); !iter.Done(); iter.Next()) {
>      nsCookieEntry* entry = iter.Get();
>  
> +    if (entry->mOriginAttributes.mAppId != aAppId ||
> +        (aOnlyBrowserElement && !entry->mOriginAttributes.mInBrowser)) {

when you pass aOriginAttributes here, it should be enough to compare mOriginAttributes != aOriginAttributes, no?

@@ +4114,1 @@
>      if (!aOnlyBrowserElement) {

I thought this was removed, that we no longer delete automatically the non-browser data.

Comment 48

[Ethan Tseng [:ethan]]

Comment on attachment 8659701 [details] [diff] [review]
Replace appId and inBrowser by originAttributes

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

Hi Honza,

Thanks for your time reviewing my patch.
I fixed those nits you addressed (will update the patch later) and answered your questions.
Meanwhile, I have several questions to you. Please help me out.

::: netwerk/cookie/CookieServiceParent.cpp
@@ +26,5 @@
>  
>  // Ignore failures from this function, as they only affect whether we do or
>  // don't show a dialog box in private browsing mode if the user sets a pref.
>  void
> +CreateDummyChannel(nsIURI* aHostURI, OriginAttributes aAttrs, bool aIsPrivate,

Right. Should pass by reference.
Thanks!

@@ +71,5 @@
>    aIsPrivate = false;
>  
> +  // TODO: Bug 1165466 - Use originAttributes in nsILoadContext.
> +  // After this bug is landed, we should get originAttributes through
> +  // aLoadContext.

As I know, it could take a while for bug 1165466 to be landed.
I prefer to file a follow-up bug to fix this.

::: netwerk/cookie/nsCookieService.cpp
@@ +579,5 @@
> +    MOZ_ASSERT(!nsCRT::strcmp(aTopic, TOPIC_CLEAR_ORIGIN_DATA));
> +
> +    MOZ_ASSERT(XRE_IsParentProcess());
> +    OriginAttributes attrs;
> +    MOZ_ALWAYS_TRUE(attrs.Init(nsAutoString(aData)));

Okay.

@@ +586,5 @@
>        = do_GetService(NS_COOKIEMANAGER_CONTRACTID);
>      MOZ_ASSERT(cookieManager);
> +
> +    // TODO: We should add a new interface RemoveCookiesForOriginAttributes in
> +    // nsICookieManager2 and use it instead of RemoveCookiesForApp.

You are right. This should be done in this bug ideally.
The reason I didn't do it is, so far I don't know how to pass jsval argument in CPP code.

The interface would be defined as:
[implicit_jscontext]
void removeCookiesForOriginAttributes(in jsval originAttributes);

In AppClearDataObserver in nsCookieService.cpp, we have to pass OriginAttributes to this function.
If you don't mind, I will file a follow-up bug to address this issue.

@@ +1193,5 @@
> +          "(baseDomain, originAttributes, name, value, host, path, expiry,"
> +          " lastAccessed, creationTime, isSecure, isHttpOnly) "
> +          "SELECT baseDomain, originAttributes, name, value, host, path, expiry,"
> +          " lastAccessed, creationTime, isSecure, isHttpOnly "
> +          "FROM moz_cookies_old"));

I am not familiar with SQL scalar function. Could you provide some keyword or example that I can refer to?

@@ +1202,5 @@
> +             "DROP TABLE moz_cookies_old"));
> +        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
> +
> +        COOKIE_LOGSTRING(LogLevel::Debug,
> +          ("Upgrade database to schema version 6"));

Yes. I'll fix the typo.

@@ +1705,5 @@
>    bool isForeign = true;
>    mThirdPartyUtil->IsThirdPartyChannel(aChannel, aHostURI, &isForeign);
>  
> +  // Get originAttributes.
> +  OriginAttributes attrs(NECKO_NO_APP_ID, false);

Yes. We should just use its default constructor. :)

@@ +4056,5 @@
>    for (auto iter = mDBState->hostTable.Iter(); !iter.Done(); iter.Next()) {
>      nsCookieEntry* entry = iter.Get();
>  
> +    if (entry->mOriginAttributes.mAppId != aAppId ||
> +        (aOnlyBrowserElement && !entry->mOriginAttributes.mInBrowser)) {

nsICookieManager2::getCookiesForApp() does not take OriginAttributes as parameter.
I would not like to change this interface. Instead, I plan to add a new pair of functions:
get/removeCookiesForOriginAttributes(), which we mentioned above.

@@ +4114,1 @@
>      if (!aOnlyBrowserElement) {

Okay, I'll remove this if-block as you suggested.
But I am not aware of this (we no longer delete automatically the non-browser data).
Could you provide more info? Such as a bug number.

Comment 49

[Ethan Tseng [:ethan]]

Attachment: Replace appId and inBrowser by originAttributes

Fixed all nits addressed in comment 47, except for:
"better would be to have a registred scalar function taking appid/inbrowser and spitting out the corresponding suffix.  you will save the loop above and do the update in a single shot."

Need more info and time to resolve this issue.

Comment 50

[Honza Bambas (:mayhemer)]

See https://bugzilla.mozilla.org/attachment.cgi?id=8647652&action=diff#a/dom/storage/DOMStorageDBThread.cpp_sec3.  That is an example of a function taking one argument and returning a processed result.  You can of course have two arguments passed into your function as well, the number of args is the second argument to CreateFunction call.

Comment 51

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #50)
Thanks Honza!
I'll improve my patch according to your reference.

Comment 52

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #39)
> 2. This patch passed xpcshell unit tests in necko and cookies.
>    - netwerk/test/unit/*
>    - netwerk/test/unit_ipc/*
>    - netwerk/cookie/test/unit/*
>    - netwerk/cookie/test/unit_ipc/*

I realized there are more cookie unit tests in the directory:
- extensions/cookie/test/unit/
- extensions/cookie/test/unit_ipc/

And I found two defects by running these test cases. test_cookies_2_migration.js and test_cookies_3_migration.js failed because of two defects.

1. Callers of nsCookieService::CreateTable()
   This function is used to create the cookie DB table for the current schema version.
   The case 4 in TryInitDB (upgrade from 4 to 5) cannot call this function because the schema is
   not for version 5.
   Solution: Make it call a new function CreateTableForSchemaVersion5().

2. Default value of originAttributes in database
   This field is an index so it cannot be NULL. If the cookie DB has records with NULL 
   originAttributes, it will result in duplicate records if users use older version of Firefox.
   Solution: Default originAttributes with empty string.

Comment 53

[Ethan Tseng [:ethan]]

Attachment: Part 1: Replace appId and inBrowser by originAttributes v2.

This patch:
1. Fixed all nits addressed in comment 47.
2. Fixed two defects stated in comment 52.
3. Use registered scalar function to update database in a single shot.

Honza,
Sorry to disturb you again. I need your feedback on these changes, especially point 2 and 3.

Comment 54

[Ethan Tseng [:ethan]]

Attachment: Part 1: Replace appId and inBrowser by originAttributes v2.

Remove debug messages.

Comment 55

[Honza Bambas (:mayhemer)]

(In reply to Ethan Tseng [:ethan] from comment #48)
> @@ +4114,1 @@
> >      if (!aOnlyBrowserElement) {
> 
> Okay, I'll remove this if-block as you suggested.
> But I am not aware of this (we no longer delete automatically the
> non-browser data).
> Could you provide more info? Such as a bug number.

I think it's bug 1168777.

Comment 56

[Honza Bambas (:mayhemer)]

Comment on attachment 8664580 [details] [diff] [review]
Part 1: Replace appId and inBrowser by originAttributes v2.

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

::: netwerk/cookie/nsCookieService.cpp
@@ +1348,2 @@
>  nsresult
>  nsCookieService::CreateTable()

This is called only from case 5:?  If so, this should be CreateTableSchemaV6.

Comment 57

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #55)
> I think it's bug 1168777.
Got it. Thanks!

Comment 58

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (not reviewing) (:mayhemer) from comment #56)
> ::: netwerk/cookie/nsCookieService.cpp
> >  nsCookieService::CreateTable()
> This is called only from case 5:?  If so, this should be CreateTableSchemaV6.

No. This function is called from several places in nsCookieService::TryInitDB() and is used to create table for the current schema version. So I'll keep it be named as CreateTable().

Comment 59

[Ethan Tseng [:ethan]]

Comment on attachment 8656360 [details] [diff] [review]
Part 0: Add util functions NS_GetOriginAttributes.

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

Honza, this patch is required for the patch of cookie part.
I copied codes from your patch in bug 1165269 and made a little change in NS_GetOriginAttributes() to make it work.
Can I land it together with the cookie patch?
Which means you need to rebase the patch in bug 1165269.

Comment 60

[Honza Bambas (:mayhemer)]

(In reply to Ethan Tseng [:ethan] from comment #59)
> Comment on attachment 8656360 [details] [diff] [review]
> Part 0: Add util functions NS_GetOriginAttributes.
> 
> Review of attachment 8656360 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Honza, this patch is required for the patch of cookie part.
> I copied codes from your patch in bug 1165269 and made a little change in
> NS_GetOriginAttributes() to make it work.
> Can I land it together with the cookie patch?
> Which means you need to rebase the patch in bug 1165269.

Go forward!  I will easily merge.  Thanks.

Comment 61

[Ethan Tseng [:ethan]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=643cee89ea75

By running Treeherder, I found another test failure to fix. :(

INFO TEST-UNEXPECTED-FAIL | extensions/cookie/test/test_app_uninstall_cookies.html | App should have no cookies - got 1, expected +0
SimpleTest.is@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:267:5
@chrome://mochitests/content/chrome/extensions/cookie/test/test_app_uninstall_cookies.html:167:1
WebappsApplicationMgmt.prototype.receiveMessage@file:///Users/ethan/workspace/mozilla/desktop/mozilla-central/obj-x86_64-apple-darwin14.5.0/dist/Nightly.app/Contents/Resources/components/Webapps.js:1071:9
19 INFO TEST-UNEXPECTED-FAIL | extensions/cookie/test/test_app_uninstall_cookies.html | Number of cookies should not have changed - got 2, expected 1

Comment 62

[Ethan Tseng [:ethan]]

Attachment: Part 0: Add util functions NS_GetOriginAttributes.

Rebase the patch.
1. Refresh comment "r=honzab".
2. Remove temporary code since bug 1165466 was already landed.

Comment 63

[Ethan Tseng [:ethan]]

Attachment: Part 1: Replace appId and inBrowser by originAttributes v2.

Update the patch.
1. Refresh comment "r=honzab".
2. Minor update to fix mochitest failure in 
   extensions/cookie/test/test_app_uninstall_cookies.html.

Comment 64

[Ethan Tseng [:ethan]]

Attachment: Part 1: Replace appId and inBrowser by originAttributes v2.

Comment 65

[Ethan Tseng [:ethan]]

Waiting for the result of Treeherder.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f62ef47166a2

Comment 66

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #65)
> Waiting for the result of Treeherder.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=f62ef47166a2

Lots of tests crash, such as dom/activities/tests/mochi/test_dev_mode_activity.html.
They crash in this assertion:
https://dxr.mozilla.org/mozilla-central/source/docshell/base/nsILoadContext.idl?offset=0#151

It means the originAttributes is null in the load contexts of these test cases.
It also means it is not safe to call this line in NS_GetOriginAttributes():
  loadContext->GetOriginAttributes(aAttributes);

Yoshi, do you have suggestion on how to fix this problem?
Is it possible to assign default value for originAttributes in nsILoadContext?

Comment 67

[Ethan Tseng [:ethan]]

Attachment: bug-1165267-crash.log

This is the stack of crashes mentioned in comment 66.

The first several stack frames are:
15:19:51     INFO -  #01: nsCookieService::GetCookieStringCommon(nsIURI*, nsIChannel*, bool, char**) [netwerk/cookie/nsCookieService.cpp:1752]
15:19:51     INFO -  #02: mozilla::net::HttpBaseChannel::AddCookiesToRequest() [xpcom/string/nsTString.h:806]
15:19:51     INFO -  #03: mozilla::net::nsHttpChannel::AsyncOpen(nsIStreamListener*, nsISupports*) [netwerk/protocol/http/nsHttpChannel.cpp:5033]
15:19:51     INFO -  #04: nsXMLHttpRequest::Send(nsIVariant*, mozilla::dom::Nullable<nsXMLHttpRequest::RequestBody> const&) [dom/base/nsXMLHttpRequest.cpp:2931]
15:19:51     INFO -  #05: nsXMLHttpRequest::Send(JSContext*, mozilla::ErrorResult&) [dom/bindings/ErrorResult.h:184]
15:19:51     INFO -  #06: nsXMLHttpRequest::Send(JSContext*, nsAString_internal const&, mozilla::ErrorResult&) [dom/base/nsXMLHttpRequest.h:464]
15:19:51     INFO -  #07: mozilla::dom::XMLHttpRequestBinding::send [dom/bindings/ErrorResult.h:172]

It seems the channel from nsXMLHttpRequest doesn't set OriginAttributes properly.

Yoshi and Henry, do you have any idea about this?

Comment 68

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

See Bug 1210459, 

try result after applying patch from Bug 1210459
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fa251ba89c2

Comment 69

[Ethan Tseng [:ethan]]

(In reply to Yoshi Huang[:allstars.chh] from comment #68)
> See Bug 1210459, 
> 
> try result after applying patch from Bug 1210459
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fa251ba89c2

Your patch works! It prevents the crash. Thanks!

Comment 70

[Ethan Tseng [:ethan]]

Bug 1210459 and bug 1210508 were both landed. They should prevent crash caused by GetOriginAttributes.
Push my patches to try again.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=541e0b9ca6c9

Comment 71

[Ethan Tseng [:ethan]]

(In reply to Ethan Tseng [:ethan] from comment #70)
> Push my patches to try again.
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=541e0b9ca6c9

The result looks good.

Comment 72

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/966edcb029a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/558925a1e26c

Comment 73

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/966edcb029a7
https://hg.mozilla.org/mozilla-central/rev/558925a1e26c

Comment 74

[Ethan Tseng [:ethan]]

A follow-up bug was filed:
Bug 1214071 - Add APIs get/removeCookiesForOriginAttributes() in nsICookieManager2.

Comment 75

[Ethan Tseng [:ethan]]

Reopen this bug because it caused a regression (bug 1217594 - Cookies are lost when switching recent Nightlies).

My patch doesn't consider the case of cookie database downgrade. I will fix it following Ehsan's suggestion in https://bugzilla.mozilla.org/show_bug.cgi?id=1217594#c9.

Henry is helping to back out the changeset 558925a1e26c right now.

Comment 76

[Henry Chang [:hchang]]

Attachment: BackoutPatch.patch (for checkin-needed)

Comment 77

[(no longer active)]

Comment on attachment 8679917 [details] [diff] [review]
BackoutPatch.patch (for checkin-needed)

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

Have you tested what happens when you go from a build of the current trunk to a build with this patch?!  It seems that you haven't.  As far as I can tell, this deletes all of the cookies for all Nightly users since you are removing a bunch of columns, and the current code on trunk happily deletes the moz_cookies table if it doesn't find those columns, exactly the problem that happens in bug 1217594.  This is unacceptable, we shouldn't break things for all Nightly users like this.

Furthermore, this patch deletes the upgrade code for schema version 5, which makes no sense.  You need to actually write a proper upgrade path to a new version of the schema, since there are already database versions with version 5, and without doing that, the next time that you reland the upgrade code will not run for any of those users.

The backout here is a semantic backout, that is, you need to write new code that upgrades to a new version which restores the previously deleted tables, and populates them with the correct data.

Comment 78

[(no longer active)]

(And the backout needs to be reviewed before being checked in.)

Comment 79

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking 44+ since this seems like an important issue for nightly.

Comment 80

[Liz Henry (:lizzard) (relman/hg->git project)]

I have just heard we are planning to do the merge to aurora tomorrow, a few days early. Mainly, so that QE can run testing for aurora 44 over the weekend. If you have work to land after the merge we will need to uplift it. I would consider this a blocker for the 44 aurora release.

Comment 81

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #77)
> Review of attachment 8679917 [details] [diff] [review]:
> -----------------------------------------------------------------
> The backout here is a semantic backout, that is, you need to write new code
> that upgrades to a new version which restores the previously deleted tables,
> and populates them with the correct data.

Ehsan, my apologies. I was misled by the word "backout".
Now I fully understand your point and the correct way to fix it.
I'll upload a fix patch right away.

Comment 82

[Ethan Tseng [:ethan]]

Attachment: Fix downgrading by restoring appId and inBrowserElement columns.

This patch adds a new schema version (7) to cookie database.

Upgrading from version 6 to 7 simply adds back |appId| and |inBrowserElement| columns and populates their values from |originAttributes|.
The end result is, version 6 is a corrupted version that would make cookie data lost while downgrading from 6 to a lower version. And version 7 fixes this issue.

I verified this patch by switching between my local latest build (version 7) and Nightly 43 (version 5). No cookie record is lost.

Comment 83

[Ethan Tseng [:ethan]]

Comment on attachment 8680567 [details] [diff] [review]
Fix downgrading by restoring appId and inBrowserElement columns.

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

Honza, I didn't take DB schema downgrading into account in the previous patch.
This need to be fixed for Aurora 44. Could you help to review the new patch?

Comment 84

[Honza Bambas (:mayhemer)]

Just to confirm, is the problem just that after we update (with the original faulty patch) we cannot go back to an older version of Fx?  Or is it something deeper I've missed during the review (not manually tested)?

Because I do an update to localStorage data a similar way (pending review) - not backward compatible, so that Aurora+ -> Nightly -> Aurora+ will also make the localStorage data inaccessible (tho not completely lost).  Honestly, doing a channel downgrade on a profile is stupidity anyway.  We never cared much about being compatible in backwards way.  It's usually impossible with these changes, tho doable when strongly desired.

Comment 85

[Honza Bambas (:mayhemer)]

Comment on attachment 8680567 [details] [diff] [review]
Fix downgrading by restoring appId and inBrowserElement columns.

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

::: netwerk/cookie/nsCookieService.cpp
@@ +1239,5 @@
> +
> +        nsAutoCString suffix;
> +        OriginAttributes attrs;
> +        bool hasResult;
> +        while (1) {

while (true) or for (;;)

@@ +1265,5 @@
> +          NS_ASSERT_SUCCESS(rv);
> +
> +          rv = update->ExecuteStep(&hasResult);
> +          NS_ENSURE_SUCCESS(rv, RESULT_RETRY);
> +        }

have you measured speed of this loop?  a function and an UPDATE statement using it might be faster.  This will run for all users up to Release and may influence startup speed.

Comment 86

[(no longer active)]

(In reply to Honza Bambas (:mayhemer) from comment #84)
> Just to confirm, is the problem just that after we update (with the original
> faulty patch) we cannot go back to an older version of Fx?  Or is it
> something deeper I've missed during the review (not manually tested)?

Yes, that's the problem.  I see that Ethan's new patch is now handling things correctly (thanks, Ethan!)  But I didn't review it in detail.

But Ethan, can you please add tests as well?  Look for examples of the way to test this in extensions/cookie/test/unit, see the tests with "migrate" or "migration" in their name.

> Because I do an update to localStorage data a similar way (pending review) -
> not backward compatible, so that Aurora+ -> Nightly -> Aurora+ will also
> make the localStorage data inaccessible (tho not completely lost). 

Oh, please don't do that.  That will result in the exact same issue.  :(

> Honestly, doing a channel downgrade on a profile is stupidity anyway.  We
> never cared much about being compatible in backwards way.  It's usually
> impossible with these changes, tho doable when strongly desired.

Am I missing something?  We have *always* supported downgrading profiles.  It's actually pretty important for our test population, a lot of the people who run Nightly/Aurora/Beta do this quite regularly, and those are probably our most valuable user base, since they provide us with invaluable feedback on prerelease channels, without whom we don't stand a chance at releasing a high-quality Firefox.  It is of extreme importance to not put them through pain that will cause them to leave these prerelease channels, and losing their data is one way to lose them.

Comment 87

[Honza Bambas (:mayhemer)]

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from > > Honestly, doing a channel downgrade on a profile is stupidity anyway.  We
> > never cared much about being compatible in backwards way.  It's usually
> > impossible with these changes, tho doable when strongly desired.
> 
> Am I missing something?  We have *always* supported downgrading profiles. 
> It's actually pretty important for our test population, a lot of the people
> who run Nightly/Aurora/Beta do this quite regularly, and those are probably
> our most valuable user base, since they provide us with invaluable feedback
> on prerelease channels, without whom we don't stand a chance at releasing a
> high-quality Firefox.  It is of extreme importance to not put them through
> pain that will cause them to leave these prerelease channels, and losing
> their data is one way to lose them.

No, you are not missing anything.  I do.  I recall we did this in the past for a lot of code.  We just didn't obey for http cache, but that is something else than cookie like data.

Thanks for the quick feedback, Ehsan.

Comment 88

[Mahendra Potharaju [:mahe]]

[Tracking Requested - why for this release]:

Unblocking for 2.5, as its for N Sec which is not in scope for 2.5.

Comment 89

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (:mayhemer) from comment #85)
> Comment on attachment 8680567 [details] [diff] [review]
> have you measured speed of this loop?  a function and an UPDATE statement
> using it might be faster.  This will run for all users up to Release and may
> influence startup speed.

Right. You told me this in the previous review, I should remember that.
I'll use mozIStorageFunction to update table instead of using a loop.

Comment 90

[Ethan Tseng [:ethan]]

Attachment: Fix downgrading issue by restoring appId and inBrowserElement columns v2. r=honzab

According to reviewer's advice (comment 85), use mozIStorageFunction instead of a loop to execute UPDATE statement, in order to improve upgrade performance.

Honza, I already manually tested my patch, and I assume this patch is already r+'ed by you since I only do the change you suggested. But I still wish you could take a look of the updated patch. Just for safety. :)

Comment 91

[Ethan Tseng [:ethan]]

(In reply to Honza Bambas (:mayhemer) from comment #85)
> have you measured speed of this loop?  a function and an UPDATE statement
> using it might be faster.  This will run for all users up to Release and may
> influence startup speed.

I measured the speed today.

*** Performance measurement result ***
Steps:
1. Run Aurora 43 (cookie version 5)
2. Populate the cookies table with 131 records from real-world websites
3. Run a latest build with my patch (compare v1 and v2 patches)
4. Observe the time spent in case 6 (upgrade cookie version from 6 to 7)

Results: (testing five times)
v1 patch (UPDATE table using a loop)
  2202, 1869, 3413, 2284, 4336 usec
  Avg: 2820.6 usec

v2 patch (UPDATE using registered scalar function)
  1247, 1211, 566, 1193, 1321 usec
  Avg: 1107.6 usec


Honza, the result proves your suggestion has much better performance. Thanks!

Comment 92

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #86)
> But Ethan, can you please add tests as well?  Look for examples of the way
> to test this in extensions/cookie/test/unit, see the tests with "migrate" or
> "migration" in their name.

Ehsan, yes, I would love to add tests (actually I should do it in the first place). But I need more time to complete the task (1~2 days), which might miss the time of Aurora 44 branching.
Do you think we should check in the patch first (I verified it manually)? Or we should add the test case and uplift them together?

Comment 93

[Honza Bambas (:mayhemer)]

Comment on attachment 8681173 [details] [diff] [review]
Fix downgrading issue by restoring appId and inBrowserElement columns v2. r=honzab

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

Disclaimer: not manually tested, leaving up to you.

r=honzab with those few nits fixed

::: netwerk/cookie/nsCookieService.cpp
@@ +911,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  attrs.PopulateFromSuffix(suffix);
> +
> +  nsCOMPtr<nsIWritableVariant> outVar(do_CreateInstance(
> +    NS_VARIANT_CONTRACTID, &rv));

use |new nsVariant()| (just include nsVariant.h)

@@ +1311,5 @@
> +        rv = mDefaultDBState->dbConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +          "UPDATE moz_cookies SET appId = SET_APP_ID(originAttributes), "
> +          "inBrowserElement = SET_IN_BROWSER(originAttributes);"
> +        ));
> +        NS_ENSURE_SUCCESS(rv, RESULT_RETRY);

remove the registered functions here

Comment 94

[(no longer active)]

(In reply to Ethan Tseng [:ethan] from comment #92)
> (In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19]
> from comment #86)
> > But Ethan, can you please add tests as well?  Look for examples of the way
> > to test this in extensions/cookie/test/unit, see the tests with "migrate" or
> > "migration" in their name.
> 
> Ehsan, yes, I would love to add tests (actually I should do it in the first
> place). But I need more time to complete the task (1~2 days), which might
> miss the time of Aurora 44 branching.
> Do you think we should check in the patch first (I verified it manually)? Or
> we should add the test case and uplift them together?

Please check what you have in, and add the tests later.  Thanks!

Comment 95

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The Aurora branch already happened.

Comment 96

[Ethan Tseng [:ethan]]

Attachment: Fix downgrading issue by restoring appId and inBrowserElement columns v3. r=honzab

Fix nits addresses in comment 93.

Comment 97

[Ethan Tseng [:ethan]]

The patch is complete and manually tested.
Waiting for Treeherder result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=75d693f4f6be

Comment 98

[Ethan Tseng [:ethan]]

(In reply to Ehsan Akhgari (don't ask for review please) [Away Nov 3-19] from comment #94)
> Please check what you have in, and add the tests later.  Thanks!

Follow-up was filed: Bug 1220361 - Add test case for Cookie database migration to new schema versions.

Comment 99

[Ethan Tseng [:ethan]]

Treeherder result looks good (except for some common intermittent test failures).
Request to check in the patch of attachment 8681579 [details] [diff] [review].

Comment 100

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f37d702d949

Comment 101

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8681579 [details] [diff] [review]
Fix downgrading issue by restoring appId and inBrowserElement columns v3. r=honzab

[Triage Comment]
We need that in aurora asap.

Comment 102

[(no longer active)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/168743bc2474

Comment 103

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f37d702d949

Comment 104

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/0f37d702d949

Comment 105

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/168743bc2474