949297 - Errors when unloading registered widgets that reside in toolbars

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

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

Here is the simplified test case that can be run in the scratchpad with browser
context: https://gist.github.com/Gozala/7923065

Running it causes error to be logged in browser console:
[CustomizableUI]" "Widget not found, unable to remove" CustomizableUI.jsm:149


Calling `load()` again seems to work fine in this case, but I'm afraid it does not
in an actual code which is more complicated one, don't actually know why.

Comment 1

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

I figured out why my widget was not placed back to the toolbar, that's because widget ID in my case
is generated each time which I guess confuses CUI. Here is an adjusted test case that illustrates that
https://gist.github.com/Gozala/7923403

First load is fine, unload logs error reported earlier, load again no longer places widget in the toolbar
but rather into pallet, which I don't think is an expected behavior.

Comment 2

[:Gijs (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #1)
> I figured out why my widget was not placed back to the toolbar, that's
> because widget ID in my case
> is generated each time which I guess confuses CUI. Here is an adjusted test
> case that illustrates that
> https://gist.github.com/Gozala/7923403
> 
> First load is fine, unload logs error reported earlier, load again no longer
> places widget in the toolbar
> but rather into pallet, which I don't think is an expected behavior.

IDs are basically like DOM IDs but global across multiple Firefox windows and even sessions. They should not change.

I suspect this is a dupe of bug 948985, which seems to also have been mentioned on IRC.

Comment 3

[:Gijs (he/him)]

Can you still reproduce this now that bug 948985 is fixed?

Comment 4

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

Yes issues is still reproducible with a code snippet attached on the latest nightly. I would also
encourage incorporating that snippet into CUI tests.

Comment 5

[:Gijs (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #0)
> Here is the simplified test case that can be run in the scratchpad with
> browser
> context: https://gist.github.com/Gozala/7923065
> 
> Running it causes error to be logged in browser console:
> [CustomizableUI]" "Widget not found, unable to remove" CustomizableUI.jsm:149

So I looked at the testcase. I don't understand what the issue is here? The error is Cu.reported, there's no actual exception involved, and no code breaks because of this - it's meant to be an informative message: you tried to remove something, and that something no longer existed. It happens because you destroyWidget'd the widget and then the area is destroyed and so its items are removed. The physical items can't be removed because you destroyWidget'd them first, so they're gone. We could try and catch this case and not log a message, but that seems like a waste of effort that might hide legitimate problematic cases.

Comment 6

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

I believe it's logged as error and AMO reviewers have pretty strict policies in regards to errors logged
in the console. Maybe changing phrasing and loglevel of the message would be good enough fix for this.

Please note that problem is there's no other way to destroy both widget and toolbar right now without
causing this error to be logged, which makes it makes it pretty unpleasant API to use.

Another option would be to allow `CUI.unregisterArea(id, {ignoreMissing: true})` that would swallow those
warnings (which won't be a default).

Comment 7

[:Gijs (he/him)]

(In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment #6)
> I believe it's logged as error and AMO reviewers have pretty strict policies
> in regards to errors logged
> in the console. Maybe changing phrasing and loglevel of the message would be
> good enough fix for this.
> 
> Please note that problem is there's no other way to destroy both widget and
> toolbar right now without
> causing this error to be logged

CUI.removeWidgetFromArea(widgetId);
CUI.destroyWidget(widgetId);
CUI.unregisterArea(toolbarId);

Note that then the placement of the widget isn't saved. Does that matter for your usecase?

Comment 8

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #6)
> > I believe it's logged as error and AMO reviewers have pretty strict policies
> > in regards to errors logged
> > in the console. Maybe changing phrasing and loglevel of the message would be
> > good enough fix for this.
> > 
> > Please note that problem is there's no other way to destroy both widget and
> > toolbar right now without
> > causing this error to be logged
> 
> CUI.removeWidgetFromArea(widgetId);
> CUI.destroyWidget(widgetId);
> CUI.unregisterArea(toolbarId);
> 
> Note that then the placement of the widget isn't saved. Does that matter for
> your usecase?

Comment 9

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

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Irakli Gozalishvili [:irakli] [:gozala] [@gozala] from comment
> #6)
> > I believe it's logged as error and AMO reviewers have pretty strict policies
> > in regards to errors logged
> > in the console. Maybe changing phrasing and loglevel of the message would be
> > good enough fix for this.
> > 
> > Please note that problem is there's no other way to destroy both widget and
> > toolbar right now without
> > causing this error to be logged
> 
> CUI.removeWidgetFromArea(widgetId);
> CUI.destroyWidget(widgetId);
> CUI.unregisterArea(toolbarId);
> 
> Note that then the placement of the widget isn't saved. Does that matter for
> your usecase?

That is exactly why we don't `CUI.removeWidgetFromArea(widgetId)`

Comment 10

[:Gijs (he/him)]

Attachment: adjust warning message severity for Australis customizableUI widget removals,

Comment 11

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/integration/fx-team/rev/42218965c4ae

Comment 12

[:Gijs (he/him)]

Comment on attachment 8376206 [details] [diff] [review]
adjust warning message severity for Australis customizableUI widget removals,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: none, but add-on author/AMO reviewer impact of spurious log messages in some cases
Testing completed (on m-c, etc.): local
Risk to taking this patch (and alternatives if risky): none
String or IDL/UUID changes made by this patch: none

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/42218965c4ae

Comment 14

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/344371c3b37e

Comment 15

[Andrei Vaida [:avaida]]

I was able to confirm the fix for this issue on the latest Nightly (Build ID: 20140310030201) and latest Aurora (Build ID: 20140310004003), using reporter's testcase under:
- Windows 7 64-bit [1],
- Mac OS X 10.9.1 [2],
- Ubuntu 13.10 64-bit [3].

1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
2. Mozilla/5.0 (Macintosh; Intel Mac OS X 10.9; rv:30.0) Gecko/20100101 Firefox/30.0
3. Mozilla/5.0 (X11; Linux x86_64; rv:30.0) Gecko/20100101 Firefox/30.0