995161 - Customize mode can still break after bootstrapped add-on with custom legacy:true toolbar restarts

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Luís Miguel [:quicksaver] (not reading bugmail)]

Can still reproduce bug 989609 if registering the testBar as legacy:true, as in step 2:
> CustomizableUI.registerArea('testBar', 
>   { type: CustomizableUI.TYPE_TOOLBAR, legacy: true }
> );

Comment 1

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

STR cripped from bug 989609, and tweaked to use legacy: true.

1. Open clean firefox profile

2. Enter this bit in the browser console to create a normal customizable toolbar under the nav-bar:
> var testBar = document.createElement('toolbar');
> testBar.id = 'testBar';
> testBar.setAttribute('toolbarname', 'Test Bar');
> testBar.hidden = false;
> testBar.collapsed = false;
> testBar.setAttribute('class', 'toolbar-primary chromeclass-toolbar');
> testBar.setAttribute('context', 'toolbar-context-menu');
> testBar.setAttribute('customizable', 'true');
> CustomizableUI.registerArea('testBar', 
>   { type: CustomizableUI.TYPE_TOOLBAR, legacy: true }
> );
> gNavToolbox.appendChild(testBar);

3. Enter customize mode. Our testBar works perfectly fine, add a couple of widgets to it to compare afterwards.
3.1 Close customize mode.

4. Enter this bit in the browser console to clean and remove our testBar:
> CustomizableUI.unregisterArea('testBar');
> testBar.remove();
> testBar = null;

5. Enter customize mode. Everything seems fine, the widgets that were added to the testBar in step 3 went back to the palette as expected.
5.1 Close customize mode.

6. Re-do step 2 to re-add the testBar. Everything seems fine, the widgets moved in step 3 were automatically re-added to the testBar.

7. Enter customize mode. The testBar and the palette are now empty! This appears in the console:

> "[CustomizeMode]" TypeError: realNode is null
> Stack trace:
> get_currentSet@chrome://browser/content/customizableui/toolbar.xml:165:1
> CustomizableUIInternal.inDefaultState@resource://app/modules/CustomizableUI.jsm:2368:1
> this.CustomizableUI.inDefaultState@resource://app/modules/CustomizableUI.jsm:3146:5
> CustomizeMode.prototype._updateResetButton@resource://app/modules/CustomizeMode.jsm:1163:5
> CustomizeMode.prototype.enter/<@resource://app/modules/CustomizeMode.jsm:277:7
> [...]

Comment 2

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

I'll see what I can do about this.

Comment 3

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

Attachment: Patch v1

Seems to do the job, but I have no idea if this is the right choice. CustomizableUI is an extraordinary machine.

Blair / Gijs, I'll let you two race to tell me whether or not this is advisable, or hair-brained. :)

Comment 4

[:Gijs (he/him)]

Comment on attachment 8405556 [details] [diff] [review]
Patch v1

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

r=me

Comment 5

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

Thanks!

remote:   https://hg.mozilla.org/integration/fx-team/rev/93dceafcb1e6

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/93dceafcb1e6

Comment 7

[:Gijs (he/him)]

Comment on attachment 8405556 [details] [diff] [review]
Patch v1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: legacy toolbars and bootstrapped add-ons can cause customize mode to break
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): very low - small update from the other patch (for non-legacy toolbars) and has an automated test (which was updated for this change)
String or IDL/UUID changes made by this patch: none

Comment 8

[Mike de Boer [:mikedeboer]]

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

Beta: https://hg.mozilla.org/releases/mozilla-beta/rev/27221179c8b0

Comment 9

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Is testing for this fully covered automatically? Is there any need for manually testing this?

Comment 10

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

(In reply to Florin Mezei, QA [:FlorinMezei] from comment #9)
> Is testing for this fully covered automatically? Is there any need for
> manually testing this?

The automated test handles this, so I think we're all good. Thanks. :)