[Windows] Awesome Screenshot removing error for a second uninstallation

VERIFIED FIXED in Firefox 56

Status

P1
normal
VERIFIED FIXED
a year ago
2 months ago

People

(Reporter: vasilica.mihasca, Assigned: haik)

Tracking

({regression})

57 Branch
mozilla57
All
Windows
regression

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 verified, firefox57 verified)

Details

MozReview Requests

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

Attachments

(4 attachments)

Created attachment 8905000 [details]
removingerror.gif

[Affected versions]:
Firefox 57.0a1 (20170905220108)

[Affected platforms]:
Windows 10 64-bit
Windows 7 64-bit


[Steps to reproduce]:
1.Launch Firefox Release with a clean profile.
2.Navigate to Add-ons Manager -> Get Add-ons and install Awesome Screenshot webextension.
3.Click on the switch button and remove the webextension.
4.Install the webextension one more time.
5.Remove it again.

 
[Expected Results]:
The webextensions is successfully uninstalled and there are no errors encountered. 

[Actual Results]:
- “An unexpected error occurred during uninstallation.” error is displayed in Disco Pane
- The following errors are thrown in Browser Console” https://pastebin.com/jPApVsNH .
- The webextensions still appears as installed in “Extensions” tab but the icon is no longer displayed in toolbar.
- This issue also reproduces while installing the webextension from amo. 
See attached screencast.

[Additional notes]:
- This issue is not reproducible under Ubuntu 16.04 32-bit and Mac OS X 10.12.3.
(Assignee)

Comment 1

a year ago
Vasilica, could you try to get a regression window?
Flags: needinfo?(vasilica.mihasca)
Flags: needinfo?(aswan)
(In reply to Haik Aftandilian [:haik] from comment #1)
> Vasilica, could you try to get a regression window?

Sorry, I forgot to mention that this issue was caused by Bug 1395898.

Last good revision: 6106d550ba968098cfb69a427c6dd646935a58d9
First bad revision: e12897c25fc597353f9ad42c9cc185a41fa9001f
Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=6106d550ba968098cfb69a427c6dd646935a58d9&tochange=e12897c25fc597353f9ad42c9cc185a41fa9001f

It is not reproducible with extensions.webextensions.remote set to false.
Flags: needinfo?(vasilica.mihasca)
(Assignee)

Comment 3

a year ago
I don't know the root cause yet, but I think there is a problem that has always existed with OOP extensions. The JAR caching that was turned on in bug 1376496 seems to mask the problem.

I tried going back to earlier builds on Windows with OOP extensions enabled. On Nightly 2017-07-17 on Windows where extensions.webextensions.remote=true, uninstalling Awesome Screenshot succeeds but the icon remains in the task bar. That was before 

This may be unrelated, but every version I tried triggers "failed to remove temporary file C:\Users\<USERNAME>\AppData\Local\Temp\tmp-<RANDOM>.xpi" during Awesome Screenshot installation:

1504807266088	addons.xpi	WARN	Failed to remove temporary file C:\Users\HAIKAF~1\AppData\Local\Temp\tmp-z0j.xpi for addon https://addons.mozilla.org/firefox/downloads/file/669836/awesome_screenshot_plus_capture_annotate_more-3.0.21-an+fx.xpi?src=discovery-promo: [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: resource://gre/modules/addons/XPIInstall.jsm :: removeTemporaryFile :: line 1558"  data: no] Stack trace: removeTemporaryFile()@resource://gre/modules/addons/XPIInstall.jsm:1558 < startInstall/<()@resource://gre/modules/addons/XPIInstall.jsm:1907

Updated

a year ago
Priority: -- → P1
(Assignee)

Updated

a year ago
Assignee: nobody → haftandilian
(Assignee)

Comment 4

a year ago
Correcting my cut-off comment above:

(In reply to Haik Aftandilian [:haik] from comment #3)
> I tried going back to earlier builds on Windows with OOP extensions enabled.
> On Nightly 2017-07-17 on Windows where extensions.webextensions.remote=true,
> uninstalling Awesome Screenshot succeeds but the icon remains in the task
> bar. That was before 

That was before bug 1376496 integrated.
Comment hidden (mozreview-request)
(Assignee)

Comment 7

11 months ago
Vasilica, could you test with the try build and see if it resolves the problem for you? Here's a direct link to the non-debug windows build.

  https://queue.taskcluster.net/v1/task/e3HOHcB7TbGUhoZ6sVBdpw/runs/0/artifacts/public/build/target.zip
Flags: needinfo?(vasilica.mihasca)
Created attachment 8905954 [details]
Animation1.gif

(In reply to Haik Aftandilian [:haik] from comment #7)
> Vasilica, could you test with the try build and see if it resolves the
> problem for you? Here's a direct link to the non-debug windows build.
>  
> https://queue.taskcluster.net/v1/task/e3HOHcB7TbGUhoZ6sVBdpw/runs/0/
> artifacts/public/build/target.zip

This issue is no longer reproducible while using the try-build under Windows 10 64-bit and Windows 7 64-bit. The webextension is enabled/disabled without error. See screencast.
Flags: needinfo?(vasilica.mihasca)
(Assignee)

Comment 9

11 months ago
Thanks, Vasilica. I need to spend some more time making sure the fix is correct and then I'll post for review.
(Assignee)

Comment 10

11 months ago
The problem here is also related to the handling of nonexistent JAR paths (that is, a path to an inner file that does not exist in the JAR) and it is a bug in the fix for bug 1395898.

In GetAsync() when we are handling a cached JAR and mJarChannel->AsyncOpen2(mListener) fails, we return the error to SimpleChannel::BeginAsyncRead() which exits early without clearing mCallbacks. SimpleChannel.mCallbacks holds an owning reference to the ExtensionStreamGetter. ExtensionStreamGetter holds an owning reference to the SimpleChannel and the cached JAR channel. As a result, we return from SimpleChannel::BeginAsyncRead() with a circular dependency relationship between SimpleChannel and ExtensionStreamGetter.

That prevents the ExtensionStreamGetter from being freed which means the cached JAR channel is never freed. The cached JAR channel holds an owning reference to an nsIZipReader for the cached JAR (to ensure it isn't displaced from the JAR cache before we read from the JAR.)

Holding the reference to the nsIZipReader prevents it from being freed when the JAR cache is cleared. Which also prevents the underlying file descriptor from being closed.

The messages on the developer console indicate the extension files can't be moved out of the profile. I haven't traced through the code to see exactly how failing to close the JAR triggers the uninstall failure. It is likely due to the parent having opened the file without using the ShareDelete flag. Still looking into that part of this.
Flags: needinfo?(aswan)
Comment hidden (mozreview-request)
(Assignee)

Updated

11 months ago
See Also: → bug 1398908

Comment 12

11 months ago
mozreview-review
Comment on attachment 8905777 [details]
Bug 1397257 - [Windows] Awesome Screenshot removing error for a second uninstallation.

https://reviewboard.mozilla.org/r/177580/#review183834
Attachment #8905777 - Flags: review?(jmathies) → review+

Comment 13

11 months ago
Pushed by haftandilian@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/74ab1f8f99bb
[Windows] Awesome Screenshot removing error for a second uninstallation. r=jimm
https://hg.mozilla.org/mozilla-central/rev/74ab1f8f99bb
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Assignee)

Comment 15

11 months ago
Comment on attachment 8905777 [details]
Bug 1397257 - [Windows] Awesome Screenshot removing error for a second uninstallation.

Approval Request Comment
[Feature/Bug causing the regression]:
Bug 1395898

[User impact if declined]:
Extensions may fail to uninstall.

[Is this code covered by automated tests?]:
Partially. Not all the error paths are covered.

[Has the fix been verified in Nightly?]:
Will be in the next Nightly build today.

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes. Follow Vasilica's steps on this bug description.

[List of other uplifts needed for the feature/fix]:
This fix depends on 1395898.

[Is the change risky?]:
No

[Why is the change risky/not risky?]:
Changes are isolated to extension handling and fix a regression. Changes are pretty well understood and frequently executed.

[String changes made/needed]:
None
Attachment #8905777 - Flags: approval-mozilla-beta?
status-firefox-esr52: --- → unaffected
Marking this affected now that we intend to uplift the patch from bug 1395898
status-firefox56: unaffected → affected
Comment on attachment 8905777 [details]
Bug 1397257 - [Windows] Awesome Screenshot removing error for a second uninstallation.

Fix for some install/uninstall situations for webextensions. OK to uplift for beta 12
Attachment #8905777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 18

11 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/fafd69ca2733
status-firefox56: affected → fixed
Flags: qe-verify+
Created attachment 8908617 [details]
Animation.gif

I confirm that the Awesome Screenshot extension is successfully uninstalled and there are no errors encountered on Firefox 57.0a1 (20170915100121) and Firefox 56.0b12 (20170914024831) under Windows 10 64-bit and Windows 7 64-bit. See screencast.
Status: RESOLVED → VERIFIED
status-firefox56: fixed → verified
status-firefox57: fixed → verified
Flags: qe-verify+

Updated

2 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.