996364 - Calling CustomizableUI.registerArea a second time for the same area will throw an exception

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Dave Townsend [:mossop]]

http://hg.mozilla.org/mozilla-central/annotate/265d82091bce/browser/components/customizableui/src/CustomizableUI.jsm#l314

The first time through defaultCollapsed is set to true then stored in gAreas. The second time that setting is retrieved from gAreas then we hit this if statement which makes us throw if aInternalCaller is false. You probably want to only throw if defaultCollapsed was passed in aProperties here.

Comment 1

[:Gijs (he/him)]

Yes.

Comment 2

[:Gijs (he/him)]

(I'm a little surprised this didn't get caught by automated testing... we should make sure we do write such a test now; it'll be pretty trivial)

Comment 3

[:Gijs (he/him)]

Jared, feel free to steal, but otherwise I will probably pick this up my morning.

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch

Comment 5

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8407050 [details] [diff] [review]
Patch

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

Drive by - this looks pretty good to me. Nice tests.

I'm curious to know - what happens if "overflowable" or "legacy", or any of the other registerArea object properties change from one registration to the other?

Comment 6

[:Gijs (he/him)]

Comment on attachment 8407050 [details] [diff] [review]
Patch

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

r=me, although mconley makes a good point. Up to you whether you want to take care of this here or in a followup.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/integration/fx-team/rev/b1470828166b

I'll file a separate bug for the things that mconley mentioned.

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8407050 [details] [diff] [review]
> Patch
> 
> Review of attachment 8407050 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me, although mconley makes a good point. Up to you whether you want to
> take care of this here or in a followup.

Filed bug 996899 for this.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b1470828166b

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8407050 [details] [diff] [review]
Patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 966599
User impact if declined: add-ons that call CustomizableUI.registerArea twice may behave unexpectedly
Testing completed (on m-c, etc.): locally, on m-c, and in automated tests
Risk to taking this patch (and alternatives if risky): none expected, may potentially fix addons that behave unexpectedly upon upgrade
String or IDL/UUID changes made by this patch: none

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8a0944daaca8