486195 - getItemLocation on WinRegInstallLocation should return a clone of the item location

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Otherwise callers can modify the location where we think the add-on is held.

Comment 1

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: 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.

Comment 2

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Pushed: http://hg.mozilla.org/mozilla-central/rev/b9659463f938

Comment 3

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: 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.

Comment 4

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

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

Comment 5

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: unit test rev 2

Better testcase using a basic pretend registry

Comment 6

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

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 <[e-mail]>.
>+ *
>+ * 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.

Comment 7

[Dave Townsend [:mossop] (PTO until Aug 21st)]

Attachment: 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.

Comment 8

[Dave Townsend [:mossop] (PTO until Aug 21st)]

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