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.

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.

(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.

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?

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.

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.

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. :)

[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?

Go ahead :)

(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.

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.

*** 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).

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.

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

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 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.

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 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 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.

(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.

(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.

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.

(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. :)

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

Minor update.

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.

Polish the patch.

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 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 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 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.

(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.

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

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?

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 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?

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

(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.

=== 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

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

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

=== 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 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?

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?

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

(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 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 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.

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.

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.

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

(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.

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.

Remove debug messages.

(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 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.

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

(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 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.

(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.

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

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

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

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

(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?

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?

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

(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!

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

(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.

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

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

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

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 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.

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

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

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.

(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.

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 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?

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 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.

(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.

(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.

[Tracking Requested - why for this release]: Unblocking for 2.5, as its for N Sec which is not in scope for 2.5.

(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.

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. :)

(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!

(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 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

(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!

The Aurora branch already happened.

Fix nits addresses in comment 93.

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

(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.

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

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.