818432 - [toolbox] this. tracked browser windows.delete is undefined

Status: RESOLVED FIXED

(DevTools :: Framework, defect, P2)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We have temporarily moved it below "if (!this._tools)" but we should fix this properly.

Original Github PR:
https://github.com/joewalker/devtools-window/pull/257

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

We should find out why the destructor is called twice (or before initialization). We could also trap exceptions if necessary.

Comment 2

[Joe Walker [:jwalker]]

I think we should tackle this alongside memory leak issues. I favour getting rid of all the "delete this.****" statements in the destroy functions, but we need to make sure we clear up after ourselves in order to do that.

Comment 3

[Joe Walker [:jwalker]]

Attachment: v1

The approach that I've taken here is: sometimes preventing double-delete is hard. A good clean-up function can clean-up without crippling itself, and there's no point in gratuitously causing problems by deleting things you don't need to delete.

The change to Toolbox isn't directly connected, but it code that seems wrong to me. Comment added for feedback. Depending on what we think I could remove this section before commit.

Comment 4

[Paul Rouget [:paul]]

Comment on attachment 699645 [details] [diff] [review]
v1

>+        /*
>+        // this._currentToolId is set at the end of selectTool so unless
>+        // we're going to be calling open after selectTool (???) then it's
>+        // always going to be undefined, so we can just remove this code
>+        // especially given the new raise code ???

That was no-op? I did that :)

>@@ -217,8 +217,13 @@ DevTools.prototype = {
>   destroy: function() {
>     Services.obs.removeObserver(this.destroy, "quit-application");
> 
>-    delete this._trackedBrowserWindows;
>-    delete this._toolboxes;
>+    for (let [key, tool] of this._tools) {
>+      this.unregisterTool(key);

Will this trigger the "tool-unregistered" event, and do all the
related DOM modifications? We want to avoid that (performance).

Comment 5

[Joe Walker [:jwalker]]

Attachment: v2

Comment 6

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Try&rev=22d48402e458

Comment 7

[Joe Walker [:jwalker]]

https://tbpl.mozilla.org/?tree=Fx-Team&rev=fd6d84e5bf49
https://hg.mozilla.org/integration/fx-team/rev/346ee288166b

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/346ee288166b