If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

getItemLocation on WinRegInstallLocation should return a clone of the item location

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Toolkit
Add-ons Manager
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: mossop, Assigned: mossop)

Tracking

Trunk
mozilla1.9.2a1
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

9 years ago
Otherwise callers can modify the location where we think the add-on is held.
(Assignee)

Comment 1

9 years ago
Created attachment 370286 [details] [diff] [review]
patch rev 1

Simple fix, clone the file from the id map. We can also drop cloning in getItemFile since they both work on getItemLocation which is already a unique reference.
Attachment #370286 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

9 years ago
Status: NEW → ASSIGNED
Attachment #370286 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 2

9 years ago
Pushed: http://hg.mozilla.org/mozilla-central/rev/b9659463f938
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
(Assignee)

Comment 3

9 years ago
Created attachment 373740 [details] [diff] [review]
unit test rev 1

This is a simple unit test to verify that we can't mutate the directory for a registry installed add-on. It's also the first test to actually verify registry installs which is probably helpful.

It will leave the Software\Mozilla key in the registry in case the user has real add-ons installed in that way.
Attachment #373740 - Flags: review?(robert.bugzilla)
Comment on attachment 373740 [details] [diff] [review]
unit test rev 1

Removing review... Dave is going to use a mock reg implementation such as the one used in bug 484579
Attachment #373740 - Attachment is obsolete: true
Attachment #373740 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 5

9 years ago
Created attachment 374164 [details] [diff] [review]
unit test rev 2

Better testcase using a basic pretend registry
Attachment #374164 - Flags: review?(robert.bugzilla)
Attachment #374164 - Flags: review?(robert.bugzilla) → review+
Comment on attachment 374164 [details] [diff] [review]
unit test rev 2

>diff --git a/toolkit/mozapps/extensions/test/unit/test_bug486195.js b/toolkit/mozapps/extensions/test/unit/test_bug486195.js
>new file mode 100644
>--- /dev/null
>+++ b/toolkit/mozapps/extensions/test/unit/test_bug486195.js
>@@ -0,0 +1,130 @@
>...
>+ * The Initial Developer of the Original Code is
>+ *      Dave Townsend <dtownsend@oxymoronical.com>.
>+ *
>+ * Portions created by the Initial Developer are Copyright (C) 2007
nit: year is wrong and I believe the name indentation is incorrect


>+function run_test()
>+{
>+  if (!("nsIWindowsRegKey" in Components.interfaces))
>+    return;
nit: can't you use Ci here?

r=me with at least the year fixed.
(Assignee)

Comment 7

9 years ago
Created attachment 375218 [details] [diff] [review]
patch rev 2

I've fixed the nits in the test however I've discovered that this fix causes a regression and registry installed items no longer uninstall correctly when the registry key is removed. Part of the uninstall process sets needs-uninstall in the startup cache which tried to get the item location. We need to return null if the item doesn't exist to keep the previous behaviour.

Can't see a way to test that specific bit since the winreg install location caches the list and we can't restart the EM completely to remove that.
Attachment #370286 - Attachment is obsolete: true
Attachment #374164 - Attachment is obsolete: true
Attachment #375218 - Flags: review?(robert.bugzilla)
Attachment #375218 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 8

9 years ago
Landed as http://hg.mozilla.org/mozilla-central/rev/edf759b1ba4a
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.