465190 - Don't create system extension directories on startup

Status: NEW

(Toolkit :: Startup and Profile System, defect)

Description

[Dave Townsend [:mossop]]

Currently during startup we attempt to create the system extension directories and it looks like we fail kinda hard when this doesn't work. We probably shouldn't be having the app attempt to create stuff in /usr/lib etc. or if it is necessary have it gracefully continue if it doesn't have permission to do so.

Comment 2

[Dave Townsend [:mossop]]

Attachment: stop logging the failure to disk

This is harder to stop doing than it seems since we'll keep trying to create things there anyway for the canAccess checks at the moment. For now let's just downgrade the error to a standard log to solve the basic problem for Fennec. Also just make canAccess false if we failed to create the directory in the first place.

Comment 3

[Robert Strong (they/them - no direct email)]

Comment on attachment 375050 [details] [diff] [review]
stop logging the failure to disk

>diff --git a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>--- a/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>+++ b/toolkit/mozapps/extensions/src/nsExtensionManager.js.in
>...
>@@ -1196,16 +1196,21 @@ DirectoryInstallLocation.prototype = {
> 
>   /**
>    * See nsIExtensionManager.idl
>    */
>   get canAccess() {
>     if (this._canAccess != null)
>       return this._canAccess;
> 
>+    if (this.location.exists()) {
>+      this._canAccess = false;
>+      return false;
>+    }
So, if the location exists canAccess will return false? If this is really what you want to do please add a comment explaining why this is correct because I find that logic rather confusing to say the least.

Comment 4

[Dave Townsend [:mossop]]

Attachment: patch rev 2

That's what I get for rushing things. If the location doesn't exist then it couldn't be created in the constructor so canAccess is false.

Comment 5

[Dave Townsend [:mossop]]

Attachment: patch rev 3

This adds a testcase. In order to trigger a failure to create the install location directory I've overridden one of the locations to be the child of a location that is actually a file. This will cause the attempt to create the directory to fail and so previously to log an error.

Comment 6

[Robert Strong (they/them - no direct email)]

Comment on attachment 375203 [details] [diff] [review]
patch rev 3

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug465190.js b/toolkit/mozapps/extensions/test/unit/test_bug465190.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/unit/test_bug465190.js
>@@ -0,0 +1,72 @@
>...
>+function run_test()
>+{
>+  var log = gProfD.clone();
>+  log.append("extensions.log");
>+  do_check_false(log.exists());
I would think that you should just remove the extensions.log if it exists here instead of testing if it exists. If it does exist it would be due to a previous test creating it and that should be checked in a previous test... true?

>+  // Setup for test
>+  createAppInfo("[e-mail]", "XPCShell", "1");
>+
>+  startupEM();
>+  do_check_false(log.exists());
>+}

r=me with either that fixed or an explanation as to why it should remain as a check

Comment 7

[Dave Townsend [:mossop]]

(In reply to comment #6)
> (From update of attachment 375203 [details] [diff] [review])
> >diff --git a/toolkit/mozapps/extensions/test/unit/test_bug465190.js b/toolkit/mozapps/extensions/test/unit/test_bug465190.js
> >new file mode 100644
> >--- /dev/null
> >+++ b/toolkit/mozapps/extensions/test/unit/test_bug465190.js
> >@@ -0,0 +1,72 @@
> >...
> >+function run_test()
> >+{
> >+  var log = gProfD.clone();
> >+  log.append("extensions.log");
> >+  do_check_false(log.exists());
> I would think that you should just remove the extensions.log if it exists here
> instead of testing if it exists. If it does exist it would be due to a previous
> test creating it and that should be checked in a previous test... true?

head_extensionmanager.js which runs before each test deletes any existing gProfD and creates a fresh directory so there cannot be anything left over from a previous test. Technically I can't think of a way extensions.log could ever appear here, but I'd rather just leave the check in anyway since if it does exist something very strange is going on.

Landed as http://hg.mozilla.org/mozilla-central/rev/19a8fc7a2812

I'm going to keep this bug open as all we've fixed here is logging about the problem, I'd still like to be able to stop creating these directories at all

Comment 8

[Robert Strong (they/them - no direct email)]

It could appear in part due to other bugs such as bug 473385. I personally think that each test should just focus on what it is testing to avoid random oranges due to bugs elsewhere in the codebase but then again tests such as this might find an unknown bug as well.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 375203 [details] [diff] [review]
patch rev 3

a191=beltzner

Comment 10

[Dave Townsend [:mossop]]

Doesn't look like I'll have the opportunity to land this so if someone else could get it on branch before freeze that'd be great.

Comment 11

[Dave Townsend [:mossop]]

Managed to get it after all: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5134c1c002de