Closed Bug 1191557 Opened 4 years ago Closed 4 years ago

Use "inner-window-destroyed" message for shutting down content sandbox in extension API

Categories

(WebExtensions :: Untriaged, defect)

34 Branch
defect
Not set

Tracking

(firefox45 fixed)

RESOLVED FIXED
mozilla45
Iteration:
45.1 - Nov 16
Tracking Status
firefox45 --- fixed

People

(Reporter: billm, Assigned: billm)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

The use of the dom-window-destroyed notification is pretty broken since we're passed the outer window.
Component: Extension Compatibility → WebExtensions
Product: Firefox → Toolkit
Iteration: --- → 44.2 - Oct 19
Iteration: 44.2 - Oct 19 → 44.3 - Nov 2
Flags: blocking-webextensions-
Attached patch patchSplinter Review
This just switches to the safer inner-window-destroyed notification. It's safer because we're getting a window ID rather than a window object that's in the process of being destroyed, which I've seen cause issues. I wasn't able to write a test for this unfortunately.
Attachment #8680932 - Flags: review?(kmaglione+bmo)
Comment on attachment 8680932 [details] [diff] [review]
patch

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

::: toolkit/components/extensions/ExtensionContent.jsm
@@ +311,5 @@
>        if (!mm || !ExtensionContent.globals.has(mm)) {
>          return;
>        }
>  
> +      this.windows.delete(windowId(window));

This doesn't seem right. Are we expecting to have an entry for the window at this point? I could see how that might be expected for the outer window, but not the inner window. Either way, it would probably be a good idea to log an error, since deleting it means contexts may not be closed properly.

@@ +405,5 @@
>  
>    shutdownExtension(extensionId) {
>      for (let global of ExtensionContent.globals.keys()) {
>        for (let [window, state] of this.enumerateWindows(global.docShell)) {
> +        let extensions = this.windows.get(windowId(window));

What about windows hidden in the BFCache? Now that |windows| is a Map rather than a WeakMap, we should probably just iterate over its entries.
Attachment #8680932 - Flags: review?(kmaglione+bmo) → review+
Iteration: 44.3 - Nov 2 → 45.1 - Nov 16
https://hg.mozilla.org/mozilla-central/rev/0bec4cffcbae
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.