973932 - [Australis] 'Restore defaults' button is enabled after every browser restart

Status: VERIFIED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Peter Retzer (:pretzer)]

STR:
1) Enter customization mode 
2) Push the 'Restore defaults' button and see that it is disabled
3) Restart Nightly
4) Enter customization mode 

Expected: The 'Restore defaults' button is  still disabled
Actual: The 'Restore defaults' button is enabled again even though all is still in default state.

Comment 1

[:Gijs (he/him)]

I can't reproduce this on an empty profile on OS X (and you marked this all/all, so...). Please provide more information (add-ons? platform? can you repro on an empty profile / safe mode?)

Comment 2

[Peter Retzer (:pretzer)]

(In reply to :Gijs Kruitbosch from comment #1)
> I can't reproduce this on an empty profile on OS X (and you marked this
> all/all, so...). Please provide more information (add-ons? platform? can you
> repro on an empty profile / safe mode?)

Sorry I'll try to do this better next time! ;-)
I can't reproduce in safe mode (holding Shift-key while starting), but I can still reproduce when I have disable all my addons by hand. I have 'Adblock Plus', 'Firefox OS 1.3 Simulator' and 'Citavi Picker' installed.
I can't reproduce with a clean profile... So maybe my profile is corrupt somehow?
Platform is Win7. Let me know if there is anything else I can do.

Comment 3

[:Gijs (he/him)]

(In reply to Peter Retzer (:pretzer) from comment #2)
> (In reply to :Gijs Kruitbosch from comment #1)
> > I can't reproduce this on an empty profile on OS X (and you marked this
> > all/all, so...). Please provide more information (add-ons? platform? can you
> > repro on an empty profile / safe mode?)
> 
> Sorry I'll try to do this better next time! ;-)
> I can't reproduce in safe mode (holding Shift-key while starting), but I can
> still reproduce when I have disable all my addons by hand. I have 'Adblock
> Plus', 'Firefox OS 1.3 Simulator' and 'Citavi Picker' installed.
> I can't reproduce with a clean profile... So maybe my profile is corrupt
> somehow?
> Platform is Win7. Let me know if there is anything else I can do.

Enable the browser.uiCustomization.debug pref in about:config, restart, and note what's causing the browser to not be in the default state.

Comment 4

[Peter Retzer (:pretzer)]

Attachment: browser_console_log.txt

I'm assuming you mean the browser console output, I put the whole log in the textfile. I think this is the part that causes the reset, I'm not sure though:

"[CustomizableUI]" "Checking default state for addon-bar:
addonbar-closebutton,wrapper-customizableui-special-spring1,status-bar
vs.
addonbar-closebutton,status-bar"
"[CustomizableUI]" "State reset"
"[CustomizableUI]" "Restoring PanelUI-contents from default state"

I thought the addonbar is gone for good?

Comment 5

[:Gijs (he/him)]

(In reply to Peter Retzer (:pretzer) from comment #4)
> Created attachment 8377650 [details]
> browser_console_log.txt
> 
> I'm assuming you mean the browser console output, I put the whole log in the
> textfile. I think this is the part that causes the reset, I'm not sure
> though:
> 
> "[CustomizableUI]" "Checking default state for addon-bar:
> addonbar-closebutton,wrapper-customizableui-special-spring1,status-bar
> vs.
> addonbar-closebutton,status-bar"
> "[CustomizableUI]" "State reset"
> "[CustomizableUI]" "Restoring PanelUI-contents from default state"
> 
> I thought the addonbar is gone for good?

There is a shim which automatically migrates items added there to the navbar or the palette, so that add-ons that have hardcoded references don't need to be updated. It is a complicated bit of code, and it probably is related to what you're seeing, but the log doesn't really provide the necessary info here.

Your suspicion about this being the relevant bit is correct in that the add-on bar isn't in its default state. What does the log say when you restart? Is there any mention of adding or removing the spring?

What confuses me is how that spring is getting re-added after you've reset and and then restarted the browser, with all your add-ons disabled. You can cause the "are we in the default state" check to happen manually by evaluating: "CustomizableUI.inDefaultState" (without quotes) in the browser console after the restart.

Comment 6

[:Gijs (he/him)]

I've been looking at code for reset/the addon bar, and I don't really understand the cause of this yet - does my last comment help in getting more details?

Comment 7

[Justin Dolske [:Dolske]]

Driveby thought: Can we ignore the addon-bar shim when checking to see if the UI is already in the default state? If it's non-default but you can't see it, seems like that shouldn't matter?

P4 since since the impact of having the Restore Defaults button enabled when it doesn't need to be is pretty low.

Comment 8

[Peter Retzer (:pretzer)]

Attachment: browser_console_log(after restart).txt

Sorry, it took me so long to respond! So I made a new log, that contains the output after a restart, where I restored the default state in the previous session.
As you can see, the addon-bar is restored from a legacy state, not the default state, even though it should be in default state.
"[CustomizableUI]" "Restoring addon-bar from legacy state"

I also checked that before restarting the browser the state was indeed 'default by using the 'CustomizableUI.inDefaultState' command you mentioned. And indeed it returned 'true' before the restart, 'false' afterwards...

Now, I don't know what's happening there. All Add-ons were disabled manually. Adblock Plus is the only thing I can remember ever having put anything in the addon-bar for me. Like I said I cannot reproduce in safe mode or with a clean profile... :-\
The only thing that seemed strange was that there was no "Saving state." log entry after restoring the default state, before restarting the browser. Could that be the problem?

Comment 9

[:Gijs (he/him)]

(In reply to Peter Retzer (:pretzer) from comment #8)
> Created attachment 8382313 [details]
> browser_console_log(after restart).txt
> 
> Sorry, it took me so long to respond! So I made a new log, that contains the
> output after a restart, where I restored the default state in the previous
> session.
> As you can see, the addon-bar is restored from a legacy state, not the
> default state, even though it should be in default state.
> "[CustomizableUI]" "Restoring addon-bar from legacy state"
> 
> I also checked that before restarting the browser the state was indeed
> 'default by using the 'CustomizableUI.inDefaultState' command you mentioned.
> And indeed it returned 'true' before the restart, 'false' afterwards...
> 
> Now, I don't know what's happening there. All Add-ons were disabled
> manually. Adblock Plus is the only thing I can remember ever having put
> anything in the addon-bar for me. Like I said I cannot reproduce in safe
> mode or with a clean profile... :-\
> The only thing that seemed strange was that there was no "Saving state." log
> entry after restoring the default state, before restarting the browser.
> Could that be the problem?

What does document.getElementById("addon-bar").getAttribute("currentset") return before the restart (but after the reset) ?

Comment 10

[Peter Retzer (:pretzer)]

"addonbar-closebutton,customizableui-special-spring1,status-bar"

Seems like it's not removed actually...

Comment 11

[:Gijs (he/him)]

(In reply to Justin Dolske [:Dolske] from comment #7)
> Driveby thought: Can we ignore the addon-bar shim when checking to see if
> the UI is already in the default state? If it's non-default but you can't
> see it, seems like that shouldn't matter?

So the reason I don't want to do this is because if add-ons manage to fudge it up somehow, at least the user has a chance of resetting things back to normal...

(In reply to Peter Retzer (:pretzer) from comment #10)
> "addonbar-closebutton,customizableui-special-spring1,status-bar"
> 
> Seems like it's not removed actually...

Nope, it's removed fine - it's just currentset which isn't updated which leads to all the grief here, because then after a restart it's used in lieu of the default placements. This would be a worse problem if you had actual add-on icons in there (shouldn't be possible, but even so...), because they'd also be stuck back in and then migrated out to the end of the toolbar or something...

Thanks for sticking with it when I kept just asking questions, and for bouncing the needinfo straight back to me so we got to the bottom of this. Now it took me only some minutes to (a) reproduce, (b) write an automated test, and (c) fix it. Patch coming up. :-)

Comment 12

[:Gijs (he/him)]

Attachment: CustomizeMode isn't resetting the currentset attribute on the addon-bar,

So persistCurrentSets in CustomizeMode.jsm relies on toolbars having a customizable='true' attribute. The add-on bar doesn't. This patch fixes that and adds a test to ensure we deal with this case properly.

Comment 13

[:Gijs (he/him)]

Egh. Accidentally landed as part of my patch queue:

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


and backed out:

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

Comment 14

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

Comment on attachment 8382985 [details] [diff] [review]
CustomizeMode isn't resetting the currentset attribute on the addon-bar,

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

Good stuff!

Comment 15

[:Gijs (he/him)]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

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

Comment 17

[:Gijs (he/him)]

Comment on attachment 8382985 [details] [diff] [review]
CustomizeMode isn't resetting the currentset attribute on the addon-bar,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis / addon-bar migration + currentset work
User impact if declined: odd 'restore defaults' button behaviour
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): low, single attribute change
String or IDL/UUID changes made by this patch: none

Comment 18

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/92d59571bdda

Comment 19

[Peter Retzer (:pretzer)]

I can verify that the problem is fixed now. Thanks!