871322 - Customize... isn't accessible on second time when Bookmarks button is removed

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Like it title. After backing out patches from bug #748894 in bug #867343 "Customize.." in toolbar context menu isn't accessible, because it's greyed out and browser window looks like in window mode, not like it should be in maximized state. It happens when you remove Bookmarks button from toolbar.

Comment 1

[Virtual_ManPL [:Virtual] 🇵🇱 - (please needinfo? me - so I will see your comment/reply/question/etc.)]

Also Error Console shows this error

Error: TypeError: this.button is null
Source File: chrome://browser/content/browser.js
Line: 2931

Comment 2

[Matthew Turnbull [Bluefang]]

This is also breaking things that use the "aftercustomization" event to rebuild themselves after toolbar customization (i.e. the download button panel).

Comment 3

[WildcatRay]

Steps to repeat:

1. Create new profile.
2. Open toolbar "Customize..." window (toolbar context menu).
3. Remove Bookmark toolbar button from toolbar.
4. Click "Done".
5. Attempt to open toolbar "Customize..." window.

Expected result:
The "Customize..." window opens.

Actual result:
"Customize..." is greyed out/disabled.

Also, "Toolbar layout..." is also disabled.

Closing/restarting the browser re-enables "Customize...".

Keeping the Bookmark button on the toolbar does not trigger the bug nor does it appear that other customizations trigger this bug.

Comment 4

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

It looks like we're attempting to manipulate this._button after it's been deleted in BookmarkingUI.customizeDone. Patch coming.

Comment 5

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

Attachment: Patch v1

The original bookmark star button did a check for this.star, and returned early if it didn't exist. Because we don't do that check anymore, we exposed ourselves to potentially using this._button *after* it'd been deleted.

Comment 6

[Matthew Turnbull [Bluefang]]

Wouldn't that still have a problem if the button was never on the toolbar? this._button would always be null in that case, and customizeChange/customizeDone are always called unchecked. Checking this.button in _updateToolbarStyle() seems like the safest route.

Also, IMHO, deleting this._button first in customizeDone makes sense to have a fresh reference to the button. I'd say the same should also go for customizeChange. If the button is removed, then immediately added back, is it the same element?

Comment 7

[Marco Bonardo [:mak]]

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

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

the reason to delete this._button is that the original element has been recreated, so the getter must acquire a new handle.
I think you should rather reintroduce an early return if (!this.button) in _updateToolbarStyle

Comment 8

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

Attachment: Patch v2

Yep, you're right, this is the right approach.

Comment 9

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

Pushed to mozilla-inbound as https://hg.mozilla.org/integration/mozilla-inbound/rev/249c9abe3e2b

Comment 10

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

Comment on attachment 748821 [details] [diff] [review]
Patch v2

[Approval Request Comment]

Bug caused by (feature/regressing bug #): Bug 867343

User impact if declined: If the user moves the bookmarks menu button from their toolbar into the palette, they will be unable to re-enter customization mode until a browser restart. If the user already has the bookmarks menu button in their palette, they will only be able to enter customization mode once until a browser restart.

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

Risk to taking this patch (and alternatives if risky): Extremely little risk.

String or IDL/UUID changes made by this patch: None.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/249c9abe3e2b

Comment 12

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/e46c4bcc41b4

Comment 13

[Bogdan Maris, Desktop Test Engineering]

Mozilla/5.0 (Windows NT 6.1; WOW64; rv:23.0) Gecko/20100101 Firefox/23.0

Verified as fixed on Firefox 23 beta 9 (buildID: 20130725195523) and latest Nightly (buildID: 20130725171558).

Comment 14

[Simona Badau, Desktop QA]

Verified as fixed on Firefox 24 beta 2:
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:24.0) Gecko/20100101 Firefox/24.0
Build ID: 20130812173056