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

RESOLVED FIXED in Firefox 21

Status

()

Firefox
Developer Tools: Framework
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: miker, Assigned: jwalker)

Tracking

unspecified
Firefox 21
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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
We should find out why the destructor is called twice (or before initialization). We could also trap exceptions if necessary.

Updated

5 years ago
Priority: -- → P2
Assignee: nobody → 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.
Depends on: 795988
Component: Developer Tools → Developer Tools: Framework
Created attachment 699645 [details] [diff] [review]
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.
Attachment #699645 - Flags: review?(paul)

Comment 4

5 years ago
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).
Created attachment 700355 [details] [diff] [review]
v2
Attachment #699645 - Attachment is obsolete: true
Attachment #699645 - Flags: review?(paul)
Attachment #700355 - Flags: review?(paul)

Updated

5 years ago
Attachment #700355 - Flags: review?(paul) → review+
https://hg.mozilla.org/mozilla-central/rev/346ee288166b
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 21
You need to log in before you can comment on or make changes to this bug.