1314177 - refactor SystemAddonInstallLocation to share more with MutableDirectoryInstallLocation

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect, P3)

Description

[Robert Helmer [:rhelmer]]

After bug 1204156 lands, SystemAddonInstallLocation will be a subclass of MutableDirectoryInstallLocation and the common methods like installAddon, getStagingDir, etc. can be refactored to use the base class instead of duplicating code.

Comment 1

[Robert Helmer [:rhelmer]]

Attachment: Bug 1314177 - use class sugar for install location classes

Review commit: https://reviewboard.mozilla.org/r/123558/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123558/

Comment 2

[Robert Helmer [:rhelmer]]

Attachment: Bug 1314177 - remove redundancy in install location classes

Review commit: https://reviewboard.mozilla.org/r/123560/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/123560/

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8851200 [details]
Bug 1314177 - use class sugar for install location classes

https://reviewboard.mozilla.org/r/123558/#review126854

I think switch from Task.jsm to async would make this even tighter, but if you prefer to do that separately that's okay by me.

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8702
(Diff revision 1)
>    /**
>     * Removes any directories not currently in use or pending use after a
>     * restart. Any errors that happen here don't really matter as we'll attempt
>     * to cleanup again next time.
>     */
> -  cleanDirectories: Task.async(function*() {
> +  cleanDirectories() {

how about async?  leaving that for a follow-up seems fine

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8851201 [details]
Bug 1314177 - remove redundancy in install location classes

https://reviewboard.mozilla.org/r/123560/#review126860

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8504
(Diff revision 1)
>      if (this._addonSet.directory) {
>        this._directory = this._baseDir.clone();
>        this._directory.append(this._addonSet.directory);
>      }
>  
> -    OS.File.makeDir(this._directory.path);
> +    super.requestStagingDir();

don't you need to return this?

Comment 5

[Robert Helmer [:rhelmer]]

Comment on attachment 8851200 [details]
Bug 1314177 - use class sugar for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/123558/diff/1-2/

Comment 6

[Robert Helmer [:rhelmer]]

Attachment: Bug 1314177 - use class sugar and async/await for install location classes

Review commit: https://reviewboard.mozilla.org/r/124394/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124394/

Comment 7

[Robert Helmer [:rhelmer]]

Attachment: Bug 1314177 - remove redundancy in install location classes

Review commit: https://reviewboard.mozilla.org/r/124396/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/124396/

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

https://reviewboard.mozilla.org/r/124394/#review126948

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3173
(Diff revision 1)
> -      yield systemAddonLocation.cleanDirectories();
> +      await systemAddonLocation.cleanDirectories();
>        return;
>      }
>  
>      // Download all the add-ons
> -    let downloadAddon = Task.async(function*(item) {
> +    let downloadAddon = await async function(item) {

you don't want to `await` here, this should just be `async function downloadAddon(item) { ...`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8799
(Diff revision 1)
>      for (let addon of aAddons) {
> -      let install = yield createLocalInstall(addon._sourceBundle, location);
> +      let install = await createLocalInstall(addon._sourceBundle, location);
>        installs.push(install);
>      }
>  
> -    let installAddon = Task.async(function*(install) {
> +    let installAddon = await async function(install) {

no await

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8805
(Diff revision 1)
>        // Make the new install own its temporary file.
>        install.ownsTempFile = true;
>        install.install();
> -    });
> +    }
>  
> -    let postponeAddon = Task.async(function*(install) {
> +    let postponeAddon = await async function(install) {

no await

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124394/diff/1-2/

Comment 10

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/1-2/

Comment 11

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

https://reviewboard.mozilla.org/r/124394/#review126948

> no await

Thanks for catching those!

Comment 12

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124394/diff/2-3/

Comment 13

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/2-3/

Comment 14

[Andrew Swan [:aswan]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

https://reviewboard.mozilla.org/r/124394/#review127334

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:3173
(Diff revision 3)
> -      yield systemAddonLocation.cleanDirectories();
> +      await systemAddonLocation.cleanDirectories();
>        return;
>      }
>  
>      // Download all the add-ons
> -    let downloadAddon = Task.async(function*(item) {
> +    let downloadAddon = async function(item) {

how about just `async function downloadAddon(item) {...`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8719
(Diff revision 3)
>      }
>  
>      try {
>        let entries = [];
>  
> -      yield iterator.forEach(entry => {
> +      await iterator.forEach(entry => {

Why is this being `await`ed?  The `yield` in the original code looks pretty questionable...  Maybe just change it to a for loop?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8799
(Diff revision 3)
>      for (let addon of aAddons) {
> -      let install = yield createLocalInstall(addon._sourceBundle, location);
> +      let install = await createLocalInstall(addon._sourceBundle, location);
>        installs.push(install);
>      }
>  
> -    let installAddon = Task.async(function*(install) {
> +    let installAddon = async function(install) {

`async function installAddon(install) {...`
actually, it doesn't even do anything async so just a plain old function should be enough

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8805
(Diff revision 3)
>        // Make the new install own its temporary file.
>        install.ownsTempFile = true;
>        install.install();
> -    });
> +    }
>  
> -    let postponeAddon = Task.async(function*(install) {
> +    let postponeAddon = async function(install) {

`async function postponeAddon(install) {...`

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8862
(Diff revision 3)
>  
>   /**
>    * Resumes upgrade of a previously-delayed add-on set.
>    */
> -  resumeAddonSet: Task.async(function*(installs) {
> -    let resumeAddon = Task.async(function*(install) {
> +  async resumeAddonSet(installs) {
> +    let resumeAddon = async function(install) {

this isn't async, and can just be `function resumeAddon()`

Comment 15

[Andrew Swan [:aswan]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

https://reviewboard.mozilla.org/r/124396/#review127338

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8195
(Diff revision 3)
>      }
>  
>      return Promise.resolve();
>    }
>  
> -  /**
> +     /**

what's this about?

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:8232
(Diff revision 3)
>        logger.warn("Failed to remove staging dir", e);
>        // Failing to remove the staging directory is ignorable
>      }
>    }
>  
> +

why the extra line?

Comment 16

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

https://reviewboard.mozilla.org/r/124396/#review127338

> why the extra line?

Sorry, breaking in new editor (vscode)

Comment 17

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124394/diff/3-4/

Comment 18

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/3-4/

Comment 19

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124394/diff/4-5/

Comment 20

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/4-5/

Comment 21

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/5-6/

Comment 22

[Robert Helmer [:rhelmer]]

As discussed in IRC - a bunch of these async functions are because they are being used later in `waitForAllPromises` which expects a promise.

I've put back the async keyword just for the functions that need it, and simplified the loop that `cleanDirectories` does.

Comment 23

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124394/diff/5-6/

Comment 24

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/6-7/

Comment 25

[Robert Helmer [:rhelmer]]

Comment on attachment 8852167 [details]
Bug 1314177 - use class sugar and async/await for install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124394/diff/6-7/

Comment 26

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/7-8/

Comment 27

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/72344a4f272e
use class sugar and async/await for install location classes r=aswan
https://hg.mozilla.org/integration/autoland/rev/2cc97556735f
remove redundancy in install location classes r=aswan

Comment 28

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing xpcshell tests toolkit/mozapps/extensions/test/xpcshell/test_bug596607.js and toolkit/mozapps/extensions/test/xpcshell/test_registry.js on Windows:

https://hg.mozilla.org/integration/autoland/rev/ccb8a0d883c889e95bc3aa59179562cf3fe45dfb
https://hg.mozilla.org/integration/autoland/rev/41bab28a81c86578737e1bee8c06e04bdfc0fdc4

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=2cc97556735f18cbe1a05e2b6eb95ef80173d91f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=88941887&repo=autoland

11:47:48     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
11:47:48     INFO -  running event loop
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418066969	addons.manager	DEBUG	Application has been upgraded"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067138	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/addons/XPIProvider.jsm: ["XPIProvider"]"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067150	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/LightweightThemeManager.jsm: ["LightweightThemeManager"]"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067172	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/addons/GMPProvider.jsm"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067183	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/addons/PluginProvider.jsm"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067184	addons.manager	DEBUG	Starting provider: XPIProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067185	addons.xpi	DEBUG	startup"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067187	addons.xpi	INFO	SystemAddonInstallLocation directory is missing"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067188	addons.xpi	INFO	Removing all system add-on upgrades."
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067190	addons.xpi	DEBUG	Skipping unavailable install location app-system-defaults"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067192	addons.xpi	DEBUG	Skipping unavailable install location app-system-user"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067193	addons.xpi	WARN	Failed to add registry install location winreg-app-user: ReferenceError: |this| used uninitialized in WinRegInstallLocation class constructor (resource://gre/modules/addons/XPIProvider.jsm:8948:5) JS Stack trace: [e-mail]:8948:5 < [e-mail]:2718:24 < [e-mail]:2772:11 < [e-mail]:268:12 < [e-mail]:755:5 < [e-mail]:922:9 < [e-mail]:3099:5 < [e-mail]:65:9 < [e-mail]:533:5 < startupManager@head_addons.js:604:3 < run_test_1@test_bug596607.js:70:3 < run_test@test_bug596607.js:55:3 < _execute_test@c:\\slave\\test\\build\\tests\\xpcshell\\head.js:536:7 < @-e:1:1"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067200	addons.xpi	DEBUG	Skipping unavailable install location app-system-share"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067201	addons.xpi	DEBUG	Skipping unavailable install location app-system-local"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067201	addons.xpi	WARN	Failed to add registry install location winreg-app-global: ReferenceError: |this| used uninitialized in WinRegInstallLocation class constructor (resource://gre/modules/addons/XPIProvider.jsm:8948:5) JS Stack trace: [e-mail]:8948:5 < [e-mail]:2718:24 < [e-mail]:2790:11 < [e-mail]:268:12 < [e-mail]:755:5 < [e-mail]:922:9 < [e-mail]:3099:5 < [e-mail]:65:9 < [e-mail]:533:5 < startupManager@head_addons.js:604:3 < run_test_1@test_bug596607.js:70:3 < run_test@test_bug596607.js:55:3 < _execute_test@c:\\slave\\test\\build\\tests\\xpcshell\\head.js:536:7 < @-e:1:1"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067206	addons.xpi	DEBUG	checkForChanges"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067208	addons.xpi	INFO	SystemAddonInstallLocation directory is missing"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067210	addons.xpi	DEBUG	Loaded add-on state from prefs: {}"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067212	addons.xpi	DEBUG	getInstallState changed: false, state: {}"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067213	addons.xpi	DEBUG	Empty XPI database, setting schema version preference to 19"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067215	addons.xpi	DEBUG	No changes found"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067279	addons.manager	DEBUG	Registering shutdown blocker for XPIProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067280	addons.manager	DEBUG	Provider finished startup: XPIProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067280	addons.manager	DEBUG	Starting provider: LightweightThemeManager"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067281	addons.manager	DEBUG	Registering shutdown blocker for LightweightThemeManager"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067282	addons.manager	DEBUG	Provider finished startup: LightweightThemeManager"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067284	addons.manager	DEBUG	Starting provider: GMPProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067299	addons.manager	DEBUG	Registering shutdown blocker for GMPProvider"
11:47:48     INFO -  PID 4696 | [4696] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80520012: file c:/builds/moz2_slave/autoland-w32-d-000000000000000/build/src/extensions/cookie/nsPermissionManager.cpp, line 2666
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067300	addons.manager	DEBUG	Provider finished startup: GMPProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067301	addons.manager	DEBUG	Starting provider: PluginProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067301	addons.manager	DEBUG	Registering shutdown blocker for PluginProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067302	addons.manager	DEBUG	Provider finished startup: PluginProvider"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067313	addons.manager	DEBUG	Completed startup sequence"
11:47:48     INFO -  "CONSOLE_MESSAGE: (info) 1491418067335	addons.xpi-utils	DEBUG	Starting async load of XPI database c:\\users\\cltbld\\appdata\\local\\temp\\xpc-profile-dnrefx\\extensions.json"
11:47:48     INFO -  PID 4696 | 1491418067808	addons.xpi-utils	DEBUG	Rebuilding XPI database with no extensions
11:47:48  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell-unpack.ini:toolkit/mozapps/extensions/test/xpcshell/test_bug596607.js | run_test_1/< - [run_test_1/< : 75] null != null
11:47:48     INFO -  c:/slave/test/build/tests/xpcshell/tests/toolkit/mozapps/extensions/test/xpcshell/test_bug596607.js:run_test_1/<:75
11:47:48     INFO -  resource://gre/modules/AddonManager.jsm:safeCall:192
11:47:48     INFO -  resource://gre/modules/AddonManager.jsm:makeSafe/<:207
11:47:48     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:process:922
11:47:48     INFO -  resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:walkerLoop:806
11:47:48     INFO -  exiting test

Comment 29

[Robert Helmer [:rhelmer]]

Thanks. I'm not sure why my try runs didn't work here - this is a failure of the registry install location which is a windows-only test. I'll figure out what's going on.

Comment 30

[Robert Helmer [:rhelmer]]

(In reply to Robert Helmer [:rhelmer] from comment #29)
> Thanks. I'm not sure why my try runs didn't work here - this is a failure of
> the registry install location which is a windows-only test. I'll figure out
> what's going on.

The lack of test failure is apparently because I did a win64 artifact try run - I'll try win32 before trying to land again.

Comment 31

[Robert Helmer [:rhelmer]]

Oh, WinRegInstallLocation is trying to extend DirectoryInstallLocation without calling super(). Should be a one-line fix.

Comment 32

[Robert Helmer [:rhelmer]]

Comment on attachment 8852168 [details]
Bug 1314177 - remove redundancy in install location classes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/124396/diff/8-9/

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/abc4ecf21670
use class sugar and async/await for install location classes r=aswan
https://hg.mozilla.org/integration/autoland/rev/0e89b1131f8a
remove redundancy in install location classes r=aswan

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/abc4ecf21670
https://hg.mozilla.org/mozilla-central/rev/0e89b1131f8a