Closed Bug 1397257 Opened 7 years ago Closed 7 years ago

[Windows] Awesome Screenshot removing error for a second uninstallation

Categories

(WebExtensions :: Request Handling, defect, P1)

57 Branch
All
Windows
defect

Tracking

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

VERIFIED FIXED
mozilla57
Tracking Status
firefox-esr52 --- unaffected
firefox55 --- unaffected
firefox56 --- verified
firefox57 --- verified

People

(Reporter: vtamas, Assigned: haik)

References

Details

(Keywords: regression)

Attachments

(4 files)

Attached image 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.
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)
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
Priority: -- → P1
Assignee: nobody → haftandilian
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.
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)
Attached image 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)
Thanks, Vasilica. I need to spend some more time making sure the fix is correct and then I'll post for review.
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)
See Also: → 1398908
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+
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
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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?
Marking this affected now that we intend to uplift the patch from bug 1395898
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+
Flags: qe-verify+
Attached image 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
Flags: qe-verify+
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.