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

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: kumar, Assigned: aswan)

Tracking

unspecified
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

(Whiteboard: triaged)

Attachments

(2 attachments, 1 obsolete attachment)

Created attachment 8759383 [details]
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
...
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.
Component: WebExtensions → Add-ons Manager
Flags: needinfo?(aswan)
(Assignee)

Comment 2

3 years ago
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.
Assignee: nobody → aswan
(Assignee)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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?
Flags: needinfo?(aswan) → needinfo?(kmaglione+bmo)
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.
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Comment 6

3 years ago
Created attachment 8759786 [details]
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/
Attachment #8759786 - Flags: review?(kmaglione+bmo)
Attachment #8759787 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 7

3 years ago
Created attachment 8759787 [details]
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 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?
Attachment #8759786 - Flags: review?(kmaglione+bmo)
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.
(Assignee)

Comment 10

3 years ago
(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...
(Assignee)

Updated

3 years ago
Depends on: 1277965
(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 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?
Attachment #8759787 - Flags: review?(kmaglione+bmo)
(Assignee)

Comment 13

3 years ago
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/
Attachment #8759786 - Attachment description: Bug 1277695 Warn if AddonInstall cannot read id → Bug 1277695 Add sanity checks for addons with no id
Attachment #8759786 - Flags: review?(kmaglione+bmo)
(Assignee)

Updated

3 years ago
Attachment #8759787 - Attachment is obsolete: true
Attachment #8759786 - Flags: review?(kmaglione+bmo) → review+
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/
(Assignee)

Comment 15

3 years ago
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/
(Assignee)

Comment 16

3 years ago
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/
(Assignee)

Updated

3 years ago
Whiteboard: triaged

Comment 17

3 years ago
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b598ce1977bc
Add sanity checks for addons with no id r=kmag

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b598ce1977bc
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox50: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.