Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Description • 9 years ago

The use of the dom-window-destroyed notification is pretty broken since we're passed the outer window.

	Bill McCloskey [inactive unless it's an emergency] (:billm) Assignee
	Comment 1 • 9 years ago

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.

	Kris Maglione [:kmag]
	Comment 2 • 9 years ago

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.

	Pulsebot
	Comment 3 • 9 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/0bec4cffcbae

	Carsten Book [:Tomcat]
	Comment 4 • 9 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/0bec4cffcbae