refactor SystemAddonInstallLocation to share more with MutableDirectoryInstallLocation

RESOLVED FIXED in Firefox 55

Status

()

Toolkit
Add-ons Manager
P3
normal
RESOLVED FIXED
a year ago
8 months ago

People

(Reporter: rhelmer, Assigned: rhelmer)

Tracking

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
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.

Updated

a year ago
Priority: -- → P3
Whiteboard: triaged
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 3

8 months ago
mozreview-review
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
Attachment #8851200 - Flags: review?(aswan) → review+

Comment 4

8 months ago
mozreview-review
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?
Attachment #8851201 - Flags: review?(aswan) → review+
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8851201 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

8 months ago
Attachment #8851200 - Attachment is obsolete: true

Comment 8

8 months ago
mozreview-review
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 11

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 14

8 months ago
mozreview-review
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()`
Attachment #8852167 - Flags: review?(aswan) → review+

Comment 15

8 months ago
mozreview-review
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?
Attachment #8852168 - Flags: review?(aswan) → review+
(Assignee)

Comment 16

8 months ago
mozreview-review-reply
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 22

8 months ago
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 hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 27

8 months ago
Pushed by rhelmer@mozilla.com:
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
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: WinRegInstallLocation@XPIProvider.jsm:8948:5 < addRegistryInstallLocation@XPIProvider.jsm:2718:24 < startup@XPIProvider.jsm:2772:11 < callProvider@AddonManager.jsm:268:12 < _startProvider@AddonManager.jsm:755:5 < startup@AddonManager.jsm:922:9 < startup@AddonManager.jsm:3099:5 < observe@addonManager.js:65:9 < promiseStartupManager@AddonTestUtils.jsm: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: WinRegInstallLocation@XPIProvider.jsm:8948:5 < addRegistryInstallLocation@XPIProvider.jsm:2718:24 < startup@XPIProvider.jsm:2790:11 < callProvider@AddonManager.jsm:268:12 < _startProvider@AddonManager.jsm:755:5 < startup@AddonManager.jsm:922:9 < startup@AddonManager.jsm:3099:5 < observe@addonManager.js:65:9 < promiseStartupManager@AddonTestUtils.jsm: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
Flags: needinfo?(rhelmer)
(Assignee)

Comment 29

8 months ago
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.
Flags: needinfo?(rhelmer)
(Assignee)

Comment 30

8 months ago
(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.
(Assignee)

Comment 31

8 months ago
Oh, WinRegInstallLocation is trying to extend DirectoryInstallLocation without calling super(). Should be a one-line fix.
Comment hidden (mozreview-request)

Comment 33

8 months ago
Pushed by rhelmer@mozilla.com:
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

8 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/abc4ecf21670
https://hg.mozilla.org/mozilla-central/rev/0e89b1131f8a
Status: NEW → RESOLVED
Last Resolved: 8 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.