1242752 - Fix exception raised when a WebExtension iframe is destroyed

Status: RESOLVED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Luca Greco [:rpl] [:luca] [:lgreco]]

An addon iframes' extension context is closed correctly when an extension is unloaded, but it fails if the iframe is removed with the exception:

   TypeError: this.extensionWindows is undefined

because of a typo in the DocumentManager.observe method.

I'm going to attach to this issue a patch with an additional test case which raise the exception, and the related fix.

Comment 1

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 001-fix-exception-on-iframe-destroy.patch

Comment 2

[Kris Maglione [:kmag]]

Comment on attachment 8711883 [details] [diff] [review]
001-fix-exception-on-iframe-destroy.patch

Review of attachment 8711883 [details] [diff] [review]:
-----------------------------------------------------------------

We should request uplift to aurora for this.

::: toolkit/components/extensions/test/mochitest/test_ext_contentscript_create_iframe.html
@@ +210,5 @@
> +     // Not necessary in browser-chrome tests, but monitorConsole gripes
> +     // if we don't call it.
> +     SimpleTest.waitForExplicitFinish();
> +
> +     SimpleTest.monitorConsole(resolve, [{ message: /extensionWindows is undefined/, forbid: true }]);

I'd rather just assert that there are no console messages. `SimpleTest.monitorConsole(resolve, [], true);`

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Unfortunately there are two unrelated errors which make the test to fail if I do so:

 - JavaScript Error: "TelemetryStopwatch: requesting elapsed time for nonexisting stopwatch. ...",
   related to resource://gre/modules/TelemetryStopwatch.jsm
 - JavaScript Error: "[Exception... "Component returned failure code: ...",
   related to chrome://browser/content/browser.js (TabsProgressListener.onStateChange)

How about forbid errors based on the "sourceName" as a compromise?

     let forbiddenMessages = [{
       forbid: true, isScriptError: true, sourceName: /ExtensionContent\.jsm/,
     }];
     ...
     SimpleTest.monitorConsole(resolve, forbiddenMessages);

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-fix-exception-webextension-iframe-destroy.patch

Updates on the previous patch version:

- fixed eslint warnings and errors
- applied the described tweak (forbid console errors based on the sourceName instead of the error message)

Re-applied the previous r+ flag.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

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

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: 0001-fix-exception-webextension-iframe-destroy.patch

Minor tweak on the previous version of the attached patch:

- removed the test case used to demonstrate the issue (a new test case to properly test that "the extension context is completely cleaned up when the iframe is removed" will be added in a follow up of this issue)

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e0c3c6e1044f

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/integration/fx-team/rev/e0c3c6e1044f

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/e0c3c6e1044f

Comment 11

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8713106 [details] [diff] [review]
0001-fix-exception-webextension-iframe-destroy.patch

Approval Request Comment
[Feature/regressing bug #]: 1214658
[User impact if declined]: minor memory leak and errors appearing in the error console.
[Describe test coverage new/current, TreeHerder]: the tear-down code isn't adequately tested, more comprehensive tests will be added as a follow-up.
[Risks and why]: The patch is composed by a low-risk single-line typo fix.
[String/UUID change made/needed]: None.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8713106 [details] [diff] [review]
0001-fix-exception-webextension-iframe-destroy.patch

Fix for recent regression, ok to uplift to aurora.

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 46+

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0feeda85b3bc