Got this in the error console when disabling my add-on. > Sat Nov 30 2013 11:31:24 > Warning: ReferenceError: reference to undefined property this.getWidgetNode(...)[0] > Source file: resource://app/modules/CustomizableUI.jsm > Line: 1921 I've finished changing the code to use the new CustomizableUI module in Australis. I've not made many tests on this (a bit of a lack of time). When disabling the add-on, I'm unregisterArea()'ing custom toolbar from it, physically removing them, and destroyWidget()'ing several custom buttons. The error only happened once, I can't reproduce it. However I'm not sure this is the fault of my own code, as it pointed to a strange place: (In isWidgetRemovable()) > if (!widgetNode) { > // Pick any of the build windows to look at. > let [window,] = [...gBuildWindows][0]; > [, widgetNode] = this.getWidgetNode(widgetId, window); > } Also, maybe bug 942581 will influence this as well, if it's the call in unregisterArea() to removeWidgetFromArea() that caused this. Any clues? Maybe throw in a failsafe in that bit would be enough? When it did error, everything seemed to continue well (everything from the add-on was removed properly with no trace) and I was able to re-enable the add-on without causing conflicts or further errors.

Did you destroyWidget() before unregisterArea or after, and did you do anything to the DOM node before then? :-)

Theoretically, either could have gone first, as I don't control the order in which things are de/initialized (a lot is done aSync'). Below, either a# or b# could have gone first, but inside them, the order (numbers) is always the same. If everything went as expected it was done in this order: a1) remove toolbar from the toolbox's externalToolbars array a2) unregisterArea() a3) physically remove the toolbar node b1) physically remove the toolbarbutton (widget) node b2) destroyWidget() (repeat b# for all buttons, 5 in total) I physically remove the node through its .removeChild() method before calling destroyWidget() because I need the node for something later to finish deinitialization, but saw that destroyWidget() only removes it if it finds the node in the document, and doesn't seem to need it for anything else. So I thought doing it shouldn't be a problem. Sorry not to be able to give a more concrete answer. Like I said, this only happened once. I've been keeping a close eye in the console but I haven't seen this message again since. Also, something else I just noticed (more of a curiosity). destroyWidget() only removes the node if it's found in the document tree; how does it get removed if it's in the palette? It definitely does this as the buttons my add-on inserts aren't in the customize screen anymore when I disable the add-on, but I can't find where in the code this is done.

(In reply to Luís Miguel [:Quicksaver] from comment #2) > Also, something else I just noticed (more of a curiosity). destroyWidget() > only removes the node if it's found in the document tree; how does it get > removed if it's in the palette? It definitely does this as the buttons my > add-on inserts aren't in the customize screen anymore when I disable the > add-on, but I can't find where in the code this is done. When the node is moved to the palette, we destroy it immediately. New-style widgets don't end up in gNavToolbox.palette at all, unless you're in customization mode (in which case it gets destroyed when you exit customize mode).

I suspect this just needs us to nullcheck the getWidgetNode call (or ensure that it at least always produces the array with a provider and node, not just null). I'll look at this later, although it'd be helpful if there were a minimal testcase of sorts. In principle you shouldn't need to ever touch the DOM nodes yourself other than to update their state to reflect changes (e.g. badges or altered icons or such). This also means I'm not sure why you're still doing anything special when calling unregisterArea. Why remove the nodes yourself?

(In reply to :Gijs Kruitbosch from comment #3) > When the node is moved to the palette, we destroy it immediately. New-style > widgets don't end up in gNavToolbox.palette at all, unless you're in > customization mode (in which case it gets destroyed when you exit customize > mode). This doesn't seem right. (Using DOM Inspector to monitor the palette) in the customization tab I have 8 widgets (all native, no add-on stuff anywhere), then after closing the customization tab, the palette still has the same 8 nodes corresponding to those widgets. Is this not supposed to happen, or am I misunderstanding what you mean? (PS. Shame on me, my own add-on buttons are removed from the palette on disabling because my code is doing that, physically removing them itself (read below for why). I have too much code to keep track of, sorry about this. My mistake, my confusion.) (In reply to :Gijs Kruitbosch from comment #4) > I suspect this just needs us to nullcheck the getWidgetNode call (or ensure > that it at least always produces the array with a provider and node, not > just null). I'll look at this later, although it'd be helpful if there were > a minimal testcase of sorts. Again, I'm sorry, I wasn't able to reproduce this since, and I haven't changed my code since either. Maybe it was a fluke of the moment thing (even though I hate that concept :) ). But a check would be fine I guess, at least it would prevent the whole routine from throwing at that point and keep going, although it was just a warning (not an error) so I don't believe it actually throws/stops anything there. > In principle you shouldn't need to ever touch the DOM nodes yourself other > than to update their state to reflect changes (e.g. badges or altered icons > or such). This also means I'm not sure why you're still doing anything > special when calling unregisterArea. Why remove the nodes yourself? I built a system that allows me to use xul overlays in a bootstrapped add-on, dynamically loading them when they're needed and removing all their traces afterwards when disabling/uninstalling the add-on. (Back-story, can skip if not interested: When I ported my old add-ons into a bootstrapped format, and saw that I had to keep track of every node element the add-on inserted, so I could safely remove it later, I got über-lazy. Instead of writing specific pieces of code for specific nodes that I used, I decided to just write a whole system where I could use whatever overlays I wanted without worrying about keeping track of everything individually.) Part of this system is keeping track of the toolbarbuttons; these nodes are automatically created when the overlay is loaded, and between having them "waiting" to be customized in the sandbox somewhere or in the palette, I thought why not have them in the palette already. Then on uninitialize, I also physically remove these, as a sort of fail-safe, to ensure the overlays are properly unloaded. In this specific case, I suppose I would need nothing else with the nodes themselves after calling destroyWidget on them, as they are destroyed immediately after this anyway. But between having the CustomizableUI module destroy them or having the add-on code do it, I'd rather have the fail-safe. In addition, there's a lot of custom attributes (especially a few on* attributes) in some of the buttons that I'd need to rewrite completely to fit into the new CustomizableUI system. Doing it this way helps with that, as the node used is the one that the add-on creates, so it keeps all those extra things that wouldn't be kept (directly) otherwise.

(In reply to Luís Miguel [:Quicksaver] from comment #5) > (In reply to :Gijs Kruitbosch from comment #3) > > When the node is moved to the palette, we destroy it immediately. New-style > > widgets don't end up in gNavToolbox.palette at all, unless you're in > > customization mode (in which case it gets destroyed when you exit customize > > mode). > > This doesn't seem right. (Using DOM Inspector to monitor the palette) in the > customization tab I have 8 widgets (all native, no add-on stuff anywhere), > then after closing the customization tab, the palette still has the same 8 > nodes corresponding to those widgets. Is this not supposed to happen, or am > I misunderstanding what you mean? I strongly suspect this is DOMI not updating, as the palette isn't even in the DOM anymore after closing customization mode. (gNavToolbox.palette isn't normally part of the DOM, so if you document.getElementById() nodes which are in there *and* you're not in customization mode, you will get null. Conversely, if you are in customization mode, you *won't* get null, so really, relying on that to tell you anything particularly useful is a bad idea. Note that just having customization mode open in some other, non-current tab === not having it open at all, for the purposes of this discussion)

(In reply to :Gijs Kruitbosch from comment #6) > (In reply to Luís Miguel [:Quicksaver] from comment #5) > > (In reply to :Gijs Kruitbosch from comment #3) > > > When the node is moved to the palette, we destroy it immediately. New-style > > > widgets don't end up in gNavToolbox.palette at all, unless you're in > > > customization mode (in which case it gets destroyed when you exit customize > > > mode). > > > > This doesn't seem right. (Using DOM Inspector to monitor the palette) in the > > customization tab I have 8 widgets (all native, no add-on stuff anywhere), > > then after closing the customization tab, the palette still has the same 8 > > nodes corresponding to those widgets. Is this not supposed to happen, or am > > I misunderstanding what you mean? > > I strongly suspect this is DOMI not updating, as the palette isn't even in > the DOM anymore after closing customization mode. (gNavToolbox.palette isn't > normally part of the DOM, so if you document.getElementById() nodes which > are in there *and* you're not in customization mode, you will get null. > Conversely, if you are in customization mode, you *won't* get null, so > really, relying on that to tell you anything particularly useful is a bad > idea. Note that just having customization mode open in some other, > non-current tab === not having it open at all, for the purposes of this > discussion) (to check whether things are in the palette post-customization mode, use something like this in the browser console: gNavToolbox.palette.querySelector('#feed-button') )

STR: 1) New profile. 2) Open browser console. 3) Enter that query: returns null as expected 4) Enter customize mode, feeds button is there 5) Close customize mode (actually close it, clicking the hamburger button) 6) Enter query in console again: returns [object XULElement] Apparently it is there, should I file a bug for this? (In reply to :Gijs Kruitbosch from comment #6) > (gNavToolbox.palette isn't > normally part of the DOM, so if you document.getElementById() nodes which > are in there *and* you're not in customization mode, you will get null. > Conversely, if you are in customization mode, you *won't* get null, so > really, relying on that to tell you anything particularly useful is a bad > idea. Note that just having customization mode open in some other, > non-current tab === not having it open at all, for the purposes of this > discussion) The palette may not be in the DOM, but it's still a property of gNavToolbox so it is never fully destroyed, so its children aren't either. When I said I was using DOMI to monitor the palette, I meant navigating to gNavToolbox.palette.childNodes (closing and reopening at all steps to ensure DOMI updates its content).

(In reply to Luís Miguel [:Quicksaver] from comment #8) > STR: > > 1) New profile. > 2) Open browser console. > 3) Enter that query: returns null as expected > 4) Enter customize mode, feeds button is there > 5) Close customize mode (actually close it, clicking the hamburger button) > 6) Enter query in console again: returns [object XULElement] > > Apparently it is there, should I file a bug for this? I should probably be less quick to assert things... there's commented out code regarding this in CustomizeMode.jsm with rationale as to why we're currently not doing this. So I stand corrected. We should then destroy it in destroyWidget, but it seems that code assumes what I previously asserted: it thinks it only needs to do anything if the node is in an area. Oops. Thank you for working through this with me! Working on a patch + test now (for both issues).

Patch + test

I think there should be an onDestroy method in widgets of type="custom", similar to the onBuild method. Since we're allowed to control widget node creation through the onBuild method, we should also be allowed the same regarding widget node destruction. I don't personally need this currently, as I just remove the node myself before calling destroyWidget, but I can see how this could be useful, at least in theory. So I thought I should mention it, even if just to keep the destruction routine consistent with the creation routine. Is this something that would be implemented in this patch or should I file a new bug for this?

(In reply to Luís Miguel [:Quicksaver] from comment #11) > I think there should be an onDestroy method in widgets of type="custom", > similar to the onBuild method. > > Since we're allowed to control widget node creation through the onBuild > method, we should also be allowed the same regarding widget node destruction. > > I don't personally need this currently, as I just remove the node myself > before calling destroyWidget, but I can see how this could be useful, at > least in theory. So I thought I should mention it, even if just to keep the > destruction routine consistent with the creation routine. Is this something > that would be implemented in this patch or should I file a new bug for this? Why would that be necessary, though? CUI.destroyWidget isn't ever called internally. So your add-on *knows* when it calls this, and can do the necessary cleanup beforehand?

(In reply to :Gijs Kruitbosch from comment #12) > Why would that be necessary, though? CUI.destroyWidget isn't ever called > internally. So your add-on *knows* when it calls this, and can do the > necessary cleanup beforehand? That's what I meant with "I don't need this currently" :) But it would definitely be necessary if the widget.destroyInstance(widgetNode) (which I'm guessing was a predecessor of CUI.destroyWidget) in CustomizeMode.jsm, that's been commented out, is ever written back in, cleaning out the palette when exiting customization mode. I'm not sure 'aftercustomization' events would be caught before the destroyWidget calls, or that listening for onWidgetAfterDOMChange events would be enough. Although that last alternative could definitely work with more or less custom routines, it would be much more indirect than an onDestroy method.

Comment on attachment 8341618 [details] [diff] [review] Australis' destroyWidget functionality should also remove the node if it's in the palette, Review of attachment 8341618 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/customizableui/src/CustomizableUI.jsm @@ +585,5 @@ > return [ CustomizableUI.PROVIDER_XUL, node ]; > } > > LOG("No node for " + aWidgetId + " found."); > + return [ null, null ]; nit, [null, null].

With nits fixed, remote: https://hg.mozilla.org/integration/fx-team/rev/3199da11173f