1273229 - Reloading in about:debugging an add-on with no id causes a new add-on to be created

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Andy McKay]

STR:
1. create an add-on with no add-on id in the manifest
2. load it into about:debugging
3. click reload

Each time the add-on is reloaded I get a new entry in about:debugging, a new icon in the toolbar etc.

Comment 1

[Kumar McMillan [:kumar]]

This behavior might change after the patch from bug 1267870 . Can you try it against the latest fx-team with that patch?

Comment 2

[Andrew Swan [:aswan]]

Oof, the problem is that the id is the key we use to match a new addon being installed with an existing one to decide to treat this as an upgrade (which is what reload now does).  If the id is ephemeral since its an unsigned temporary addon, then we don't find a match so the new version is just installed alongside the old version instead of replacing it.

The path for temporary addons is stored in _sourceBundle, a brute force solution would be for installTemporaryAddon() to search for a matching path instead of (or in addition to) a matching id.  My initial instinct is that this doesn't feel quite right but I can't think of anything more appropriate.  Dave, any thoughts?

Comment 3

[Kumar McMillan [:kumar]]

Maybe we could store a map of add-on source paths (e.g. /Users/kumar/dev/my-addon) to their auto-generated IDs in memory. Or maybe we could switch to generating an ID *based* on the source path since that should not change during the course of a temporary install session.

Comment 4

[Andrew Swan [:aswan]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #3)
> Maybe we could store a map of add-on source paths (e.g.
> /Users/kumar/dev/my-addon) to their auto-generated IDs in memory.

But we already have the needed information in the XPIDatabase, I feel better about searching there than about creating a new data structure and making sure we keep it updated in all the right places.

> Or maybe
> we could switch to generating an ID *based* on the source path since that
> should not change during the course of a temporary install session.

That sounds intriguing but we're constrained by the limitations on IDs:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#275

Comment 5

[Andy McKay]

(In reply to Andrew Swan [:aswan] from comment #4)
> That sounds intriguing but we're constrained by the limitations on IDs:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#275

hash_of_the_path(....)@temporary.addon.com

Comment 6

[Dave Townsend [:mossop]]

(In reply to Andy McKay [:andym] from comment #5)
> (In reply to Andrew Swan [:aswan] from comment #4)
> > That sounds intriguing but we're constrained by the limitations on IDs:
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > internal/XPIProvider.jsm#275
> 
> hash_of_the_path(....)@temporary.addon.com

^^ this, or IIRC an sha1 gives you exactly the right number of characters to form a UUID.

Comment 7

[Kumar McMillan [:kumar]]

Attachment: Bug 1273229 - auto-generate IDs for temp installs from dir path.

Review commit: https://reviewboard.mozilla.org/r/57694/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/57694/

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

https://reviewboard.mozilla.org/r/57694/#review54574

<p>I like this. Just a few minor issues to address.</p>

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1325
(Diff revision 1)
> +    const hasher = Cc["@mozilla.org/security/hash;1"]
> +      .createInstance(Ci.nsICryptoHash);
> +    hasher.init(hasher.SHA1);
> +    // Split the directory path into an array of Unicode characters.
> +    const data = dirPath.split("");
> +    hasher.update(data, data.length);

This needs to be an array of character codes. I'd go with `new TextEncoder().encode(dirPath)`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1332
(Diff revision 1)
> +
> +    // return the two-digit hexadecimal code for a byte
> +    const toHexString = charCode => ("0" + charCode.toString(16)).slice(-2);
> +    // convert the binary hash data to a hex string.
> +    let hash = Array.from(binary, c => toHexString(c.charCodeAt(0)));
> +    return hash.join("").toLowerCase();

You can use `getHashStringForCrypto` for the hexification.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:53
(Diff revision 1)
>  
> +// We should be able to temporarily install an unsigned web extension
> +// that does not have an ID in its manifest.
> +add_task(function* test_unsigned_no_id_temp_install() {
> +  if (!TEST_UNPACKED) {
> +    do_print("this test does not apply when using packed extensions");

Please capitalize.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:72
(Diff revision 1)
> +  function* filterAddonsByName(targetName) {
> +    const allAddons = yield new Promise(resolve => {
> +      AddonManager.getAllAddons(addons => resolve(addons));
> +    });
> +    do_print(`Found these add-ons: ${allAddons.map(a => a.name).join(", ")}`);
> +    return allAddons.filter(addon => addon.name === targetName);

Can we just save the add-on returned by `installTemporaryAddon` and check that the ID is the same the second time that we call it?

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:76
(Diff revision 1)
> +    do_print(`Found these add-ons: ${allAddons.map(a => a.name).join(", ")}`);
> +    return allAddons.filter(addon => addon.name === targetName);
> +  }
> +
> +  let filteredList = yield filterAddonsByName(manifest.name);
> +  do_check_eq(filteredList.length, 1);

`equal(...)`

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:88
(Diff revision 1)
> +  yield AddonManager.installTemporaryAddon(addonDir);
> +
> +  filteredList = yield filterAddonsByName(manifest.name);
> +  // Make sure the old add-on was removed.
> +  do_check_eq(filteredList.length, 1);
> +

We should also check that installing another temporary add-on alongside this one, with a different directory name that's the same length (and where none of the differing characters are numbers) gives us a separate add-on instance, with a separate ID.

Comment 9

[Kris Maglione [:kmag]]

Oh, and we should probably generate a random salt, or a random suffix, for each session, just in case someone decides to publish an add-on with its generated ID.

Comment 10

[Kumar McMillan [:kumar]]

(In reply to Kris Maglione [:kmag] from comment #9)
> Oh, and we should probably generate a random salt, or a random suffix, for
> each session, just in case someone decides to publish an add-on with its
> generated ID.

I don't follow. Do we have forthcoming code that will publish an extension from the browser? Otherwise, the ID will never make it to their manifest. Also, I can't think of why publishing an extension with the current auto-generated ID would be bad.

Comment 11

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/57694/#review54574

> This needs to be an array of character codes. I'd go with `new TextEncoder().encode(dirPath)`

It looks like TextEncoder.encode() will return an array of encoded bytes. I was hoping to keep it within the realm of Unicode and I was assuming theString.split("") will give me an array of Unicode code points (right?). I'd like to avoid encoding the directory path into bytes because I don't know how TextEncoder is going to choose what encoding to use. File system encodings are whacko and if it is choosing something like UTF8 then there could be problems. For example, a UTF16 Windows file system may not encode as UTF8 -- https://en.wikipedia.org/wiki/UTF-8#Invalid_code_points . Although I have no idea how Firefox will decode such a file name in the first place. In any case, not introducing byte conversion seems safer to me.

Comment 12

[Kris Maglione [:kmag]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #10)
> (In reply to Kris Maglione [:kmag] from comment #9)
> > Oh, and we should probably generate a random salt, or a random suffix, for
> > each session, just in case someone decides to publish an add-on with its
> > generated ID.
> 
> I don't follow. Do we have forthcoming code that will publish an extension
> from the browser? Otherwise, the ID will never make it to their manifest.
> Also, I can't think of why publishing an extension with the current
> auto-generated ID would be bad.

I'd just like to avoid the possibility of the generated IDs ever clashing with
a normally installed add-on. It's not really a big deal, but it's so easy to
avoid by just adding a per-session salt that I think we should do it.

Comment 13

[Kris Maglione [:kmag]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #11)
> https://reviewboard.mozilla.org/r/57694/#review54574
> 
> > This needs to be an array of character codes. I'd go with `new TextEncoder().encode(dirPath)`
> 
> It looks like TextEncoder.encode() will return an array of encoded bytes. I
> was hoping to keep it within the realm of Unicode and I was assuming
> theString.split("") will give me an array of Unicode code points (right?).
> I'd like to avoid encoding the directory path into bytes because I don't
> know how TextEncoder is going to choose what encoding to use. File system
> encodings are whacko and if it is choosing something like UTF8 then there
> could be problems. For example, a UTF16 Windows file system may not encode
> as UTF8 -- https://en.wikipedia.org/wiki/UTF-8#Invalid_code_points .
> Although I have no idea how Firefox will decode such a file name in the
> first place. In any case, not introducing byte conversion seems safer to me.

string.split("") gives you an array of single-character strings, not
codepoints. Even if it did give you codepionts, nsICryptoHash can only hash
octets, so UTF-16 code units (which is how JavaScript strings are encoded)
would just wind up throwing away data.

Don't worry about encoding issues. TextEncoder.encode() will not choke on any
data that you give it, and even if there are encoding issues, the result will
be more than good enough to generate a hash.

Comment 14

[Kumar McMillan [:kumar]]

(In reply to Kris Maglione [:kmag] from comment #12)
> (In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment
> #10)
> > (In reply to Kris Maglione [:kmag] from comment #9)
> > > Oh, and we should probably generate a random salt, or a random suffix, for
> > > each session, just in case someone decides to publish an add-on with its
> > > generated ID.
> > 
> > I don't follow. Do we have forthcoming code that will publish an extension
> > from the browser? Otherwise, the ID will never make it to their manifest.
> > Also, I can't think of why publishing an extension with the current
> > auto-generated ID would be bad.
> 
> I'd just like to avoid the possibility of the generated IDs ever clashing
> with
> a normally installed add-on. It's not really a big deal, but it's so easy to
> avoid by just adding a per-session salt that I think we should do it.

We do want IDs to clash with installed add-ons so you can replace them -- that was one of the original use cases for installing temporary add-ons (and there is already code to support it). Example: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpcshell/test_temporary.js#111-112

Comment 15

[Kumar McMillan [:kumar]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #14)
> We do want IDs to clash with installed add-ons so you can replace them --
> that was one of the original use cases for installing temporary add-ons (and
> there is already code to support it). Example:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/xpcshell/test_temporary.js#111-112

This is a moot point though because only temporarily installed add-ons will can come from a directory path outside of the profile. Adding a per-session salt adds code complexity so I just want to make sure we need it.

Comment 16

[Kris Maglione [:kmag]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #14)
> (In reply to Kris Maglione [:kmag] from comment #12)
> > I'd just like to avoid the possibility of the generated IDs ever clashing
> > with
> > a normally installed add-on. It's not really a big deal, but it's so easy to
> > avoid by just adding a per-session salt that I think we should do it.
> 
> We do want IDs to clash with installed add-ons so you can replace them --
> that was one of the original use cases for installing temporary add-ons (and
> there is already code to support it). Example:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> test/xpcshell/test_temporary.js#111-112

Right, I'm talking specifically about our generated path-based IDs clashing
with explicit IDs, which I can't see a good use for.

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #15)
> This is a moot point though because only temporarily installed add-ons will
> can come from a directory path outside of the profile. Adding a per-session
> salt adds code complexity so I just want to make sure we need it.

It adds so little complexity that it doesn't worry me:

    const SESSION_SALT = = new Uint8Array(Float64Array.of(Math.random()).buffer);

    ...

    crypto.update(SESSION_SALT, SESSION_SALT.length);

Comment 17

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/57694/#review54574

> Can we just save the add-on returned by `installTemporaryAddon` and check that the ID is the same the second time that we call it?

Part of the bug was that duplicate add-ons appeared in the list (because a unique ID was auto-generated on each install/reload). I added a comment about why the test inspects the listing.

Comment 18

[Kumar McMillan [:kumar]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57694/diff/1-2/

Comment 19

[Kumar McMillan [:kumar]]

(In reply to Kris Maglione [:kmag] from comment #16)
> Right, I'm talking specifically about our generated path-based IDs clashing
> with explicit IDs, which I can't see a good use for.

How does the salt guarantee [or even increase the chance] that a generated path-based ID won't clash with an explicit ID? There is no way to predict the value of an explicit ID. My original approach won't protect against clashes and I don't see how the salted approach protects against clashes.

Comment 20

[Kris Maglione [:kmag]]

(In reply to Kumar McMillan [:kumar] (needinfo all the things) from comment #19)
> How does the salt guarantee [or even increase the chance] that a generated
> path-based ID won't clash with an explicit ID? There is no way to predict
> the value of an explicit ID. My original approach won't protect against
> clashes and I don't see how the salted approach protects against clashes.

Without the salt, it's possible to generate an explicit ID that would match a given path every time. With it, it's possible to generate an add-on with a 1/2^52 chance of matching a given path.

Comment 21

[Kris Maglione [:kmag]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

https://reviewboard.mozilla.org/r/57694/#review54870

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:76
(Diff revisions 1 - 2)
>      do_print(`Found these add-ons: ${allAddons.map(a => a.name).join(", ")}`);
>      return allAddons.filter(addon => addon.name === targetName);
>    }
>  
>    let filteredList = yield filterAddonsByName(manifest.name);
> -  do_check_eq(filteredList.length, 1);
> +  equal(filteredList.length, 1);

I still don't understand why we can't just check that the ID returned by the two `installTemporaryAddon` calls are the same.

Comment 22

[Kumar McMillan [:kumar]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57694/diff/2-3/

Comment 23

[Kumar McMillan [:kumar]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57694/diff/3-4/

Comment 24

[Kris Maglione [:kmag]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

https://reviewboard.mozilla.org/r/57694/#review54952

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:1351
(Diff revisions 2 - 4)
> +        const sess = TEMP_INSTALL_ID_GEN_SESSION;
> +        hasher.update(sess, sess.length);
>          hasher.update(data, data.length);
>          addon.id = `${getHashStringForCrypto(hasher)}@temporary-addon`;
> -        logger.info(`Generated temporary id ${addon.id} for ${aDir.path}`);
> +        logger.info(
> +          `Generated temp id ${addon.id} (${sess.join("")}) for ${aDir.path}`);

`sess.join("")` is not going to produce anything useful.

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:67
(Diff revisions 2 - 4)
>  
>    const addonDir = writeWebManifestForExtension(manifest, gTmpD,
>                                                  "the-addon-sub-dir");
>    yield AddonManager.installTemporaryAddon(addonDir);
>  
>    function* filterAddonsByName(targetName) {

We don't need this if we're checking that both `installTemporaryAddon` calls return an add-on with the same ID. That implies that there's only one instance.

Comment 25

[Kumar McMillan [:kumar]]

https://reviewboard.mozilla.org/r/57694/#review54952

> `sess.join("")` is not going to produce anything useful.

Seems pretty useful to me. It joins all the numbers together. If we ever see dupe add-ons again we could glance at the log to make sure that the same session ID is being used to generate each new add-on ID.

Comment 26

[Kumar McMillan [:kumar]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/57694/diff/4-5/

Comment 27

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/57694/#review54952

> Seems pretty useful to me. It joins all the numbers together. If we ever see dupe add-ons again we could glance at the log to make sure that the same session ID is being used to generate each new add-on ID.

Except that [11, 1] and [1, 11] are different values, but both show up as "111"

Comment 28

[Kumar McMillan [:kumar]]

(In reply to Kris Maglione [:kmag] from comment #27)
> Except that [11, 1] and [1, 11] are different values, but both show up as
> "111"

Doesn't matter. I just want it in the logging to know if the session ID ever changes between ID generations. Since it's one of the inputs to the hasher it will be essential for debugging. Otherwise there is no way to reason about the hashed value when something unexpected happens.

Comment 29

[Kumar McMillan [:kumar]]

Try run looks clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=59fcfdde8979

Comment 30

[Kumar McMillan [:kumar]]

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

Approval Request Comment
[Feature/regressing bug #]: 1273229
[User impact if declined]: Developing cross platform web extensions would be confusing because each time the developer reloads their add-on, a new one would appear. This would probably make the reload process intolerable after a while.
[Describe test coverage new/current, TreeHerder]: Two new tests were added in toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js . There were no tests previously covering this exact case of auto-generating an add-on ID.
[Risks and why]: The change should be low risk because the patch only touches code for temporarily installing an extension, something that only developers do on the about:debugging page.
[String/UUID change made/needed]: No string or UUID changes were made.

Comment 31

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/2011b521c197
auto-generate IDs for temp installs from dir path. r=kmag

Comment 32

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

https://hg.mozilla.org/mozilla-central/rev/2011b521c197

Comment 33

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

Comment on attachment 8759859 [details]
Bug 1273229 - auto-generate IDs for temp installs from dir path.

Fix for addon developers, should not have much risk, new tests added. 
OK to uplift to aurora and beta.

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/e5301bb031fb

Comment 35

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/28760febab26

Comment 36

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 49.0a1 (2016-05-16) under Windows 10 64-bit.

Verified fixed on Firefox 48.0.2 (20160823121617),Firefox 49 (20160912134115),Firefox 50.0a2 (2016) and Firefox 51.0a1 (2016-09-14) under Windows 10. The webextension is no longer multiplied while clicking “Reload” button.