1277695 - test_webextension_install.js: test_implicit_id FAIL: Failed to move file, NS_ERROR_FILE_NAME_TOO_LONG

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Kumar McMillan [:kumar]]

Attachment: test_webextension_install.txt

STR:
- configure artifact builds
- use Mac OS X (although it may fail elsewhere)
- run: ./mach test toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js

I'm attaching the full log but the interesting bit is this:

0:07.69 PROCESS_OUTPUT: Thread-5 (pid:38881) "1464902970973	addons.xpi	ERROR	Failed to move entry /var/folders/h7/rttdqkks7fl_txp9_rr4j4840000gn/T/xpc-profile-tqoDLG/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/trash/extensions/undefined.xpi: [Exception... "Component returned failure code: 0x80520011 (NS_ERROR_FILE_NAME_TOO_LONG) [nsIFile.moveTo]"  nsresult: "0x80520011 (NS_ERROR_FILE_NAME_TOO_LONG)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: SafeInstallOperation.prototype._installFile :: line 424"  data: no] Stack trace: SafeInstallOperation.prototype._installFile()@resource://gre/modules/addons/XPIProvider.jsm:424 < SafeInstallOperation.prototype._installDirEntry()@resource://gre/modules/addons/XPIProvider.jsm:505 < SafeInstallOperation.prototype._installDirectory()@resource://gre/modules/addons/XPIProvider.jsm:454 < SafeInstallOperation.prototype._installDirEntry()@resource://gre/modules/addons/XPIProvider.jsm:503
...

Comment 1

[Kris Maglione [:kmag]]

Andrew, can you look into this? It looks like it's happening because of some of your changes.

As far as I can tell, what's happening is that it's trying to install an add-on without an ID, and rather than generating one, it's using `undefined`. It checks for conflicts in the profile directory, but nsIFile.append(undefined) succeeds without an error, and the file still points to the profile extensions directory. It then thinks this is a conflicting extension install, and tries to move that directory to `extensions/trash`, which is a subdirectory of itself, and gets into an infinite loop.

Comment 2

[Andrew Swan [:aswan]]

Hm, I agree with your assessment and I can reproduce this too, I'll dig deeper.
I'm perplexed though about why this fails locally but not on try.

Comment 3

[Andrew Swan [:aswan]]

Hm, there's logic to mock out the signature validation in xpcshell so that we don't have to sign xpi's, but if that logic is in place then we can't get the addon id from the certificate.
The local/try difference remains a mystery, I'll pick this up later tonight.

Comment 4

[Andrew Swan [:aswan]]

I suspect this has has to do with the (MOZ_)ADDON_SIGNING flag, and that that flag is true by default in try builds but false elsewhere.  dxr took me here: https://dxr.mozilla.org/mozilla-central/source/build/mozconfig.common#20
but that suggests that signing should be enabled by default, I'm not sure what musty corner of the build/config system disables it locally.

In any case, I'm not sure what to do here, verifying implicit ids for signed xpi's kind of requires that we run signing code.  I could disable that test if signing is disabled or rig up some way to enable signing just for that test.  Kris, any thoughts?

Comment 5

[Kris Maglione [:kmag]]

Hm. You're right. It's defined by default in build/mozconfig.common, which is used by automation, but not used in most local builds. Adding it to my local mozconfig fixes the problem for me.

Regardless of the fix for that, we should add some sanity checks to the add-on manager code so we don't try to move a directory to a sub-directory of itself, or accept a null ID.

Comment 6

[Andrew Swan [:aswan]]

Attachment: Bug 1277695 Add sanity checks for addons with no id

AddonInstall is kind of a mess and can be used in a variety of ways
but this adds a check to the obvious case where we can end up with
an addon without an id.

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

Comment 7

[Andrew Swan [:aswan]]

Attachment: Bug 1277695 Skip tests that need signing if necessary

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

Comment 8

[Kris Maglione [:kmag]]

Comment on attachment 8759786 [details]
Bug 1277695 Add sanity checks for addons with no id

https://reviewboard.mozilla.org/r/57660/#review54510

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:435
(Diff revision 1)
>        throw e;
>      }
>      this._installedFiles.push({ oldFile: oldFile, newFile: newFile });
>    },
>  
>    _installDirectory: function(aDirectory, aTargetDirectory, aCopy) {

Can we also throw if `aDirectory.contains(aTargetDirectory)` here?

Comment 9

[Kris Maglione [:kmag]]

https://reviewboard.mozilla.org/r/57662/#review54512

Can you try to find out why we have signing checks disabled outside of automation runs? It seems like disabling these tests in local runs is going to be a footgun for people working on this code.

Otherwise, it would be nice to at least test that the install fails in a sane way when signing is disabled.

Comment 10

[Andrew Swan [:aswan]]

(In reply to Kris Maglione [:kmag] from comment #9)
> Can you try to find out why we have signing checks disabled outside of
> automation runs? It seems like disabling these tests in local runs is going
> to be a footgun for people working on this code.

I thought that was an explicit policy decision.  But there is a distinction between whether signing code is enabled and whether signing is required.  Of course we want to keep an option for the second, but perhaps its time to get rid of the config/build time flag for the former and just enable it always?

> Otherwise, it would be nice to at least test that the install fails in a
> sane way when signing is disabled.

Its tricky to do from a test since the actual error is caught, logged, and then discarded here: https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5305-5314
Searching for a particular string in the log sounds terrible...

Comment 11

[Kris Maglione [:kmag]]

(In reply to Andrew Swan [:aswan] from comment #10)
> > Otherwise, it would be nice to at least test that the install fails in a
> > sane way when signing is disabled.
> 
> Its tricky to do from a test since the actual error is caught, logged, and
> then discarded here:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#5305-5314
> Searching for a particular string in the log sounds terrible...

You can use `promiseConsoleOutput` for that.

Comment 12

[Kris Maglione [:kmag]]

Comment on attachment 8759787 [details]
Bug 1277695 Skip tests that need signing if necessary

https://reviewboard.mozilla.org/r/57662/#review54596

::: toolkit/mozapps/extensions/test/xpcshell/test_webextension_install.js:29
(Diff revision 1)
>  // applications or browser_specific_settings, so its id comes
>  // from its signature, which should be the ID constant defined below.
>  add_task(function* test_implicit_id() {
> +  // This test needs to read the xpi certificate which only works
> +  // if signing is enabled.  If it is not, skip the test...
> +  if (!ADDON_SIGNING) {

Can we just make this `ok(ADDON_SIGNING, "...")` since we're signing by default again?

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8759786 [details]
Bug 1277695 Add sanity checks for addons with no id

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

Comment 14

[Kris Maglione [:kmag]]

Comment on attachment 8759786 [details]
Bug 1277695 Add sanity checks for addons with no id

https://reviewboard.mozilla.org/r/57660/#review55240

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:437
(Diff revision 2)
>      this._installedFiles.push({ oldFile: oldFile, newFile: newFile });
>    },
>  
>    _installDirectory: function(aDirectory, aTargetDirectory, aCopy) {
> +    if (aDirectory.contains(aTargetDirectory)) {
> +      let err = new Error(`Not installing ${aDirectory} into its own ancestor ${aTargetDirectory}`);

s/ancestor/descendant/

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8759786 [details]
Bug 1277695 Add sanity checks for addons with no id

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

Comment 16

[Andrew Swan [:aswan]]

Comment on attachment 8759786 [details]
Bug 1277695 Add sanity checks for addons with no id

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

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b598ce1977bc
Add sanity checks for addons with no id r=kmag

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b598ce1977bc