Open Bug 465190 Opened 11 years ago Updated 9 years ago

Don't create system extension directories on startup

Categories

(Toolkit :: Startup and Profile System, defect)

defect
Not set

Tracking

()

People

(Reporter: mossop, Unassigned)

References

Details

(Keywords: fixed1.9.1)

Attachments

(1 file, 2 obsolete files)

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.
Duplicate of this bug: 470507
Blocks: 459117
Attached patch stop logging the failure to disk (obsolete) — Splinter Review
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.
Attachment #375050 - Flags: review?(robert.bugzilla)
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.
Attachment #375050 - Flags: review?(robert.bugzilla) → review-
Attached patch patch rev 2 (obsolete) — Splinter Review
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.
Attachment #375050 - Attachment is obsolete: true
Attachment #375082 - Flags: review?(robert.bugzilla)
Attached patch patch rev 3Splinter Review
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.
Attachment #375082 - Attachment is obsolete: true
Attachment #375203 - Flags: review?(robert.bugzilla)
Attachment #375082 - Flags: review?(robert.bugzilla)
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("xpcshell@tests.mozilla.org", "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
Attachment #375203 - Flags: review?(robert.bugzilla) → review+
(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
Attachment #375203 - Flags: approval1.9.1?
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.
Attachment #375203 - Flags: approval1.9.1? → approval1.9.1+
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.
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [needs 191 landing]
Keywords: checkin-needed
Managed to get it after all: http://hg.mozilla.org/releases/mozilla-1.9.1/rev/5134c1c002de
Whiteboard: [needs 191 landing]
Assignee: dtownsend → nobody
You need to log in before you can comment on or make changes to this bug.