Closed Bug 989609 Opened 6 years ago Closed 6 years ago

Customize mode can break after bootstrapped add-on with custom toolbar restarts

Categories

(Firefox :: Toolbars and Customization, defect)

29 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: quicksaver, Assigned: mconley)

References

(Blocks 1 open bug)

Details

(Whiteboard: [Australis:P3-])

Attachments

(1 file, 6 obsolete files)

I'm sorry about all the direct CC'ing, but unless I'm missing something very basic or misinterpreting the whole thing completely, Australis is right around the corner and this breaks UX for every user, when a bootstrapped add-on with custom toolbars updates itself (not talking about SDK although I haven't tested it so it could be affected as well).

STR:

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: false }
> );
> 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
> [...]

At this point, we can't do almost anything in customize mode and can't even exit it. Only a browser restart will get things back working again.

Slight variation of this same problem: if in step 3 you also add a widget to the nav-bar, so that it's not in its default state, step 7 won't throw an error and customize mode won't break completely, the palette will populate correctly, !!-> except that the widgets in the testBar will be moved to the palette !! (I'm almost sure this is the same problem, but let me know if you think otherwise, and I'll open another bug for it.)

In both cases, after a browser restart (and re-doing step 2 to add back the testBar of course) everything will be fine, the widgets will be back in the testBar and customize mode will work fine. Do steps 4-onwards again and everything will break in the same way as before.

PS: registering the area with legacy:true changes nothing in this regard.
I'll look at this today. Prioritizing as a P3- until I figure out how bad this is.
Assignee: nobody → mconley
Whiteboard: [Australis:P3-]
Can you tell me exactly what items you're moving into the testBar to reproduce this bug?
Flags: needinfo?(quicksaver)
It can be any widgets (so far that hasn't really mattered, at least I didn't notice any difference about what widgets I move).

I just tried moving 'open-file-button' and 'characterencoding-button' to the testBar, so at least those I can confirm reproduce the bug. And I also moved 'tabview-button' to the end of nav-bar for the variation I mentioned.
Flags: needinfo?(quicksaver)
Reproduced, and I have a minimal test case. I'll post that shortly, and try to find a fix next.
Visiting family, so I'm only getting to look at this off and on, but I think I've figured out the problem - but not necessarily the solution.

The problem is that when the "testBar" is re-added, the currentArea for the API-created widgets that are put into it are not updated, so they're still null. That means that when the palette is populated, it goes through all of the API-create widgets, sees that those widgets have no currentArea, and then moves them into the palette.

But that's _after_ the items in the testBar have been wrapped. So now we have these empty wrappers in the testBar, and that causes things to break pretty badly while checking if we're in the default state, since the currentSet getter for the toolbars are not prepared to have children that are toolbarpaletteitems with no children.

So we need to find out why the currentArea is not being updated.
Comment on attachment 8399003 [details] [diff] [review]
Patch v1

Not 100% sure this is the right solution / right place to put this, but it solves the issue. Could use a sanity check. I'll let Blair and Jared race. :)
Attachment #8399003 - Flags: review?(jaws)
Attachment #8399003 - Flags: review?(bmcbride)
Comment on attachment 8399003 [details] [diff] [review]
Patch v1

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

Ah, so, the issue is that normally restoreStateForArea() normally does this by calling addWidgetToArea(). But in this case it doesn't because the state has already been restored, so restoreStateforArea() bails out early.

The issue with this patch is that it's inconsistent with what the normal path does - it's skipping all the normal checks, and skipping sending the onWidgetAdded event.

I can see two different ways to fix this:
* Make restoreStateForArea() bypass restoring state to gPlacements if that's already been restored, but still apply those placements via addWidgetToArea().
* Have unregisterArea() move gPlacements[area] to gFuturePlacements (would need to ensure the prefs are saved right after it did that).
Attachment #8399003 - Flags: review?(jaws)
Attachment #8399003 - Flags: review?(bmcbride)
Attachment #8399003 - Flags: review-
Attached patch Patch v2 (obsolete) — Splinter Review
So, something more like this? (I chose your second option - we also already force flushing the prefs here with endBatchUpdate(true)).
Attachment #8399003 - Attachment is obsolete: true
Attachment #8399514 - Flags: review?(bmcbride)
Comment on attachment 8399002 [details] [diff] [review]
Regression test v1

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

Tentative r=me, I will do some functional testing when the main patch is done.

::: browser/components/customizableui/test/browser_989609_bootstrapped_custom_toolbar.js
@@ +4,5 @@
> +
> +"use strict";
> +
> +const kTestBarID = "testBar";
> +const kWidgetID = "characterencoding-button";

This constant is not used in this file, you might want to remove it.

@@ +8,5 @@
> +const kWidgetID = "characterencoding-button";
> +
> +function createTestBar() {
> +  let testBar = document.createElement("toolbar");
> +  testBar.id = kTestBarID;

nit: you might want to use `setAttribute("id", kTestBarID);` to have only one type of setter used in this test.

@@ +11,5 @@
> +  let testBar = document.createElement("toolbar");
> +  testBar.id = kTestBarID;
> +  testBar.setAttribute("customizable", "true");
> +  CustomizableUI.registerArea(kTestBarID,
> +    { type: CustomizableUI.TYPE_TOOLBAR, legacy: false }

nit: unnecessary spaces in this object literal

@@ +71,5 @@
> +});
> +
> +add_task(function* () {
> +  yield checkRestoredPresence("characterencoding-button")
> +});
\ No newline at end of file

nit: missing newline at end of file - depends on your preferred style, I always do it...
Attachment #8399002 - Flags: review?(mdeboer) → review+
Comment on attachment 8399514 [details] [diff] [review]
Patch v2

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +372,4 @@
>        } else {
>          // Otherwise we need to re-set them, as removeFromArea will have emptied
>          // them out:
>          gPlacements.set(aName, placements);

Er, this doesn't actually work, does it?

If I'm understanding the state of things correctly, this needs to be putting it in gFutruePlacements *instead of* gPlacements, not both.
Attachment #8399514 - Flags: review?(bmcbride) → review-
(In reply to Blair McBride [:Unfocused] from comment #12)
> Comment on attachment 8399514 [details] [diff] [review]
> Patch v2
> 
> Review of attachment 8399514 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/customizableui/src/CustomizableUI.jsm
> @@ +372,4 @@
> >        } else {
> >          // Otherwise we need to re-set them, as removeFromArea will have emptied
> >          // them out:
> >          gPlacements.set(aName, placements);
> 
> Er, this doesn't actually work, does it?
> 
> If I'm understanding the state of things correctly, this needs to be putting
> it in gFutruePlacements *instead of* gPlacements, not both.

Clearly I've forgotten how all of this stuff works. I'll take a closer look today.

(But yes, this patch did work, and all tests passed).
Attached patch Patch v3 (obsolete) — Splinter Review
Alright, scrapping the unregisterArea approach - that would mean moving items out of unregistered areas would be impossible without deeper work that I think is too risky to land.

So here's the alternative that Blair suggested - on restoring state, we look to see if we have pre-existing placement information for that area. If so, we iterate those placements and use "moveWidgetWithinArea" one by one, positioned at each placement index. This is a noop DOM-wise since the positions are not changing - but I've moved the part that updates the currentArea and currentPosition to before we bail out.
Attachment #8399514 - Attachment is obsolete: true
Comment on attachment 8400285 [details] [diff] [review]
Patch v3

Is this more in tune with what you had in mind?
Attachment #8400285 - Flags: review?(bmcbride)
Comment on attachment 8400285 [details] [diff] [review]
Patch v3

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

Yep :) Minus one bug I think I found...

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1670,2 @@
>  
>        if (gSavedState && aArea in gSavedState.placements) {

Pretty sure we need to avoid taking this branch (and the case where we handle aLegacyState) if placementsPreexisted=true.

gSavedState is only ever set on startup - it's never updated. So the following situation should (in theory) currently result in the wrong behaviour:

1. add toolbar and register it
2. add widgets A,B,C,D
3. restart
4. add same toolbar and re-register it
5. remove a widget B from the toolbar
6. unregister toolbar and remove it
7. re-add same toolbar and re-register it

I believe this would result in the toolbar having widgets A,B,C,D - whereas it should have only A,C,D. If I'm right, then we're missing some test coverage for this.

And then, you could probably simplify the readability of this a little by moving your new block to be above this, and adding |!restored| to the condition here.
Attachment #8400285 - Flags: review?(bmcbride) → review-
Attached patch Patch v3.1 (obsolete) — Splinter Review
Unfortunately, mochitest doesn't have restart-test capabilities, and I'm not entirely sure I want to drag marionette into this right now. Maybe worthy of a follow-up, if you agree.

Anyhow, I manually confirmed your buggy scenario, and manually confirmed that this newest version of the patch fixes it.

Whaddya think?
Attachment #8400285 - Attachment is obsolete: true
Attachment #8400966 - Flags: review?(bmcbride)
Comment on attachment 8400966 [details] [diff] [review]
Patch v3.1

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

Oh, er, yea, point taken. I probably should have through that through a bit more, considering the whole "restart" part :)
Attachment #8400966 - Flags: review?(bmcbride) → review+
Attached patch Regression test v1.1 (obsolete) — Splinter Review
Ok, defects addressed, and the main patch is done! Feel like giving this a spin?
Attachment #8399002 - Attachment is obsolete: true
Attachment #8401057 - Flags: review?(mdeboer)
Comment on attachment 8401057 [details] [diff] [review]
Regression test v1.1

Well, without the patch applied, the test fails with `TypeError: realNode is null` and with the patch applied it runs a-ok.

So it's pretty obvious the whole thing works as expected! \o/

Ship it!
Attachment #8401057 - Flags: review?(mdeboer) → review+
remote:   https://hg.mozilla.org/integration/fx-team/rev/48e5925d8c81
Flags: in-testsuite+
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
Backed out for suspected introduction of a leak on bc1 for Linux Debug 64:

log: https://tbpl.mozilla.org/php/getParsedLog.php?id=37290990&tree=Fx-Team

remote:   https://hg.mozilla.org/integration/fx-team/rev/6a285c43e58d
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Depends on: 993084
No longer depends on: 993084
Depends on: 993084
No longer depends on: 993084
A try push of the same patch comes up green for me:

https://tbpl.mozilla.org/?tree=Try&rev=ec77fa9c8dcb

I'm calling shenanigans. Relanding.

remote:   https://hg.mozilla.org/integration/fx-team/rev/f047476811f7
Whiteboard: [Australis:P3-] → [Australis:P3-][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/f047476811f7
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3-][fixed-in-fx-team] → [Australis:P3-]
Target Milestone: --- → Firefox 31
This is what got committed.
Attachment #8400966 - Attachment is obsolete: true
Attachment #8401057 - Attachment is obsolete: true
Comment on attachment 8404709 [details] [diff] [review]
Final folded patch (r+'d by mikedeboer and Unfocused)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

Australis.


User impact if declined: 

Users who install add-ons that create custom toolbars can have customize mode break on them if they put widgets in that toolbar, and then disable and re-enable the add-on.


Testing completed (on m-c, etc.): 

Lots of local testing. Baked on m-c for a few days. Includes automated test.


Risk to taking this patch (and alternatives if risky): 

Low risk.


String or IDL/UUID changes made by this patch:

None.
Attachment #8404709 - Flags: approval-mozilla-beta?
Attachment #8404709 - Flags: approval-mozilla-aurora?
Attachment #8404709 - Attachment description: Final folded patch → Final folded patch (r+'d by mikedeboer and Unfocused)
Attachment #8404709 - Flags: review+
Attachment #8404709 - Flags: approval-mozilla-beta?
Attachment #8404709 - Flags: approval-mozilla-beta+
Attachment #8404709 - Flags: approval-mozilla-aurora?
Attachment #8404709 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.