660880 - WidgetViews are not destroyed on window close

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Alexandre Poirot [:ochameau]]

Attachment: Fix this and add a test

I think title is quite explicit.
WidgetView doesn't dispatch 'detach' event nor is destroyed when a browser window is closed.

Comment 2

[Myk Melez [:myk] [@mykmelez]]

How severe is the leakage here?

Comment 3

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 536333 [details] [diff] [review]
Fix this and add a test

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

Just a small readability issues.

::: packages/addon-kit/lib/widget.js
@@ +320,5 @@
>      this._views.splice(idx, 1);
>    },
>    
> +  // Called on browser window closed, to destroy related views
> +  _onWindowClosed: function _onWindowClosed(window) {

Please either comment why it should be backwards or use

`for (let element in array)`

which is easier to read.

@@ +322,5 @@
>    
> +  // Called on browser window closed, to destroy related views
> +  _onWindowClosed: function _onWindowClosed(window) {
> +    for (let i = this._views.length - 1; i >= 0; i--) {
> +      let view = this._views[i];

I assume you're trying to find a view associated for that window. In that case can you please factor that out into separate function `getWindowView(window, views)` or similar. That would be much easier to understand and maintain.

Comment 4

[Alexandre Poirot [:ochameau]]

Attachment: Use regular loop and add some comments.

Actually it is not justified in this case to go backward.
I badly copy paste loop code from a piece of code that has to go backward and where this behavior is commented.
So I just use regular loop and added some additional comment.

myk: I'm more concerned about bug and addon breakage on window close than the leak. The leak is not that big as document should be destroyed, so we may end up with unecessary SDK WidgetViews objects. But what is really wrong with this bug is that when a window is closed and addon code use widget.postMessage or widget.port.emit, it will throw an exception and avoid next widget on following windows to receive the event/message.

Comment 5

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 536616 [details] [diff] [review]
Use regular loop and add some comments.

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

Looks good, thanks! Make sure to get a+ as well ;)

Comment 6

[Myk Melez [:myk] [@mykmelez]]

Comment on attachment 536616 [details] [diff] [review]
Use regular loop and add some comments.

a=myk for landing during freeze.

Comment 7

[Alexandre Poirot [:ochameau]]

Landed:
https://github.com/mozilla/addon-sdk/commit/22687408a3f46b8a2facadc51e5304d830cf19ce