Closed Bug 1435100 Opened 6 years ago Closed 6 years ago

ExtensionTestCommon.jsm generateZipFile throws NS_ERROR_FILE_ACCESS_DENIED

Categories

(WebExtensions :: General, defect, P2)

58 Branch
defect

Tracking

(firefox59 verified, firefox60 verified)

VERIFIED FIXED
mozilla60
Tracking Status
firefox59 --- verified
firefox60 --- verified

People

(Reporter: mixedpuppy, Assigned: rpl)

References

Details

Attachments

(1 file, 2 obsolete files)

This is showing up in bunches of places now, some going back to july 2017
I didn't search exhaustively but in a sampling of recent failures, all were on Windows which has much stricter file locking rules than linux/mac.
Depends on: 1434777
Blocks: 1434777
No longer depends on: 1434777
Blocks: 1437582
Blocks: 1439596
Blocks: 1439011
As described in Bug 1437582, my suspect was that the reasons behind the failure of a test on the nsIFile.createUnique function (called from https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/components/extensions/ExtensionTestCommon.jsm#266) is actually triggered by a issue (non-fatal, because it doesn't make the test to fail when it happens) on removing the xpi file generated from the test that has been executed right before (https://searchfox.org/mozilla-central/rev/47cb352984bac15c476dcd75f8360f902673cb98/toolkit/components/extensions/Extension.jsm#1616).

As an additional confirmation of this analysis, the tests that presented the higher number of these failures are running right after a test that is injecting a CSS using an extension url, e.g.:

- browser_ext_tabs_lastAccessed.js was running after browser_ext_tabs_insertCSS.js (until it has been disabled on windows)
- browser_ext_tabs_lazy.js has started to fail more frequently after browser_ext_tabs_lastAccessed.js has been disabled on windows 
- test_ext_contentscript_devtools_metadata.html, which has also been disabled on windows for the same reason, was running after test_ext_contentscript_css.html
- test_ext_contentscript_exporthelpers.html has started to fail more frequently after test_ext_contentscript_devtools_metadata.html has been disabled on windows

Once I successfully reproduced this issue locally (on a window 10 machine and using "./mach mochitest --verify browser/components/extensions/test/browser/browser_ext_tabs_insertCSS.js") I've been able to verify that the reason for the error raised when the test is going to remove the generated xpi file is related to a CSS stylesheet, injected into a webpage using tabs.insertCSS with an extension url, which has not been completely removed from the extension "preloaded stylesheet" css caches immediately after the test extension is shutting down and then removed.

The attached patch introduces some additional changes to ExtensionContent.jsm to make the BaseCSSCache class able to ensure that all the cached stylesheets are deleted when the extension is shutting down (and a small change to ExtensionChild.jsm to emit a shutdown event from the BrowserExtensionContent class).

After I applied these changes locally, I've not been able to reproduce the NS_ERROR_FILE_ACCESS_DENIED error anymore (both the one raised from `nsIFile.createUnique` and the one raised from `file.remove`), and the following push to try seems to confirm that the applied changes are fixing the intermittency:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb45238b93a0339dfc9cce3f01e9ab51c50c9879 

This try build also has the second of the attached patches (which re-enable browser_ext_tabs_lastAccessed.js which has been disabled on windows during the past week because it was failing with a pretty high frequency), and none of the oranges is related to the NS_ERROR_FILE_ACCESS_DENIED error.
Attachment #8952735 - Flags: review?(mixedpuppy)
Comment on attachment 8952735 [details]
Bug 1435100 - Ensure preloaded css and script caches are cleared when a WebExtension is shutting down.

https://reviewboard.mozilla.org/r/221964/#review227902

::: toolkit/components/extensions/ExtensionContent.jsm:201
(Diff revision 1)
> +  deleteAll() {
> +    for (let url of this.keys()) {
> +      this.delete(url);
> +    }
> +  }

Please just override the built-in clear() method.
Comment on attachment 8952735 [details]
Bug 1435100 - Ensure preloaded css and script caches are cleared when a WebExtension is shutting down.

https://reviewboard.mozilla.org/r/221964/#review227914

::: toolkit/components/extensions/ExtensionContent.jsm:165
(Diff revision 1)
>  class BaseCSSCache extends CacheMap {
> +  constructor(expiryTimeout, defaultConstructor, extension) {
> +    super(expiryTimeout, defaultConstructor);
> +
> +    // Clear the cssCache once the extension is being unloaded (to ensure that
> +    // none of the extension urls is still used in any of the existent

"are still"

::: toolkit/components/extensions/ExtensionContent.jsm:197
(Diff revision 1)
> +  // This method is called once the extension shutdown, to ensure that all the
> +  // cached stylesheet are removed from the any existent document and removed
> +  // from the cache (so that the extension xpi file is not actively used by Firefox
> +  // anymore once the extension has been disabled/uninstalled).

This method is called when the extension is shutdown.  This ensures that all the cached stylesheets are deleted from the cache to ensure the xpi is no longer actively used.
Attachment #8952735 - Flags: review?(mixedpuppy) → review+
I've applied the tweaks suggested by Kris (comment 6) and Shane (comment 7), besides that I've also extended the same "cache clearing on extension shutdown" for the caches related to the script because fixing the issue with the css files has helped to make the similar intermittent related to the cached scripts to be visible in one of my last push to try (https://treeherder.mozilla.org/#/jobs?repo=try&revision=cb616854bf89ef94596f2008f2cbd3da2d7a8000&selectedJob=163518028 which is tracked by Bug 1399348).

In the last push to try (which includes the last round of changes on the attached patch) none of the oranges is related to the NS_ERROR_FILE_ACCESS_DENIED issue anymore (on the contrary they are all related to Bug 1438663, browser_ext_popup_focus.js, which has been disabled 22h ago for frequent failures, and so it was still active when I created the hg bookmark where I've been working on this fix):

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=f06c507ec0bc9e3e507732bc9ea8849f33d0f9d4
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
Comment on attachment 8952735 [details]
Bug 1435100 - Ensure preloaded css and script caches are cleared when a WebExtension is shutting down.

https://reviewboard.mozilla.org/r/221964/#review228284
Attachment #8952736 - Attachment is obsolete: true
Attachment #8952812 - Attachment is obsolete: true
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/75cd2c0a8af9
Ensure preloaded css and script caches are cleared when a WebExtension is shutting down. r=mixedpuppy
https://hg.mozilla.org/mozilla-central/rev/75cd2c0a8af9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Blocks: 1435639
Comment on attachment 8952735 [details]
Bug 1435100 - Ensure preloaded css and script caches are cleared when a WebExtension is shutting down.

Approval Request Comment

[Feature/Bug causing the regression]:

This patch can be considered as a follow up of the changes introduced by Bug 1420485,
and it fixes a number of intermittent failures on the windows platform
which had a pretty high frequency on mozilla-central (and it seems that
there are also some of these failures on beta, but way less frequent than on
mozilla-central, eg. these are some failures on beta that seems to be related to the same issue: https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1434777&startday=2018-02-18&endday=2018-02-23&tree=all).

It is not 100% clear if the regression has been introduced by Bug 1420485
or it has triggered an existent issue and made it more frequent, because there are intermittents related to the same error (and so likely the same issue)
that are older than when we landed the patches from Bug 1420485, but it
still seems that Bug 1420485 could have at least contributed to the higher intermittency experienced on mozilla-central over the last weeks.
(and Bug 1420485 has been uplifted to beta 16 days ago: https://bugzilla.mozilla.org/show_bug.cgi?id=1420485#c80)

The fix ensures that the all the caches of the preloaded css and scripts injected by an extension are completely cleared when the extension is shutting down,
otherwise the xpi file of the extension will be kept active by Firefox
even after the extension as been disabled (which can prevent the extension
to be completely removed from disk on windows, because windows will raise
an error if the file is still used).

[User impact if declined]:

We are not sure if this issue has a visible impact on the users (e.g. none of the bugs reported by users seems to suggest it, we noticed the issue because of the high intermittency of the automated tests),
but because of it the xpi file of some extension may still be kept active by Firefox after the extension has been disabled by a user, and there is a chance that the same exception that was preventing the tests from removing the generated xpi of the
test extensions may also be triggered by the Firefox AddonManager when
the user wants to remove the extension.

[Is this code covered by automated tests?]:

Yes, the patch doesn't add new tests, but many of the existent tests cover the APIs that use this caches,
we tested that the intermittent were not failing anymore when the patch has been applied, by pushing it on try (and re-enabling in the same push to try all the tests disabled because of this issue, to have an higher chance to trigger the failures).
The final confirmation that this patch has fixed the related intermittents as we expect is going to be orangefactor data for the next week.

[Has the fix been verified in Nightly?]:

No

[Needs manual test from QE? If yes, steps to reproduce]: 

No

[List of other uplifts needed for the feature/fix]:

None

[Is the change risky?]:

Mid/Low risk

[Why is the change risky/not risky?]:

Mid/Low risky because even if the actual change is small and limited to a small area of that part of the internals, the change is still applied to the internal components that are used for the extensions' content scripts (both the ones registered from the extension manifest and the ones dynamically injected, e.g. using the tabs.executeScript API method) and "injected stylesheet" (the ones injected using the tabs.insertCSS API method), which are one of the 
WebExtensions features used by many extensions (including the adblockers which
are one of the most commonly installed kinds of extensions).

[String changes made/needed]:
None
Attachment #8952735 - Flags: approval-mozilla-beta?
Comment on attachment 8952735 [details]
Bug 1435100 - Ensure preloaded css and script caches are cleared when a WebExtension is shutting down.

Follow-up fix for bug 1420485 (uplifted to 59), which appears to have exacerbated a number of intermittent test failures since landing. Given the risk to popular addons, some exploratory testing by QE for any functional issues in 59b13 would be a good idea.
Flags: needinfo?(andrei.vaida)
Attachment #8952735 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Hello,
We have just performed the testing of the Beta 59.0b13-build1 today and we have included our "Add-on Compatibility" test suite to cover this bug on the following OSs:

- Windows 8.1 x64
- Windows 10 x86
- macOS X 10.11
- Ubuntu 16.04 x64

Our team hasn't found any new issues regarding this bug or other new bugs related to add-ons or WebExtensions.
Flags: needinfo?(andrei.vaida)
Hi again,
I've just finished testing the fix for this issue on Nightly 60.0a1 (id: 20180228100110) on the following OSs:

- Windows 8.1 x86
- Windows 10 x64
- macOS 10.13
- Ubuntu 16.04 x64

I haven't found any issues with the WebExtensions or other add-ons. Marking this as VERIFIED FIXED.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
See Also: → 1463489
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.