Closed Bug 1314361 Opened 8 years ago Closed 7 years ago

Remove addonId from origin attributes

Categories

(Core :: Security: CAPS, defect)

Product:
Core
Component:
Security: CAPS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: billm, Assigned: kmag)

References

Details

(Whiteboard: [OA])

Attachments

(9 files, 2 obsolete files)

Bug 1314361: Part 1 - Generate nsIPrincipal.addonId from AddonPolicyService rather than origin attributes.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1314361: Part 2 - Stop using addonId origin attribute for permission checks.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1314361: Part 3 - Stop using origin attributes for add-on ID in console filtering.
58 bytes, text/x-review-board-request
baku
: review+
Details
Bug 1314361: Part 4 - Stop setting addonId origin attribute.
58 bytes, text/x-review-board-request
billm
: review+
Details
Bug 1314361: Part 5 - Remove origin attribute comparison helpers for ignoring addonId.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Bug 1314361: Part 7a - Strip addonId from existing DOMStorage origin attributes.
58 bytes, text/x-review-board-request
mayhemer
: review+
Details
Bug 1314361: Part 6 - Remove the addonId origin attribute.
58 bytes, text/x-review-board-request
bholley
: review+
Details
Patch to fix a test failure 1
1.91 KB, patch
Details | Diff | Splinter Review
Patch to fix a test failure 2
13.22 KB, patch
Details | Diff | Splinter Review 
I think we can eliminate the addonId origin attribute as follows:

1. For BasePrincipal::{AddonHasPermission,AddonAllowsLoad}, we can get the add-on ID from the  principal's URL using ExtensionURIToAddonId from the add-on policy service.

2. Do a similar thing for BasePrincipal::GetAddonId I guess.

3. Do a similar thing here:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/toolkit/components/extensions/ExtensionManagement.jsm#243
Or I guess we could just use principal.addonId.

As far as I can tell, that's all we need to change. Then we can remove the origin attribute and everything that's used to generate it.

Kris, do you think you could do this? It seems like it would simplify a lot of things in the platform.
Flags: needinfo?(kmaglione+bmo)
Yeah, it sounds pretty doable.
Assignee: nobody → kmaglione+bmo
Flags: needinfo?(kmaglione+bmo)
It's true that 'mAddonId' behaves differently than the other attributes in OriginAttributes, and I like the idea to fix it. But I need to understand the big picture before reviewing this code.
Currently, mAddonId is the only flag that can be set to a chrome docShell (and this is what I don't like). How does it work with the new setup? When do we set it? When do we remove this new flag?
How is in charge of creating this addonIds and what about the security model behind this new approach?
And: how does this addonId works together with the rest of OriginAttributes?
Flags: needinfo?(wmccloskey)
If you remove it from OriginAttributes it won't be creating separate data jars (for example IndexedDB), is that what you want ?
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review90576

::: dom/quota/ActorsParent.cpp:7010
(Diff revision 1)
>  
>          continue;
>        }
> +
> +      // If the directory name contains an addonId origin attribute, remove it.
> +      nsAutoCString newOrigin;

Are you sure this code actually runs and upgrades existing origin directories ?
(In reply to Andrea Marchesini [:baku] from comment #12)
> Currently, mAddonId is the only flag that can be set to a chrome docShell
> (and this is what I don't like). How does it work with the new setup? When
> do we set it? When do we remove this new flag?

Now it's automatically set on the principal based on the URI and the add-on policy service. It's never added to origin attributes, and it's never inherited. Since it belongs strictly to the principal, it's also never removed.

> How is in charge of creating this addonIds and what about the security model
> behind this new approach?

The security model is the same, and in practice the behavior is exactly the same. The only real difference is that we no longer need a lot of special case code to override the normal inheritance behavior for origin attributes.

When a principal is created with a "moz-extension" URL, the CAPS code looks up the URL in the add-on policy service[1]. If the URL is owned by a currently running add-on, that add-on's ID is returned, and attached to the principal, and is used in later permission lookups for things like cross-origin requests and clipboard access.

[1]: http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/caps/nsIAddonPolicyService.idl#59-62

> And: how does this addonId works together with the rest of OriginAttributes?

It doesn't really have any particular relationship to them (and it never really did). Add-on code running in different user contexts or in private browsing contexts will still be treated as separate origins. The only real difference now is that there's one less origin attribute that never really behaved as an origin attribute to begin with.

There's more detail in bug 1278804 where this conversation started.

(In reply to Jan Varga [:janv] from comment #13)
> If you remove it from OriginAttributes it won't be creating separate data
> jars (for example IndexedDB), is that what you want ?

Yes. This origin attribute was only applied to URLs with specific moz-extension://... origins, so it didn't really add any information. The add-ons still get their own, unique data jars based on URL alone.
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review90576

> Are you sure this code actually runs and upgrades existing origin directories ?

No, it turns out that it only worked in the one circumstance that I tested. It looks like I need to bump the schema version and add another migration function.

I've also run into a new snag, in that if there's a clash where both the old and new origin exist, and I leave the old one behind, storage initialization fails because it can't parse the addonId origin attribute in the old origin. Which in practice should only affect people using the same profile for multiple Firefox versions, but those people definitely exist.
Comment on attachment 8807802 [details]
Bug 1314361: Part 3 - Stop using origin attributes for add-on ID in console filtering.

https://reviewboard.mozilla.org/r/90832/#review90730

::: dom/tests/browser/browser_ConsoleAPI_originAttributes.js:8
(Diff revision 2)
>  
>  const ConsoleAPIStorage = Cc["@mozilla.org/consoleAPI-storage;1"]
>        .getService(Ci.nsIConsoleAPIStorage);
>  
> +// FIXME: This test shouldn't need to rely on low-level internals.
> +const {Service} = Cu.import("resource://gre/modules/ExtensionManagement.jsm", {});

ExtensionService ? or ExtensionManagementService? or ems...
Attachment #8807802 - Flags: review?(amarchesini) → review+
Comment on attachment 8807804 [details]
Bug 1314361: Part 5 - Remove origin attribute comparison helpers for ignoring addonId.

https://reviewboard.mozilla.org/r/90836/#review90898

So this is just a straight backout of that other patch? I'd also be ok with keeping the SubsumesInternal behavior change, but this is fine too, and probably lowest risk.
Attachment #8807804 - Flags: review?(bobbyholley) → review+
Comment on attachment 8807805 [details]
Bug 1314361: Part 6a - Add helper functions for stripping addonId from origin suffix strings.

https://reviewboard.mozilla.org/r/90838/#review90900

Hm, why do we need to do this complicated stripping? Can't we just teach the deserialization logic to ignore addonId when parsing?
Attachment #8807805 - Flags: review?(bobbyholley) → review-
Comment on attachment 8807808 [details]
Bug 1314361: Part 6 - Remove the addonId origin attribute.

https://reviewboard.mozilla.org/r/90844/#review90902
Attachment #8807808 - Flags: review?(bobbyholley) → review+
Comment on attachment 8807804 [details]
Bug 1314361: Part 5 - Remove origin attribute comparison helpers for ignoring addonId.

https://reviewboard.mozilla.org/r/90836/#review90898

Yeah, for the most part. It also removes the `IsOriginAttributesEqualIgnoringAddonId` function, which predates that patch but isn't needed anymore.

I decided to remove the SubsumesInternal change mainly because it removed the need to check Kind() in Subsumes. It might be worth bringing back if we need to add this kind of comparison again, though.
Comment on attachment 8807805 [details]
Bug 1314361: Part 6a - Add helper functions for stripping addonId from origin suffix strings.

https://reviewboard.mozilla.org/r/90838/#review90900

There's existing data in DOMStorage and IndexedDB that's keyed by origin, and includes the addonId origin attribute. Stripping the ID during deserialization would prevent the error when trying to load those, but it wouldn't allow add-ons to access it, so we need to run migrations to remove it from the existing entries.

I suppose that could be done by deserializing and then reserializing the origins, but the code wouldn't be any simpler.
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review90912

So this patch looks much better, I didn't check all the details yet, however you only covered the origin stored in indexeddb databases and origin directory names. There's one more place, it's the metadata file which contains the original origin string (before it's sanitized) and also the suffix and the group. I know this is really painful, but it needs to be done.
Comment on attachment 8807800 [details]
Bug 1314361: Part 1 - Generate nsIPrincipal.addonId from AddonPolicyService rather than origin attributes.

https://reviewboard.mozilla.org/r/90828/#review90916

::: caps/nsPrincipal.cpp:51
(Diff revision 2)
>      mutableObj &&
>      NS_SUCCEEDED(mutableObj->GetMutable(&isMutable)) &&
>      !isMutable;
>  }
>  
> +static nsCOMPtr<nsIAddonPolicyService> gAddonPolicyService;

Please declare this inside GetAddonPolicyService. Otherwise you'll be increasing the number of static constructors in the tree.

::: caps/nsPrincipal.cpp:381
(Diff revision 2)
>  }
>  
>  NS_IMETHODIMP
> +nsPrincipal::GetAddonId(nsAString& aAddonId)
> +{
> +  if (mAddonIdCache.isNothing()) {

Maybe switch this test so you return early if mAddonIdCache.IsSome(). That way you could unindent the main body of the function one level.

::: caps/nsPrincipal.cpp:387
(Diff revision 2)
> +    NS_ENSURE_TRUE(mCodebase, NS_ERROR_FAILURE);
> +
> +    nsresult rv;
> +    bool isMozExt;
> +    if (NS_SUCCEEDED(mCodebase->SchemeIs("moz-extension", &isMozExt)) && isMozExt) {
> +      auto addonPolicyService = GetAddonPolicyService(&rv);

The 'auto' here hides the fact that you don't keep a refcount to the service. That's probably fine since it's owned until shutdown, but I'd like it to be clearer what's happening. So please write this as an nsIAddonPolicyService*.
Attachment #8807800 - Flags: review?(wmccloskey) → review+
Comment on attachment 8807805 [details]
Bug 1314361: Part 6a - Add helper functions for stripping addonId from origin suffix strings.

https://reviewboard.mozilla.org/r/90838/#review90936

Per IRC discussion, I see why we need this now.

However, it seems like we ought PopulateFromSuffix / PopulateFromOrigin directly and then reserialize, rather than duplicating the string munging. With that, It's not clear to me that we really need a helper in caps.
Comment on attachment 8807801 [details]
Bug 1314361: Part 2 - Stop using addonId origin attribute for permission checks.

https://reviewboard.mozilla.org/r/90830/#review90928
Attachment #8807801 - Flags: review?(wmccloskey) → review+
Comment on attachment 8807803 [details]
Bug 1314361: Part 4 - Stop setting addonId origin attribute.

https://reviewboard.mozilla.org/r/90834/#review90978

::: toolkit/components/extensions/Extension.jsm:663
(Diff revision 2)
> -        baseURI, {addonId: addon.id}
> +        baseURI, {});
> -      );
>        Services.qms.clearStoragesForPrincipal(principal);
>  
>        // Clear localStorage created by the extension
> -      let attrs = JSON.stringify({addonId: addon.id});
> +      Services.domStorageManager.getStorage(null, principal).clear();

It's kind of a shame we're going to lose some of the functionality here. It looks like this used to clear a variety of other things. Oh well.
Attachment #8807803 - Flags: review?(wmccloskey) → review+
I think Kris answered all the questions here.
Flags: needinfo?(wmccloskey)
Comment on attachment 8807806 [details]
Bug 1314361: Part 7a - Strip addonId from existing DOMStorage origin attributes.

https://reviewboard.mozilla.org/r/90840/#review91376
Attachment #8807806 - Flags: review?(honzab.moz) → review+
Attachment #8807805 - Attachment is obsolete: true
Kris, did you intend to strip out the add-on ID bit from all persisted OriginAttributes?

I can think of at least a couple of missing cases: the permission manager and the cookie service.  There are probably other places too, but I can't think of a great way of enumerating all of them.  Maybe look for all consumers of PopulateFromSuffix?
Flags: needinfo?(kmaglione+bmo)
(In reply to :Ehsan Akhgari from comment #48)
> Kris, did you intend to strip out the add-on ID bit from all persisted
> OriginAttributes?
> 
> I can think of at least a couple of missing cases: the permission manager
> and the cookie service.  There are probably other places too, but I can't
> think of a great way of enumerating all of them.  Maybe look for all
> consumers of PopulateFromSuffix?

The only places we're actually expecting the add-on ID to be persisted are in DOM and IndexedDB storage. We never add it to HTTP origins, so it shouldn't be anywhere in the cookie store. In case it happens to be persisted anywhere else, PopulateFromSuffix silently drops it when it parses the origin, so it shouldn't cause any harm.
Flags: needinfo?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #49)
> (In reply to :Ehsan Akhgari from comment #48)
> > Kris, did you intend to strip out the add-on ID bit from all persisted
> > OriginAttributes?
> > 
> > I can think of at least a couple of missing cases: the permission manager
> > and the cookie service.  There are probably other places too, but I can't
> > think of a great way of enumerating all of them.  Maybe look for all
> > consumers of PopulateFromSuffix?
> 
> The only places we're actually expecting the add-on ID to be persisted are
> in DOM and IndexedDB storage. We never add it to HTTP origins, so it
> shouldn't be anywhere in the cookie store. In case it happens to be
> persisted anywhere else, PopulateFromSuffix silently drops it when it parses
> the origin, so it shouldn't cause any harm.

Thanks for the explanation.  Sounds great!
I hate to nag, but it looks like this origin attribute is causing IndexedDB failures for some Windows users due to path length issues, so I'd like to get it landed as soon as possible.
Flags: needinfo?(jvarga)
See Also: → 1320404
We are using index database storage for storing persistent data via web extension. These index database files are stored under a profile folder. For windows, profile folder path looks similar to -

C:\Users{userName}\AppData\Roaming\Mozilla\Firefox\Profiles{profileName}\storage\default\moz-extension+++32685a26-a0e5-4a5a-8151-71b89e65ddca^addonId=fd29864804c222152361d9a46fd6c98b7d88525e%40temporary-addon\idb{fileName}

This part has some variable components such as userName, profileName, fileName which would be different for different users & different machines. Windows has a restriction on max path length. We observed that storing into index database fails when this path length becomes greater that 238. The typical error that comes up while saving a file is : 
"The operation failed for reasons unrelated to the database itself and not covered by any other error code."

Our Web extension uses index database storage heavily and this bug will completely break the extension for some of the users. This bug exists on latest FF 50 and was present on earlier versions as well. Do you have work arounds for this issue and when can we expect a fix for this ?
I have few explicit questions 
1. Is this issue specific to windows ? Or it can happen on linux & mac as well ? 
2. We are already using latest FF 50. Can we ensure that next released version of firefox has this fix ?
3 .Till the issue is fixed, is there any work around that we can suggest ?
(In reply to Nipun jain from comment #55)
> 1. Is this issue specific to windows ? Or it can happen on linux & mac as
> well ?

On Linux, PATH_MAX is typically 4096. On OS-X, it's 1024. So while it's technically possible to run into this there, it's extremely unlikely.

> 2. We are already using latest FF 50. Can we ensure that next released
> version of firefox has this fix ?

The data migrations required for this change aren't without risk. We may be able to get this uplifted to beta before the next major release, but I definitely can't make any promises.

> 3. Till the issue is fixed, is there any work around that we can suggest ?

Probably not.
(In reply to Kris Maglione [:kmag] from comment #53)
> I hate to nag, but it looks like this origin attribute is causing IndexedDB
> failures for some Windows users due to path length issues, so I'd like to
> get it landed as soon as possible.

Sorry for the delay. I went through your latest patch and it seems we are almost there.
This is a sensitive change and we should be really careful about it.
I think I found a flaw in our upgrade loop in QM, it's not your fault and things work correctly if we don't add a new UpgradeStorageFromXToY method. We need to fix that first.
The problem is that very old FF profiles that don't have storage.sqlite database wouldn't be upgraded with your new UpgradeStorageFrom1_0To1_1() method. I need to refactor stuff around the upgrade loop a bit.

Also I'm not sure if this should be just a minor version change. If we bump just the minor version, then we will allow older FF to run with upgraded profile, it's not a huge problem, but older FF might create all these origin directories with addonId again.

If we decide to bump the major version, we should land this along with bug 1311057 which is going to bump the major version too.

I'm going to ask Andrew Sutherland who reviewed my patch for the upgrade loop in QM, if this should be a major or minor version change:
Andrew, the patch basically deserialize and re-serialize origin attributes which drops obsolete ones (and in theory adds new ones). The patch handles all 3 places where we store serialized origin (origin string + origin suffix), origin directory names, metadata files, IDB databases.
If we bump just the minor version then for example aurora can run with the new profile and create origin directories, etc. with obsolete origin attributes.
Flags: needinfo?(jvarga) → needinfo?(bugmail)
I think you make the case for a major version bump quite well!  So, yes, major version bump.

It sounds like the minor change would only safely work if the schema version was (partially) downgraded when storageVersion > kStorageVersion.  I'd proposed something like that in bug 1195930 comment 86, but I think it was the right call to go with the current solution.  Informed by this case, it may make sense to think about such a used-by-older-version indication in the schema version.
Flags: needinfo?(bugmail)
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review96404

::: dom/indexedDB/ActorsParent.cpp:99
(Diff revision 4)
>  #include "nsRefPtrHashtable.h"
>  #include "nsStreamUtils.h"
>  #include "nsString.h"
>  #include "nsThreadPool.h"
>  #include "nsThreadUtils.h"
> +#include "nsVariant.h"

maybe no need for this

::: dom/indexedDB/ActorsParent.cpp:167
(Diff revision 4)
>  // schema version.
>  static_assert(JS_STRUCTURED_CLONE_VERSION == 8,
>                "Need to update the major schema version.");
>  
>  // Major schema version. Bump for almost everything.
> -const uint32_t kMajorSchemaVersion = 25;
> +const uint32_t kMajorSchemaVersion = 26;

So here you are bumping the major version, so already upgraded databases won't work with previous FF versions at all. But the QM upgrade is bumping just the minor version...

::: dom/indexedDB/ActorsParent.cpp:4143
(Diff revision 4)
>  
>    return NS_OK;
>  }
>  
> +class StripAddonIdFunction final
> +  : public mozIStorageFunction

I know that the end result is stripping of addonId, but this function doesn't check addonId explicitely and your comment below says "... any obsolete origin attributes", so I think the name of the method should reflect that. StripObsoleteOriginAttributesFunction ?

::: dom/indexedDB/ActorsParent.cpp:4155
(Diff revision 4)
> +  { }
> +
> +  NS_IMETHOD
> +  OnFunctionCall(mozIStorageValueArray* aArguments,
> +                 nsIVariant** aResult) override
> +  {

This could do some checks first like CompressDataBlobsFunction::OnFunctionCall(), the asserts, profiler label and verification of aArguments.

::: dom/indexedDB/ActorsParent.cpp:4156
(Diff revision 4)
> +
> +  NS_IMETHOD
> +  OnFunctionCall(mozIStorageValueArray* aArguments,
> +                 nsIVariant** aResult) override
> +  {
> +    nsAutoCString origin;

no need for nsAutoCString here, auto strings are only useful when you do multiple e.g. Append()

::: dom/indexedDB/ActorsParent.cpp:4167
(Diff revision 4)
> +    // Deserialize and re-serialize to automatically drop any obsolete origin
> +    // attributes.
> +    GenericOriginAttributes oa;
> +    nsAutoCString newOrigin;
> +    bool ok = oa.PopulateFromOrigin(origin, newOrigin);
> +

Nit: no need for an empty line here

::: dom/indexedDB/ActorsParent.cpp:4172
(Diff revision 4)
> +
> +    if (NS_WARN_IF(!ok)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    nsAutoCString newSuffix;

nsCString

::: dom/indexedDB/ActorsParent.cpp:4177
(Diff revision 4)
> +    nsAutoCString newSuffix;
> +    oa.CreateSuffix(newSuffix);
> +    newOrigin.Append(newSuffix);
> +
> +    nsCOMPtr<nsIWritableVariant> outVar = new nsVariant();
> +    rv = outVar->SetAsAUTF8String(newOrigin);

why not just |nsCOMPtr<nsIVariant> result = new mozilla::storage::TextVariant(newOrigin)| ?

::: dom/indexedDB/ActorsParent.cpp:4188
(Diff revision 4)
> +    return NS_OK;
> +  }
> +};
> +
> +NS_IMPL_ISUPPORTS(StripAddonIdFunction,
> +                  mozIStorageFunction);

This should go after |NS_IMPL_ISUPPORTS(EncodeKeysFunction, mozIStorageFunction)|
And it fits in one line.

::: dom/indexedDB/ActorsParent.cpp:4200
(Diff revision 4)
> +
> +  PROFILER_LABEL("IndexedDB",
> +                 "UpgradeSchemaFrom25_0To26_0",
> +                 js::ProfileEntry::Category::STORAGE);
> +
> +  NS_NAMED_LITERAL_CSTRING(functionName, "strip_addon_id");

again, a generic name would be better I think

::: dom/indexedDB/test/unit/test_addonIdStorageUpgrade.js:1
(Diff revision 4)
> +/**

test_obsoleteOriginAttributesUpgrade.js ?
and then obsoleteOriginAttributes_profile.zip ?

::: dom/indexedDB/test/unit/test_addonIdStorageUpgrade.js:8
(Diff revision 4)
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +var testGenerator = testSteps();
> +
> +function* testSteps()

ok, so function* requires the changes in helpers.js and the main benefit is that we don't have to call "yield undefined", just "yield" ?

::: dom/indexedDB/test/unit/test_addonIdStorageUpgrade.js:13
(Diff revision 4)
> +function* testSteps()
> +{
> +  const openParams = [
> +    { url: "moz-extension://8ea6d31b-917c-431f-a204-15b95e904d4f",
> +      dbName: "Hello.", dbVersion: 1 },
> +  ];

There's just one entry, so it doesn't have to be an array. See test_oldDirectories.js
However you might want to add more entries for storage/permanent and storage/temporary.

::: dom/indexedDB/test/unit/test_addonIdStorageUpgrade.js:33
(Diff revision 4)
> +  clearAllDatabases(continueToNextStepSync);
> +  yield;
> +
> +  installPackagedProfile("addonIdUpgrade_profile");
> +
> +  for (let params of openParams) {

maybe no need for the loop

::: dom/indexedDB/test/unit/test_addonIdStorageUpgrade.js:46
(Diff revision 4)
> +  }
> +
> +  resetAllDatabases(continueToNextStepSync);
> +  yield;
> +
> +  for (let params of openParams) {

maybe no need for the loop

::: dom/indexedDB/test/unit/test_addonIdStorageUpgrade.js:55
(Diff revision 4)
> +    request.onsuccess = grabEventAndContinueHandler;
> +    let event = yield;
> +
> +    is(event.type, "success", "Correct event type");
> +  }
> +

Nit: if you want to be 100% sure about the upgrade, you can add a check here for existence of the old origin directory name. Something like in test_oldDirectories.js

::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js:44
(Diff revision 4)
>  
>  function run_test() {
>    runTest();
>  };
>  
> +function send(iterator, value) {

I think this should go before grabEventAndContinueHandler

::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js:45
(Diff revision 4)
>  function run_test() {
>    runTest();
>  };
>  
> +function send(iterator, value) {
> +  if (iterator.send)

We usually add curly braces even for one line branches. ComparyKeys() in this file is somehow an exception, I don't think we should follow that method.

::: dom/indexedDB/test/unit/xpcshell-head-parent-process.js:83
(Diff revision 4)
>    }
>  
>    SpecialPowers.removeFiles();
>  
>    do_execute_soon(function(){
> +    if (testGenerator.close)

curly braces

::: dom/quota/ActorsParent.cpp:129
(Diff revision 4)
>  
>  // Major storage version. Bump for backwards-incompatible changes.
>  const uint32_t kMajorStorageVersion = 1;
>  
>  // Minor storage version. Bump for backwards-compatible changes.
> -const uint32_t kMinorStorageVersion = 0;
> +const uint32_t kMinorStorageVersion = 1;

I'm leaning towards a major version change, but we will see what asuth says.

::: dom/quota/ActorsParent.cpp
(Diff revision 4)
>  {
>    MOZ_ASSERT(!NS_IsMainThread());
>    MOZ_ASSERT(aDirectory);
>    MOZ_ASSERT(aTimestamp);
>    MOZ_ASSERT(aIsApp);
> -  MOZ_ASSERT(mStorageInitialized);

This is quite importand assertion. It's there to prevent touching possibly not yet upgraded metadata files as they were new (or already upgraded).

::: dom/quota/ActorsParent.cpp:4159
(Diff revision 4)
>      return rv;
>    }
>  
>    return NS_OK;
>  }
>  #endif

You should be replacing this |#if 0| method.

::: dom/quota/ActorsParent.cpp:4162
(Diff revision 4)
>    return NS_OK;
>  }
>  #endif
>  
>  nsresult
> +QuotaManager::MaybeStripAddonIdAndRename(nsIFile* aDir)

consider to change the name of this method

::: dom/quota/ActorsParent.cpp:4164
(Diff revision 4)
>  #endif
>  
>  nsresult
> +QuotaManager::MaybeStripAddonIdAndRename(nsIFile* aDir)
> +{
> +  nsAutoString leafName;

nsString

::: dom/quota/ActorsParent.cpp:4165
(Diff revision 4)
>  
>  nsresult
> +QuotaManager::MaybeStripAddonIdAndRename(nsIFile* aDir)
> +{
> +  nsAutoString leafName;
> +  MOZ_ALWAYS_SUCCEEDS(aDir->GetLeafName(leafName));

I would prefer to switch all GetLeafName() calls to MOZ_ALWAYS_SUCCEEDS() at once, so please just check the result here as usual.

::: dom/quota/ActorsParent.cpp:4174
(Diff revision 4)
> +  // Deserialize and re-serialize to automatically drop any obsolete origin
> +  // attributes.
> +  GenericOriginAttributes oa;
> +  nsAutoCString newOrigin;
> +  bool ok = oa.PopulateFromOrigin(origin, newOrigin);
> +

no need for an empty line here

::: dom/quota/ActorsParent.cpp:4179
(Diff revision 4)
> +
> +  if (NS_WARN_IF(!ok)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  nsAutoCString newSuffix;

nsCString

::: dom/quota/ActorsParent.cpp:4216
(Diff revision 4)
> +  }
> +
> +  QM_WARNING("Renaming old add-on storage directory %s to %s",
> +             NS_ConvertUTF16toUTF8(leafName).get(), newOrigin.get());
> +
> +  rv = aDir->MoveTo(nullptr, newLeafName);

I think RenameTo() is more suitable here.

::: dom/quota/ActorsParent.cpp:4228
(Diff revision 4)
> +  nsAutoCString currentOrigin;
> +  nsAutoCString currentSuffix;
> +  nsAutoCString group;
> +  bool isApp;
> +  rv = GetDirectoryMetadata2(aDir, &timestamp, currentSuffix, group,
> +                             currentOrigin, &isApp);

GetDirectoryMetadata2() can fail here if we modify the structure of the file in future. The upgrade method/helper should fork the GetDirectoryMetadata2() method like other helpers do.
So I think you should create a new helper for this upgrade, inheriting from StorageDirectoryHelper. Take a look at UpgradeDirectoryMetadataFrom1To2Helper.

::: dom/quota/ActorsParent.cpp:4229
(Diff revision 4)
> +  nsAutoCString currentSuffix;
> +  nsAutoCString group;
> +  bool isApp;
> +  rv = GetDirectoryMetadata2(aDir, &timestamp, currentSuffix, group,
> +                             currentOrigin, &isApp);
> +  if (NS_WARN_IF(NS_FAILED(rv))) {

So this can fail if the metadata file doesn't exist, right ?
A missing metadata file is not a fatal error, we have code to restore it. I think we don't have to restore it here, but we need to bail out if the file doesn't exist.

::: dom/quota/ActorsParent.cpp:4234
(Diff revision 4)
> +  if (NS_WARN_IF(NS_FAILED(rv))) {
> +    return rv;
> +  }
> +
> +  // Update the origin suffix in the group.
> +  auto index = group.FindChar('^');

I believe you don't need this FindChar hackery once you implement the upgrade helper.

::: dom/quota/ActorsParent.cpp:4253
(Diff revision 4)
> +{
> +  AssertIsOnIOThread();
> +  MOZ_ASSERT(aConnection);
> +
> +  nsresult rv;
> +  for (const PersistenceType persistenceType : kAllPersistenceTypes) {

So you are going through all persistence types here. I assume an extension could store stuff in all these storages. Ok, but then I thik you should add databases to the packaged profile to actually test it.

::: dom/quota/ActorsParent.cpp:4266
(Diff revision 4)
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return rv;
> +    }
> +
> +    bool exists;
> +    if (NS_FAILED(directory->Exists(&exists)) || !exists) {

Either:
rv = directory->Exists(&exists);
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
if (!exists) {
  continue;
}

or:
MOZ_ALWAYS_SUCCEEDS(directory->Exists(&exists));
if (!exists) {
  continue;
}

but given the style you use for GetDirectoryEntries, the former better fits here

::: dom/quota/ActorsParent.cpp:4288
(Diff revision 4)
> +
> +      nsCOMPtr<nsIFile> originDir = do_QueryInterface(entry);
> +      MOZ_ASSERT(originDir);
> +
> +      bool isDirectory;
> +      if (NS_SUCCEEDED(originDir->IsDirectory(&isDirectory)) && isDirectory) {

Again, this should bail out if IsDirectory fails.
Also, it's an error if find an unknown file in the directory (except DSSTORE_FILE_NAME).
See UpgradeDirectoryMetadataFrom1To2Helper::UpgradeMetadataFiles how it's done.

::: dom/quota/ActorsParent.cpp:4424
(Diff revision 4)
>  
>        while (storageVersion != kStorageVersion) {
>          /* if (storageVersion == MakeStorageVersion(1, 0)) {
>            rv = UpgradeStorageFrom1To2(connection);
> -        } else */ {
> +        } else */
> +        if (storageVersion == MakeStorageVersion(1, 0)) {

This should be replacing the /* ... */ code above.

::: dom/quota/ActorsParent.cpp:6826
(Diff revision 4)
>        (isAbout = aToken.EqualsLiteral("about") ||
>                   aToken.EqualsLiteral("moz-safe-about")) ||
>        aToken.EqualsLiteral("indexeddb") ||
>        (isFile = aToken.EqualsLiteral("file")) ||
>        aToken.EqualsLiteral("app") ||
> +      aToken.EqualsLiteral("moz-extension") ||

Ok, so this is a separate bug that you are fixing here ? It's fine, just curious.

::: dom/quota/QuotaManager.h:460
(Diff revision 4)
>    nsresult
>    UpgradeStorageFrom1To2(mozIStorageConnection* aConnection);
>  #endif
>  
>    nsresult
> +  UpgradeStorageFrom1_0To1_1(mozIStorageConnection* aConnection);

This should be replacing the |#if 0| method which was added there as an example for first real upgrade method.

::: dom/quota/QuotaManager.h:463
(Diff revision 4)
>  
>    nsresult
> +  UpgradeStorageFrom1_0To1_1(mozIStorageConnection* aConnection);
> +
> +  nsresult
> +  MaybeStripAddonIdAndRename(nsIFile* aDir);

Consider to changed the method name.
Attachment #8807807 - Flags: review?(jvarga)
(In reply to Jan Varga [:janv] from comment #59)
> ::: dom/quota/ActorsParent.cpp:4229
> (Diff revision 4)
> > +  nsAutoCString currentSuffix;
> > +  nsAutoCString group;
> > +  bool isApp;
> > +  rv = GetDirectoryMetadata2(aDir, &timestamp, currentSuffix, group,
> > +                             currentOrigin, &isApp);
> > +  if (NS_WARN_IF(NS_FAILED(rv))) {
> 
> So this can fail if the metadata file doesn't exist, right ?
> A missing metadata file is not a fatal error, we have code to restore it. I
> think we don't have to restore it here, but we need to bail out if the file
> doesn't exist.
> 

Actually, if you implement the heleper, you can use mNeedsRestore to restore the metadata file.
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review96404

> I know that the end result is stripping of addonId, but this function doesn't check addonId explicitely and your comment below says "... any obsolete origin attributes", so I think the name of the method should reflect that. StripObsoleteOriginAttributesFunction ?

Yeah, the original version explicitly stripped the add-on ID, but I didn't rename this function after that changed.

> ok, so function* requires the changes in helpers.js and the main benefit is that we don't have to call "yield undefined", just "yield" ?

Well, the main benefit is that old-style generator functions are deprecated and being removed. I mainly made this change because they prevented me from running ESLint on this code.

> This is quite importand assertion. It's there to prevent touching possibly not yet upgraded metadata files as they were new (or already upgraded).

The only alternative I can think of is to set mStorageInitialized before calling the function. One way or another, we need to read the old version of the metadata during the migration.

But I guess if we fork it, that's not an issue.

> I think RenameTo() is more suitable here.

I initially used RenameTo, but it doesn't update the leaf name of the file instance after the rename.

> Ok, so this is a separate bug that you are fixing here ? It's fine, just curious.

It's probably a separate bug, yes. I mainly fixed it here because it was causing warnings during the migration tests.
(In reply to Jan Varga [:janv] from comment #57)
> Also I'm not sure if this should be just a minor version change. If we bump
> just the minor version, then we will allow older FF to run with upgraded
> profile, it's not a huge problem, but older FF might create all these origin
> directories with addonId again.
>
> If we decide to bump the major version, we should land this along with bug
> 1311057 which is going to bump the major version too.
>
> I'm going to ask Andrew Sutherland who reviewed my patch for the upgrade
> loop in QM, if this should be a major or minor version change:
> Andrew, the patch basically deserialize and re-serialize origin attributes
> which drops obsolete ones (and in theory adds new ones). The patch handles
> all 3 places where we store serialized origin (origin string + origin
> suffix), origin directory names, metadata files, IDB databases.
> If we bump just the minor version then for example aurora can run with the
> new profile and create origin directories, etc. with obsolete origin
> attributes.

(In reply to Andrew Sutherland [:asuth] from comment #58)
> I think you make the case for a major version bump quite well!  So, yes,
> major version bump.
>
> It sounds like the minor change would only safely work if the schema version
> was (partially) downgraded when storageVersion > kStorageVersion.  I'd
> proposed something like that in bug 1195930 comment 86, but I think it was
> the right call to go with the current solution.  Informed by this case, it
> may make sense to think about such a used-by-older-version indication in the
> schema version.

I'm on the fence. The post-migration profile can safely be used in an older
version of Firefox, but any switching between versions won't share the same
extension IndexedDB data. That's definitely an annoyance, but I'm not sure
it's enough of an annoyance to justify a major schema version bump.
(In reply to Jan Varga [:janv] from comment #57)
> I think I found a flaw in our upgrade loop in QM, it's not your fault and
> things work correctly if we don't add a new UpgradeStorageFromXToY method.
> We need to fix that first.
> The problem is that very old FF profiles that don't have storage.sqlite
> database wouldn't be upgraded with your new UpgradeStorageFrom1_0To1_1()
> method. I need to refactor stuff around the upgrade loop a bit.

A fix for this just landed on inbound, bug 1329689.
(In reply to Kris Maglione [:kmag] from comment #62)
> I'm on the fence. The post-migration profile can safely be used in an older
> version of Firefox, but any switching between versions won't share the same
> extension IndexedDB data. That's definitely an annoyance, but I'm not sure
> it's enough of an annoyance to justify a major schema version bump.

We have to do a major version bump in bug 1311057 anyway.
Doing a major version bump in this bug is not a thing that we *must* do, but if we have the option to do it along with bug 1311057, it will be just cleaner.
Yeah, any switching between versions won't share the same extension IndexedDB data, but data that was re-created in an older version, won't be deleted in any new version again.
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

If you have time, please try to address my comments, so we can land this along with bug 1311057. Thanks
Thanks. I'll try to get this in along with that and bug 1320404.
(In reply to Kris Maglione [:kmag] from comment #66)
> Thanks. I'll try to get this in along with that and bug 1320404.

Oh, bug 1320404 will also affect quota manager, oh dear :)
Whiteboard: [OA]
Tanvi asked how to set the security-review flag on IRC backlog. Flagging it for her.
Flags: sec-review?(tanvi)
Thanks Freddy!  This should go through a security review before landing.  I'm also wondering if this effects the SafeBrowsing implementation.
I'm not sure why this would need a security review. The security characteristics are not substantially different from what they were before, except perhaps for the fact that we remove the risk of unprivileged content accidentally inheriting an addonId origin attribute. Given that bholley, smaug, billm, and baku have all been involved in this and haven't raised concerns, I don't think more review is needed.

I also can't see how this would affect SafeBrowsing. This origin attribute only ever applied to moz-extension: URLs, which shouldn't have any interaction with SafeBrowsing.
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review109586

Some initial comments.

::: dom/indexedDB/ActorsParent.cpp:4154
(Diff revision 5)
> +    MOZ_ASSERT(aResult);
> +
> +    PROFILER_LABEL("IndexedDB",
> +                   "StripObsoleteOriginAttributesFunction::OnFunctionCall",
> +                   js::ProfileEntry::Category::STORAGE);
> +

This could also include:

    uint32_t argc;
    nsresult rv = aArguments->GetNumEntries(&argc);
    if (NS_WARN_IF(NS_FAILED(rv))) {
      return rv;
    }

    if (argc != 1) {
      NS_WARNING("Don't call me with the wrong number of arguments!");
      return NS_ERROR_UNEXPECTED;
    }

    int32_t type;
    rv = aArguments->GetTypeOfIndex(0, &type);
    if (NS_WARN_IF(NS_FAILED(rv))) {
      return rv;
    }

    if (type != mozIStorageStatement::VALUE_TYPE_TEXT) {
      NS_WARNING("Don't call me with the wrong type of arguments!");
      return NS_ERROR_UNEXPECTED;
    }

::: dom/indexedDB/ActorsParent.cpp:4170
(Diff revision 5)
> +    bool ok = oa.PopulateFromOrigin(origin, newOrigin);
> +    if (NS_WARN_IF(!ok)) {
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    nsAutoCString newSuffix;

Nit: just nsCString

::: dom/indexedDB/ActorsParent.cpp:4175
(Diff revision 5)
> +    nsAutoCString newSuffix;
> +    oa.CreateSuffix(newSuffix);
> +    newOrigin.Append(newSuffix);
> +
> +    nsCOMPtr<nsIVariant> result = new mozilla::storage::TextVariant(
> +      NS_ConvertUTF8toUTF16(newOrigin));

Sorry that I didn't notice earlier, there's also UTF8TextVariant, so you don't need to do the conversion here.

::: dom/indexedDB/test/unit/test_obsoleteOriginAttributesUpgrade.js:69
(Diff revision 5)
> +    file.leafName = NEW_NAME;
> +    ok(file.exists(), `New ${subdir} storage directory should exist`);
> +  }
> +
> +  finishTest();
> +  yield;

I think this final yield isn't needed anymore. See bug 1321218
Attachment #8807807 - Flags: review?(jvarga)
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review109600
Attachment #8807807 - Flags: review?(jvarga)
Hi Kris,

Looks like I was wrong about addonId's connection with Safe Browsing.  Safe Browsing was actually using an appId and now is using the firstPartyDomain Origin Attribute (https://bugzilla.mozilla.org/show_bug.cgi?id=1320402).

I'd still like a quick chat between you and Dan to ensure that we aren't breaking isolation in any way by removing addonId as an Origin Attribute.

Dan, passing the secreview to you.  Please reach out to Kris.

Thanks!
Flags: sec-review?(tanvi) → sec-review?(dveditz)
Comment on attachment 8807807 [details]
Bug 1314361: Part 7b - Strip addonId from existing quota storage origins.

https://reviewboard.mozilla.org/r/90842/#review110958

::: dom/indexedDB/ActorsParent.cpp:4172
(Diff revision 5)
> +      return NS_ERROR_FAILURE;
> +    }
> +
> +    nsAutoCString newSuffix;
> +    oa.CreateSuffix(newSuffix);
> +    newOrigin.Append(newSuffix);

We can do |new UTF8TextVariant(newOrigin + newSuffix)|, so this append is not needed.

::: dom/indexedDB/ActorsParent.cpp:4194
(Diff revision 5)
> +                 "UpgradeSchemaFrom25_0To26_0",
> +                 js::ProfileEntry::Category::STORAGE);
> +
> +  NS_NAMED_LITERAL_CSTRING(functionName, "strip_obsolete_attributes");
> +
> +  nsCOMPtr<mozIStorageFunction> stripObsoleteAttributes = new StripObsoleteOriginAttributesFunction();

This overflows 80 chars

::: dom/indexedDB/ActorsParent.cpp:4196
(Diff revision 5)
> +
> +  NS_NAMED_LITERAL_CSTRING(functionName, "strip_obsolete_attributes");
> +
> +  nsCOMPtr<mozIStorageFunction> stripObsoleteAttributes = new StripObsoleteOriginAttributesFunction();
> +
> +  nsresult rv = aConnection->CreateFunction(functionName, 1, stripObsoleteAttributes);

ditto

::: dom/indexedDB/ActorsParent.cpp:4206
(Diff revision 5)
> +  rv = aConnection->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +    "UPDATE DATABASE "
> +    "  SET origin = strip_obsolete_attributes(origin) "
> +    "  WHERE origin LIKE '%^%' "
> +    ";"
> +  ));

rv is not checked here.

::: dom/quota/ActorsParent.cpp:7771
(Diff revision 5)
> +
> +  nsCOMPtr<nsIBinaryInputStream> binaryStream;
> +  nsresult rv = GetBinaryInputStream(aDirectory,
> +                                     NS_LITERAL_STRING(METADATA_V2_FILE_NAME),
> +                                     getter_AddRefs(binaryStream));
> +  NS_ENSURE_SUCCESS(rv, rv);

All these NS_ENSURE_SUCCESS should be converted to
if (NS_WARN_IF(NS_FAILED(rv))) {
  return rv;
}
Sorry for delays ...
I submitted some new review comments, but I'm merging your patch 7b with my patch 4 from bug 1311057, so you don't have to address them, I'll do it myself to speed it up.
The primary reason for the merge is that we won't need to traverse directories twice, there will be only one major version change.
Blocks: 1331176
Comment on attachment 8841308 [details] [diff] [review]
Patch to fix a test failure 1

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

Hm. It's unfortunate that we need another special case like this, but I've fixed the rebase to remove this conflict.

I think that this logic is going to need to be updated to handle first-party-domain checks properly, though, similarly to what we did for add-on IDs and do for opener restrictions:

http://searchfox.org/mozilla-central/rev/90d1cbb4fd3dc249cdc11fe5c3e0394d22d9c680/dom/base/PostMessageEvent.cpp#108-119

Otherwise postMessage will probably fail in surprising ways and abort in non-release builds. But that's completely separate from this bug.
Attachment #8807807 - Attachment is obsolete: true
Attachment #8807807 - Flags: review?(jvarga)
I pushed rebased versions of all of the patches apart from 7b. Please feel free to land them along with your merged migration patch.
Blocks: 1339081
Pushed by jvarga@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f72d29f41cd9
Part 1: Generate nsIPrincipal.addonId from AddonPolicyService rather than origin attributes. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec4c621bb3fe
Part 2: Stop using addonId origin attribute for permission checks. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/d07ca792049f
Part 3: Stop using origin attributes for add-on ID in console filtering. r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/73265f9e2940
Part 4: Stop setting addonId origin attribute. r=billm
https://hg.mozilla.org/integration/mozilla-inbound/rev/fffb91ea88ab
Part 5: Remove origin attribute comparison helpers for ignoring addonId. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/ebabe5ba82f1
Part 6: Remove the addonId origin attribute. r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/99ca8cce005e
Part 7: Strip addonId from existing DOMStorage origin attributes. r=mayhemer
Depends on: 1345961
Flags: sec-review?(dveditz)
Not sure if this needs dev-doc updates; adding ddn just in case.
Keywords: dev-doc-needed
This is very much an implementation detail. It doesn't need documentation.
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: