An empty staging dir is left around if an extension causes an update using runtime.reload()

RESOLVED FIXED in Firefox 52

Status

()

Toolkit
WebExtensions: Untriaged
P2
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: mattw, Assigned: rhelmer)

Tracking

unspecified
mozilla52
Points:
---

Firefox Tracking Flags

(firefox52 fixed)

Details

MozReview Requests

()

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

Attachments

(1 attachment)

(Reporter)

Description

a year ago
In bug 1252871, runtime.onInstalled is tested by mocking an update and having the update install by calling runtime.reload();  however, at the end of the xpcshell test, the following error is displayed:

0:01.97 LOG: Thread-3 ERROR Error: Test cleanup: path /Users/username/Firefox/mozilla-central/obj-desktop/obj-x86_64-apple-darwin15.4.0/temp/xpc-profile-bd_MQn/extensions/staged exists when it should not at resource://testing-common/AddonTestUtils.jsm:282
pathShouldntExist@resource://testing-common/AddonTestUtils.jsm:282:15

A current workaround is to add "this.installLocation.releaseStagingDir();" after http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#6261.
(Assignee)

Comment 1

a year ago
(In reply to Matthew Wein [:K-9, :mattw] from comment #0)
> In bug 1252871, runtime.onInstalled is tested by mocking an update and
> having the update install by calling runtime.reload();  however, at the end
> of the xpcshell test, the following error is displayed:
> 
> 0:01.97 LOG: Thread-3 ERROR Error: Test cleanup: path
> /Users/username/Firefox/mozilla-central/obj-desktop/obj-x86_64-apple-
> darwin15.4.0/temp/xpc-profile-bd_MQn/extensions/staged exists when it should
> not at resource://testing-common/AddonTestUtils.jsm:282
> pathShouldntExist@resource://testing-common/AddonTestUtils.jsm:282:15
> 
> A current workaround is to add "this.installLocation.releaseStagingDir();"
> after
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProvider.jsm#6261.

This doesn't seem like the wrong thing to do, but I'd like to understand why http://searchfox.org/mozilla-central/rev/572e74ee991bbfd812766b4524237eb77577a4b1/toolkit/mozapps/extensions/test/xpcshell/test_delay_update_webextension.js#125-183 passes while this doesn't ...
(Assignee)

Comment 2

a year ago
Turns out `test_delay_update_webextension.js` is hiding the bug, since it's restarting AddonManager. I'll fix this up.
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
mozreview-review
Comment on attachment 8796398 [details]
Bug 1306492 - release staging directory when add-on install is postponed

https://reviewboard.mozilla.org/r/82264/#review80898

Hm actually thinking about this, we're intenionally staging the install for the next restart, right? Don't we expect the staging dir to hang around if that's the case?
(Assignee)

Comment 5

a year ago
Comment on attachment 8796398 [details]
Bug 1306492 - release staging directory when add-on install is postponed

Don't bother reviewing this, let's discuss tomorrow.
Attachment #8796398 - Flags: review?(mwein)
(Assignee)

Comment 6

a year ago
Oh ok, I get it now - right, the stage dir should be unlocked after a formerly-postponed extension is resumed.
Comment hidden (mozreview-request)
Comment on attachment 8796398 [details]
Bug 1306492 - release staging directory when add-on install is postponed

https://reviewboard.mozilla.org/r/82264/#review81068

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:6262
(Diff revision 2)
>              yield this.stageInstall(true, stagedAddon, true);
>  
>              AddonManagerPrivate.callInstallListeners("onInstallPostponed",
>                                                       this.listeners, this.wrapper)
>  
> +            yield this.installLocation.releaseStagingDir();

This doesn't seem right. Shouldn't we only be releasing the staging directory when the add-on explicitly resumes the postponed installation?
Comment hidden (mozreview-request)
(Assignee)

Comment 10

a year ago
mozreview-review-reply
Comment on attachment 8796398 [details]
Bug 1306492 - release staging directory when add-on install is postponed

https://reviewboard.mozilla.org/r/82264/#review81068

> This doesn't seem right. Shouldn't we only be releasing the staging directory when the add-on explicitly resumes the postponed installation?

Hm yeah I thought about that... I worry a bit about locking it indefinitely if a postponed add-on is never resumed, but I guess that's what we're doing currently and we've seen no ill effect so it's certainly no worse.
Comment on attachment 8796398 [details]
Bug 1306492 - release staging directory when add-on install is postponed

https://reviewboard.mozilla.org/r/82264/#review81068

> Hm yeah I thought about that... I worry a bit about locking it indefinitely if a postponed add-on is never resumed, but I guess that's what we're doing currently and we've seen no ill effect so it's certainly no worse.

My understanding is that we're locking the staging directory at this point so that the update is automatically installed on the next restart if it isn't installed before then. So I would expect releasing the staging directory to break that.

Which should also probably break a test...
Comment on attachment 8796398 [details]
Bug 1306492 - release staging directory when add-on install is postponed

https://reviewboard.mozilla.org/r/82264/#review81468
Attachment #8796398 - Flags: review?(kmaglione+bmo) → review+

Comment 13

a year ago
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c554ce55f7eb
release staging directory when add-on install is postponed r=kmag

Comment 14

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c554ce55f7eb
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox52: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
You need to log in before you can comment on or make changes to this bug.