Fix exception raised when a WebExtension iframe is destroyed

RESOLVED FIXED in Firefox 46

Status

()

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

People

(Reporter: rpl, Assigned: rpl)

Tracking

({regression})

46 Branch
mozilla47
regression
Points:
---
Bug Flags:
blocking-webextensions +

Firefox Tracking Flags

(firefox45 unaffected, firefox46+ fixed, firefox47+ fixed)

Details

(Whiteboard: [triaged])

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Comment 1

2 years ago
Created attachment 8711883 [details] [diff] [review]
001-fix-exception-on-iframe-destroy.patch
Attachment #8711883 - Flags: review?(kmaglione+bmo)
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);`
Attachment #8711883 - Flags: review?(kmaglione+bmo) → review+
(Assignee)

Comment 3

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=383db4705e13
(Assignee)

Comment 4

2 years ago
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);
(Assignee)

Comment 5

2 years ago
Created attachment 8712131 [details] [diff] [review]
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.
Attachment #8711883 - Attachment is obsolete: true
Attachment #8712131 - Flags: review+
(Assignee)

Comment 6

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab31e7a51672
(Assignee)

Updated

2 years ago
Assignee: nobody → lgreco
Status: NEW → ASSIGNED
(Assignee)

Comment 7

2 years ago
Created attachment 8713106 [details] [diff] [review]
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)
Attachment #8712131 - Attachment is obsolete: true
Attachment #8713106 - Flags: review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Updated

2 years ago
Flags: blocking-webextensions+
Priority: -- → P2
Whiteboard: [triaged]

Comment 8

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/e0c3c6e1044f
Keywords: checkin-needed

Comment 9

2 years ago
bugherderlanding
https://hg.mozilla.org/integration/fx-team/rev/e0c3c6e1044f

Comment 10

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e0c3c6e1044f
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 11

2 years ago
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.
Attachment #8713106 - Flags: approval-mozilla-aurora?
Comment on attachment 8713106 [details] [diff] [review]
0001-fix-exception-webextension-iframe-destroy.patch

Fix for recent regression, ok to uplift to aurora.
Attachment #8713106 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking for 46+
status-firefox45: --- → unaffected
status-firefox46: --- → affected
tracking-firefox46: --- → +
tracking-firefox47: --- → +
Keywords: regression

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/0feeda85b3bc
status-firefox46: affected → fixed
Blocks: 1214658
Version: unspecified → 46 Branch
You need to log in before you can comment on or make changes to this bug.