STR: 1. open firefox on a clean profile (no sidebar open) 2. open a new window with accel-N 3. in the new window, open the bookmarks sidebar with accel-b 4. close the new window 5. open another new window with accel-N ER: sidebar button in the new window matches sidebar state (could argue for either open or closed, but closed (and button not checked) is probably the most sensible) AR: sidebar button is 'checked', but sidebar doesn't open. (I imagine this doesn't depend on how exactly you open new windows / sidebars, but I am keeping the specifics to make sure people can repro. I tested on OS X, but I expect this to happen everywhere.) This might also affect 56, I'm not sure and don't have time to check before PTO. :-( Brian, can you pick this up? If not, can you ping Mike or other structure folks to find an owner? Thanks, and sorry to drop this right before going on PTO, but I literally only noticed today (because I poked sidebars for bug 1406367) and figured it was better off being filed than forgotten...

I don't see this on 56

I guess this is because we are persisting the 'checked' attribute (which is used to show the toolbar button as toggled) in uninit(), but then the sidebar is not shown in adoptFromWindow. I guess we could either remove the checked attribute in adoptFromWindow when we find that the source window doesn't have a sidebar, or somehow not persist the checked state unless if this is the last window being closed.

I think this condition is meant to prevent this behavior from happening: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sidebar.js#71. But I just confirmed locally that when closing the new window the `!enumerator.hasMoreElements()` branch is entered and the persist functions get called.

Comment on attachment 8919385 [details] Bug 1407737 - Don't persist sidebar state unless if the last window is being closed. https://reviewboard.mozilla.org/r/190234/#review195902 Fantastic - I was hoping it'd go this smooth, since you've been wading through this code more than I did. Thanks! ::: browser/base/content/test/sidebar/browser_sidebar_adopt.js:16 (Diff revision 1) > > function failIfSidebarFocusedFires() { > ok(false, "This event shouldn't have fired"); > } > > add_task(async function() { nit: Please provide a unique name for your test function, because it helps whilst tracing logs (locally or on Treeherder) for failures.

Pushed by bgrinstead@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ff21cb09fe2f Don't persist sidebar state unless if the last window is being closed. r=mikedeboer

Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using Nightly 58.0a1 (64-bit) with today's build.

Do we want this fix in 57? If so please request uplift when you get a chance.

Comment on attachment 8919385 [details] Bug 1407737 - Don't persist sidebar state unless if the last window is being closed. Approval Request Comment [Feature/Bug causing the regression]: Bug 1372954 [User impact if declined]: Sidebar button gets in the wrong state after toggling the sidebar in a new window, closing the window, and then opening another window [Is this code covered by automated tests?]: Yes [Has the fix been verified in Nightly?]: Yes [Needs manual test from QE? If yes, steps to reproduce]: Yes, STR in Comment 0 [List of other uplifts needed for the feature/fix]: None [Is the change risky?]: Not too risky [Why is the change risky/not risky?]: This changes the condition when we save the sidebar state. The intention for was that we would only persist state when the last window was being closed, but this bug was caused because were getting false positives and persisting state when the second to last window was being closed as well. The risk is that we somehow accidentally stop persisting state at all, even when the last window is closed if indeed the call to `enumerator.getNext()` was needed in some cases. This issue is hard to cover in mochitests since it requires closing the browser and restarting it, but I tested this manually in development prior to landing. We could limit the risk with manual testing across all platforms to make sure that sidebar state is correctly persisted after restarting the browser. [String changes made/needed]:

Comment on attachment 8919385 [details] Bug 1407737 - Don't persist sidebar state unless if the last window is being closed. Recent regression, Beta57+

Verified on Windows 10 64bit, Mac OS X 10.13, Ubuntu 16.04 64bit using 57.0b11 (64-bit).