938995 - [Australis] Customization Page Fails to Close

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Brendan Curran-Johnson]

Attachment: Missing Tool Options

Open the Customization page by selecting:
View > Toolbars > Customize...
Click on "Restore Defaults" until the tool icons fail to appear (this happened to me on a first click once, and is consistently reproducible after repeated clicks). Once this happens, closing the tab will not close the customization page. Other tabs that are opened will also show the customization page.

Comment 1

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

Yikes, that's not a great first experience. Thanks for reporting!

Comment 2

[Mike de Boer [:mikedeboer]]

This sounds like a JS error, which we should be able to see when we can reproduce it locally.

Comment 3

[:Gijs (he/him)]

Could you provide a list of the add-ons that were installed on this profile, and/or other, more detailed steps to reproduce, and/or check the browser console for any relevant errors?

Comment 4

[Brendan Curran-Johnson]

(In reply to :Gijs Kruitbosch from comment #3)
> Could you provide a list of the add-ons that were installed on this profile,
> and/or other, more detailed steps to reproduce, and/or check the browser
> console for any relevant errors?

I can only get the bug to manifest if I enable both  HTTPS-Everywhere 3.4.2 and Stylish 1.4.0 (no user styles are required). With both extensions are enabled, the "Restore Defaults" button is not always disabled when the UI is restored to a default state (The button always fails to disable when the customize page is loaded as part of a session restore. It sometimes disables when the page is loaded manually).

The problem that causes the page to incorrectly reload, and prevents the page from closing appears to be a race condition. It doesn’t occur consistently, though I can always reproduce it within 20 seconds by repeatedly clicking “Restore Defaults”. When it occurs, the following errors in CustomizeMode.jsm (at lines 49, and 552) are printed to the console:
[CustomizeMode]" "no toolbarItem child for toolbarpaletteitem#wrapper-open-file-button"
Argument 1 of Node.replaceChild is not an object.

I’m running “28.0a1 (2013-11-15)” on OS X 10.9. Let me know if you need any more details.

Comment 5

[Mike de Boer [:mikedeboer]]

Woah, that's very detailed info! Thanks! We'll try to verify this as soon as possible.

Comment 6

[:Gijs (he/him)]

(In reply to Brendan Curran-Johnson from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> > Could you provide a list of the add-ons that were installed on this profile,
> > and/or other, more detailed steps to reproduce, and/or check the browser
> > console for any relevant errors?
> 
> I can only get the bug to manifest if I enable both  HTTPS-Everywhere 3.4.2
> and Stylish 1.4.0 (no user styles are required). With both extensions are
> enabled, the "Restore Defaults" button is not always disabled when the UI is
> restored to a default state (The button always fails to disable when the
> customize page is loaded as part of a session restore. It sometimes disables
> when the page is loaded manually).
> 
> The problem that causes the page to incorrectly reload, and prevents the
> page from closing appears to be a race condition. It doesn’t occur
> consistently, though I can always reproduce it within 20 seconds by
> repeatedly clicking “Restore Defaults”. When it occurs, the following errors
> in CustomizeMode.jsm (at lines 49, and 552) are printed to the console:
> [CustomizeMode]" "no toolbarItem child for
> toolbarpaletteitem#wrapper-open-file-button"
> Argument 1 of Node.replaceChild is not an object.
> 
> I’m running “28.0a1 (2013-11-15)” on OS X 10.9. Let me know if you need any
> more details.

Huh, this is weird. As Mike said, these are very detailed steps, and I expected now I could reproduce, but I can't. Here's what I did:

0) using the nov. 16 OS X build on 10.9, and a clean profile
1) installed stylish from https://addons.mozilla.org/nl/firefox/addon/stylish/
2) install https everywhere from https://www.eff.org/https-everywhere
3) restarted
4) turned on the SSL observatory when prompted (probably not relevant, but it was a modal dialog so I had to choose...)
5) opened customize mode
6) close customize mode (worked)
7) reopen customize mode
8) click restore defaults (works, button becomes disabled)
9) close customize mode (worked)

Then I tried various sequences of restarts, restoring customize mode as part of session restore either indirectly (from about:home) or directly (after toggling the pref to always restore stuff), I tried moving the stylish and https everywhere icons back to the navbar and clicking restore defaults again with a restart in between, but it seems determined to keep working here. The restore defaults button also always becomes disabled after clicking it. :-\

The error makes me think something already moved the content of the wrapper for the open file button elsewhere, before closing customize mode. Unwrapping the button then just fails. We could catch that, but I'm worried about how this is even happening in the first place (it, err, shouldn't...). Are there no other relevant errors in the console?

I have to say, some of your steps make me think I'm missing something else. In principle, once you hit restore defaults and it *does* get disabled, how do you enable it again? Is it enabled when you've restarted? Because it's not here... (and it shouldn't be - the button should be disabled if you're already in the default state, which *should* be the case after you've pressed that button...). Are both the HTTPS everywhere and the stylish icon moved to the palette when you click 'restore defaults'?

Comment 7

[:Gijs (he/him)]

(In reply to Brendan Curran-Johnson from comment #7)
> Created attachment 832727 [details]
> Missing Tool Options
> 
> Open the Customization page by selecting:
> View > Toolbars > Customize...
> Click on "Restore Defaults" until the tool icons fail to appear (this
> happened to me on a first click once, and is consistently reproducible after
> repeated clicks). Once this happens, closing the tab will not close the
> customization page. Other tabs that are opened will also show the
> customization page.

Dumb question: were you clicking in quick succession? I wonder if we could fix this just by making the button disabled while a reset was in progress (a reset is non-blocking)

Comment 8

[:Gijs (he/him)]

OK, so here's what's going on, at least as far as I can tell against current trunk in a profile where I could reproduce this.

1) Add-on SDK addons used to add themselves to toolbars. This fix hasn't yet been uplifted to trunk. See bug 931092 (which is about something else, but I fixed this there).

2) If you've got any SDK add-on (such as HTTPS everywhere, IIRC) in the toolbar, it's going to have removable="false" in some circumstances, because the manual insertion code that used to live in the SDK (and is still on m-c) doesn't set this correctly. We used to set it to true in customize mode but bug 940946 fixed that.

3) non-removable items that aren't in the default placements of their container mean you can click the restore defaults button until kingdom come, they're not coming out of their container, and we'll never be in the default state. (bug!)

4) if you click restore defaults quickly in succession, because it's asynchronous, you might fire it twice in a row without the first having completed, which has rather disastrous consequences. (bug!)

So 2 is fixed, 1 is on its way to being fixed - we should still fix 3 and 4. I'll take care of that here.

Comment 9

[Brendan Curran-Johnson]

(In reply to :Gijs Kruitbosch from comment #7)
> (In reply to Brendan Curran-Johnson from comment #7)
> > Created attachment 832727 [details]
> > Missing Tool Options
> > 
> > Open the Customization page by selecting:
> > View > Toolbars > Customize...
> > Click on "Restore Defaults" until the tool icons fail to appear (this
> > happened to me on a first click once, and is consistently reproducible after
> > repeated clicks). Once this happens, closing the tab will not close the
> > customization page. Other tabs that are opened will also show the
> > customization page.
> 
> Dumb question: were you clicking in quick succession? I wonder if we could
> fix this just by making the button disabled while a reset was in progress (a
> reset is non-blocking)

Sorry for the slow reply.

When I orginally encountered the bug, I didn't think I had clicked the button more than once. I can't reproduce the error with slow clicks, so I'm going to guess that I must have double-clicked, and caused two events to fire.

Luckily, it seems you've already worked out what's going on.

Comment 10

[:Gijs (he/him)]

Attachment: Australis' customize mode sometimes doesn't close, restore defaults stays enabled after clicking,

Fix + test. Also, we should possibly investigate making CustomizableUI.reset be smarter. It takes a long time...

Comment 11

[:Gijs (he/him)]

(In reply to Brendan Curran-Johnson from comment #9)
> Sorry for the slow reply.
> 
> When I orginally encountered the bug, I didn't think I had clicked the
> button more than once. I can't reproduce the error with slow clicks, so I'm
> going to guess that I must have double-clicked, and caused two events to
> fire.
> 
> Luckily, it seems you've already worked out what's going on.

No worries, thank you for reporting the issue and also for confirming my suspicion! It could have been something else, and then I could 'fix' whatever I like but it wouldn't have helped you. Always good to be sure. :-)

Comment 12

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

Comment on attachment 8336200 [details] [diff] [review]
Australis' customize mode sometimes doesn't close, restore defaults stays enabled after clicking,

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

::: browser/components/customizableui/src/CustomizableUI.jsm
@@ +1922,5 @@
>        let buildAreaNodes = gBuildAreas.get(areaId);
>        if (buildAreaNodes && buildAreaNodes.size) {
>          let container = [...buildAreaNodes][0];
> +        let removableOrDefault = (itemNodeOrItem) => {
> +          let item = (itemNodeOrItem && itemNodeOrItem.id) || itemNodeOrItem;

I'm pretty sure this can just be written as:
let item = itemNodeOrItem && itemNodeOrItem.id;

Operator && will return the last truthy argument, so it should accomplish what you've got above with less characters.

::: browser/components/customizableui/src/CustomizeMode.jsm
@@ +607,5 @@
>    },
>  
>    reset: function() {
>      this.resetting = true;
> +    // Disable the reset button temporarily while customizing:

s/customizing/resetting/

Comment 13

[:Gijs (he/him)]

(In reply to Jared Wein [:jaws] from comment #12)
> I'm pretty sure this can just be written as:
> let item = itemNodeOrItem && itemNodeOrItem.id;
> 
> Operator && will return the last truthy argument, so it should accomplish
> what you've got above with less characters.

Erm, no?

15:03:18.427 < "foo" && "foo".id
15:03:18.430 > undefined

Makes sense - if you AND two truthy things, you get true, if you AND true and false, you get false, right?

Comment 14

[Brendan Curran-Johnson]

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Jared Wein [:jaws] from comment #12)
> > I'm pretty sure this can just be written as:
> > let item = itemNodeOrItem && itemNodeOrItem.id;
> > 
> > Operator && will return the last truthy argument, so it should accomplish
> > what you've got above with less characters.
> 
> Erm, no?
> 
> 15:03:18.427 < "foo" && "foo".id
> 15:03:18.430 > undefined
> 
> Makes sense - if you AND two truthy things, you get true, if you AND true
> and false, you get false, right?

You need the OR, the last evaluated value (regardless of truthiness) is returned:
> 3 && 5
5
> 3 || 5
3
> 3 && null
null
> null && 3
null

Comment 15

[:Gijs (he/him)]

Alright, shipped with the comment fixed:

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

Comment 16

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

(In reply to :Gijs Kruitbosch from comment #13)
> (In reply to Jared Wein [:jaws] from comment #12)
> > I'm pretty sure this can just be written as:
> > let item = itemNodeOrItem && itemNodeOrItem.id;
> > 
> > Operator && will return the last truthy argument, so it should accomplish
> > what you've got above with less characters.
> 
> Erm, no?
> 
> 15:03:18.427 < "foo" && "foo".id
> 15:03:18.430 > undefined
> 
> Makes sense - if you AND two truthy things, you get true, if you AND true
> and false, you get false, right?

let x = 3 && 5; // x == 5
let x = 5 && 3; // x == 3

With that being said, you can land as-is, I didn't think about the null dereference.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

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