I now use 3 different tab addons for tab management, now that Tab Groups is going away: Tree Tabs, Tab Center Redux, and Sea Containers. All three live in the sidebar. I've noticed that the past few days, when I restart Nightly to update, the sidebar starts closed; then when I open it, it shows me Bookmarks. I think it would be useful to remember if the sidebar was open, and which view was open, and restore that at startup. OSX Nightly 56.0a1 (2017-07-29) (64-bit) https://hg.mozilla.org/mozilla-central/rev/2fba314d7de77ad8ab693a2ea0112c0cda5dd564

You wrote "the past few days" - did this use to work properly? Does it work properly if you have the bookmarks or history sidebar open when you exit Firefox? FWIW, it not working is a bug, not a missing feature - there is already code to preserve the last selected sidebar and reopen it on restart.

It looks like restarting with the bookmarks loaded in the sidebar gives me a sidebar on restart, and restarting with Tree Tabs in the sidebar gives me no sidebar. I think this is a regression from last week.

(In reply to Aki Sasaki [:aki] from comment #2) > It looks like restarting with the bookmarks loaded in the sidebar gives me a > sidebar on restart, and restarting with Tree Tabs in the sidebar gives me no > sidebar. I think this is a regression from last week. Can you try using mozregression to find out when exactly this broke?

That gave me 7:45.85 INFO: Last good revision: 07484bfdb96bc7297c404e377eea93f1d8ca4442 7:45.85 INFO: First bad revision: 32d9d1e81cc607320a36391845917f645f7a7f72 7:45.85 INFO: Pushlog: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=07484bfdb96bc7297c404e377eea93f1d8ca4442&tochange=32d9d1e81cc607320a36391845917f645f7a7f72 though I don't see an obvious culprit there.

(In reply to Aki Sasaki [:aki] from comment #4) > That gave me > > 7:45.85 INFO: Last good revision: 07484bfdb96bc7297c404e377eea93f1d8ca4442 > 7:45.85 INFO: First bad revision: 32d9d1e81cc607320a36391845917f645f7a7f72 > 7:45.85 INFO: Pushlog: > https://hg.mozilla.org/mozilla-central/ > pushloghtml?fromchange=07484bfdb96bc7297c404e377eea93f1d8ca4442&tochange=32d9 > d1e81cc607320a36391845917f645f7a7f72 > > though I don't see an obvious culprit there. Yeah, that doesn't look right... Brian, do you have ideas?

I've tested with Tab Center Redux with a clean profile using these steps: Install Tab Center Redux from about:addons Open the sidebar Close the browser Reopen the browser Sidebar is still opened Do you see this on a clean profile? And are there any errors in the Browser Console at startup?

I do see the sidebar disappear if I install a temporary addon from about:debugging (like https://github.com/mdn/webextensions-examples/tree/master/annotate-page), but I expect that since the addon isn't installed on the next startup.

I used `mozregression --good 2017-07-24 --bad 2017-07-29 --profile-persistence reuse -p p` so it was a new profile at first, but then reused. This was mainly to avoid having to reinstall the addon every time. I do see While creating services from category 'profile-after-change', service for entry 'Notification Telemetry Service', contract ID '@mozilla.org/notificationTelemetryService;1' does not implement nsIObserver. 1501543509864 addons.webextension.TreeTabs@jagiello.it WARN Loading extension 'TreeTabs@jagiello.it': Reading manifest: Error processing background.persistent: Event pages are not currently supported. This will run as a persistent background page. 1501543509875 addons.webextension.TreeTabs@jagiello.it WARN Loading extension 'TreeTabs@jagiello.it': Reading manifest: Error processing options_page: An unexpected property was found in the WebExtension manifest.

Hm, so I'm confused that this apparently doesn't reproduce reliably. (I haven't had time to try it myself.) Does the tab center redux or treetabs do something different with how it declares the sidebar? I just noticed https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/extensions/ext-sidebarAction.js#123-125 when looking at this code for another bug. This makes it seem like the code that's there now only reopens the sidebar on install and upgrade, not on regular restarts, and I imagine that the sidebar code itself that reopens sidebars might be running before the webextension in question has started up, or something? That is, maybe this is just a race, and that's why it doesn't reproduce reliably on different machines?

The sidebar relies on session restore to reopen the sidebar. Specifically, ext-sidebaraction has to insert certain elements prior to session restore. https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/extensions/ext-sidebarAction.js#

argh. fixed link. https://dxr.mozilla.org/mozilla-central/rev/52285ea5e54c73d3ed824544cef2ee3f195f05e6/browser/components/extensions/ext-sidebarAction.js#69

The sidebar UI gets initialized after SessionStore.promiseInitialized: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1747. And I assume we are hitting the `!commandID` condition here: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sidebar.js#216-220.

I reported duplicate bug 1384317 ("After restarting, Firefox (Nightly 56) does not restore sidebar view"), but now I'm not seeing the issue anymore, i.e., after a restart the sidebar is correctly showing the same extension as before.

+1, it's now working again for me in osx 57.0a1 (2017-08-14) (64-bit). Do we want to resolve?

(In reply to Aki Sasaki [:aki] from comment #15) > +1, it's now working again for me in osx 57.0a1 (2017-08-14) (64-bit). Do we > want to resolve? I'm kinda worried that this was a race condition before and now it's just happening to fewer users. It would help if we understood what fixed this. Can someone run mozregression with --find-fix to find out what unbroke this? :-)

5:07.29 INFO: First good revision: ffda55accd9ac78cd469c982afe1917cfeb6efb9 5:07.29 INFO: Last bad revision: 84a262bfe53ccf11e1b0bb86f9f6bfad760affeb 5:07.29 INFO: Pushlog: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=84a262bfe53ccf11e1b0bb86f9f6bfad760affeb&tochange=ffda55accd9ac78cd469c982afe1917cfeb6efb9

Just happened again, after letting Nightly update itself to 57.0a1 (2017-08-21) on Mac. Is there anything I should do to help, next time this happens?

(In reply to Brian Grinstead [:bgrins] from comment #13) > The sidebar UI gets initialized after SessionStore.promiseInitialized: > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#1747. And I assume we are hitting the `!commandID` condition here: > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser- > sidebar.js#216-220. Hm, but why would that happen? The sidebar command should be persisted fine, right? I rather think we take the else clause here: https://dxr.mozilla.org/mozilla-central/rev/a9d372645a32b8d23d44244f351639af9d73b96a/browser/base/content/browser-sidebar.js#222-229 which would mean _show is not called, the sidebarcommand attribute is removed, and the sidebar remains closed. This would happen if the add-on has not constructed its broadcaster/command (whatever has the ID listed in sidebarcommand) before the sidebar checks for it. Are manifest entries guaranteed to be processed (ie does 'onManifestEntry' actually fire) before session restore's promiseInitialized resolves? What about onReady - that is the code that runs through the windows that have *already* been opened by that time. More generally, it seems like the webextension code must be racy there anyway - we wait for onReady() to deal with windows opened prior to ready, but we attach the window opener listener before that, so the net result is that we could end up firing the window opener listener before onReady anyway, even though we wait with running that code against the currently-open windows until onReady. That seems like a bug (either we should never run the listener before onReady, or we should also run it for already-open windows prior to onReady), though not sure if fixing that would fix this bug... Shane, what would the correct solution to the latter issue be? Is that likely to help? Anyway, feels like we should maybe just update the sidebar code such that it is possible for the webextension code to determine, whenever it starts up, whether that add-on's sidebar was supposed to have been showing (and clearing that information as soon as the user loads another sidebar, and keeping track of open/close state separately, given bug 1391549 and bug 1391280).

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) Gecko/20100101 Firefox/57.0 I have tested this issue on Mac OSX with Nightly 56.a01 build from 2017-07-29 and managed to reproduce it, but it seems to be fixed on latest Nightly (Build ID: 20170821100350). I reproduced it using Tree Tabs following this steps: 1. Install Tree Tabs (https://addons.mozilla.org/en-US/firefox/addon/tree-tabs/) 2. Open multiple tabs 3. Restart the browser 4. Observe the sidebar Actual result: The browser restarted with the sidebar open I have used MozRegression tool using "--find-fix" command in order to find which bug fixed this. Here are the results: First good revision: 5a3309f0bb857e2edbfd72e92ce4334669a5569e Last bad revision: 08266a98cd9b5d6d1d071d9d5c31e1e4eabaf608 Pushlog: https://goo.gl/gaU8sM Afterwards, I also performed a regression in order to find which bug affected this. Here are the results: Last good revision: 495c6b6e45a784ee1fd39aab0a02eff3d526e1b0 First bad revision: 33154264e3d9e232ef0bab0cd271ba63f296775c Pushlog: https://goo.gl/YBmZRu Also, I have tested this issue on Windows using latest Firefox release (55.0.2) and latest Nightly build and could not reproduce it. @Gerald, did you reproduce this issue on latest Nightly build with Tree Tabs add-on installed? @Gijs, Can you please take a look over the provided regressions to see if they are related to the issue?

(In reply to Cosmin Muntean [:CosminMCG], Desktop Engineering QA from comment #20) > User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.12; rv:57.0) > Gecko/20100101 Firefox/57.0 > > I have tested this issue on Mac OSX with Nightly 56.a01 build from > 2017-07-29 and managed to reproduce it, but it seems to be fixed on latest > Nightly (Build ID: 20170821100350). > I reproduced it using Tree Tabs following this steps: > 1. Install Tree Tabs > (https://addons.mozilla.org/en-US/firefox/addon/tree-tabs/) > 2. Open multiple tabs > 3. Restart the browser > 4. Observe the sidebar > Actual result: The browser restarted with the sidebar open > > I have used MozRegression tool using "--find-fix" command in order to find > which bug fixed this. Here are the results: > First good revision: 5a3309f0bb857e2edbfd72e92ce4334669a5569e > Last bad revision: 08266a98cd9b5d6d1d071d9d5c31e1e4eabaf608 > Pushlog: https://goo.gl/gaU8sM > > Afterwards, I also performed a regression in order to find which bug > affected this. Here are the results: > Last good revision: 495c6b6e45a784ee1fd39aab0a02eff3d526e1b0 > First bad revision: 33154264e3d9e232ef0bab0cd271ba63f296775c > Pushlog: https://goo.gl/YBmZRu > > Also, I have tested this issue on Windows using latest Firefox release > (55.0.2) and latest Nightly build and could not reproduce it. > > @Gerald, did you reproduce this issue on latest Nightly build with Tree Tabs > add-on installed? > @Gijs, Can you please take a look over the provided regressions to see if > they are related to the issue? They don't look related to me, and they're different form the one in comment 17 which also wasn't related, which further cements my belief that this bug is intermittent / a race condition and therefore hard to reproduce reliably.

(In reply to :Gijs from comment #19) > (In reply to Brian Grinstead [:bgrins] from comment #13) > > The sidebar UI gets initialized after SessionStore.promiseInitialized: > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > > js#1747. And I assume we are hitting the `!commandID` condition here: > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser- > > sidebar.js#216-220. > > Hm, but why would that happen? The sidebar command should be persisted fine, > right? I rather think we take the else clause here: > > https://dxr.mozilla.org/mozilla-central/rev/ > a9d372645a32b8d23d44244f351639af9d73b96a/browser/base/content/browser- > sidebar.js#222-229 I concur. You can emulate the situation by using about:debugging to load a sidebar extension, then restart. On restart the extension of course is not loaded, but it creates a situation where the SidebarUI.startDelayedLoad is called prior to an extension inserting its elements. That is the essence of the race condition being hit here, and it is the else condition in 222-229. > Are manifest entries guaranteed to be processed (ie does 'onManifestEntry' > actually fire) before session restore's promiseInitialized resolves? What > about onReady - that is the code that runs through the windows that have > *already* been opened by that time. This I am not sure about. I believe it was, but there has been a lot of startup perf work done over the past couple months by kmag which may have had some affect here. It may also be that we need an event earlier than load: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/ext-tabs-base.js#1386 > More generally, it seems like the webextension code must be racy there > anyway - we wait for onReady() to deal with windows opened prior to ready, > but we attach the window opener listener before that, so the net result is > that we could end up firing the window opener listener before onReady > anyway, even though we wait with running that code against the > currently-open windows until onReady. That seems like a bug (either we > should never run the listener before onReady, or we should also run it for > already-open windows prior to onReady), though not sure if fixing that would > fix this bug... That is a confusing paragraph. onReady does a full build and shows the sidebar if necessary, but I dont think its necessary to be called prior to session restore. The window opener listener only attaches broadcaster/command/menu elements, which is what needs to be in place prior to SessionStore.promiseInitialized to hook everything up correctly. That is what I think is not happening intermittently. We may need to create a SidebarActions.init function similar to BrowserPageActions.init: https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#1360 https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-pageActions.js#46 All that would do is call sidebarAction.createMenuItem for each sidebar. This would guarantee the order of calls. This assumes that onManifestEntry is called earler.

(In reply to Shane Caraveo (:mixedpuppy) from comment #22) > (In reply to :Gijs from comment #19) > > Are manifest entries guaranteed to be processed (ie does 'onManifestEntry' > > actually fire) before session restore's promiseInitialized resolves? What > > about onReady - that is the code that runs through the windows that have > > *already* been opened by that time. > > This I am not sure about. I believe it was, but there has been a lot of > startup perf work done over the past couple months by kmag which may have > had some affect here. Hmm. Kris? > > More generally, it seems like the webextension code must be racy there > > anyway - we wait for onReady() to deal with windows opened prior to ready, > > but we attach the window opener listener before that, so the net result is > > that we could end up firing the window opener listener before onReady > > anyway, even though we wait with running that code against the > > currently-open windows until onReady. That seems like a bug (either we > > should never run the listener before onReady, or we should also run it for > > already-open windows prior to onReady), though not sure if fixing that would > > fix this bug... > > That is a confusing paragraph. Let me attempt to rephrase. When an add-on starts up, two things get called: onManifestEntry onReady the former hooks up updateWindow() for new windows. The latter hooks up updateWindow() for any windows that are already open when it gets called. I assume the the former gets called before the latter. What I don't understand is why onManifestEntry doesn't enumerate windows. Why do we wait for onReady? Why don't we just enumerate windows straight in onManifestEntry? If we did that, and *if* the bug here is that the sequence of events is: window open onManifestEntry side bar init, but webext sidebar isn't present yet: onReady Then this bug could be fixed by just moving the iteration into onManifestEntry, I think. Is that possible? > We may need to create a SidebarActions.init function similar to > BrowserPageActions.init: > > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser. > js#1360 > https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser- > pageActions.js#46 > > All that would do is call sidebarAction.createMenuItem for each sidebar. > This would guarantee the order of calls. This assumes that onManifestEntry > is called earler. Well, I considered this, but I have 2 questions about this approach: 1) how would the browser-sidebar.js code enumerate webextensions to load / have createMenuItem called for them? Can it delegate that somehow, by firing an observer notification or something? 2) this will only work if we're not already racing with webext initialization. It feels to me like as part of photon-perf we're pushing to do things later, to not block startup on having data for X or UI for Y, to create things as late as possible. As a result, it seems like it'd be a better pattern to have the webext sidebar "announce itself" once ready, and then just open it if it was open last time. What do you think? :-)

(In reply to Cosmin Muntean [:CosminMCG], Desktop Engineering QA from comment #20) > @Gerald, did you reproduce this issue on latest Nightly build with Tree Tabs > add-on installed? Yes, I've got Tree Tabs in the sidebar on the left side. As per comment 18, I saw the issue when updating to 57.0a1 (2017-08-21) about 26 hours ago. I've just updated again, to 57.0a1 (2017-08-22), but this time the sidebar correctly showed up with Tree Tabs. The last few times I've closed&reopened Firefox manually, I haven't seen the issue. Good luck!

I've seen this a couple times over the last week, but I've also seen it work a few times. I agree with the race condition theory.

I'm fairly certain that is all we need to do to ensure the session restore/sidebar startup work correctly.

Comment on attachment 8900032 [details] Bug 1385630 fix extension sidebar restore after app update, https://reviewboard.mozilla.org/r/171362/#review176528 I mean, wfm if this is sufficient. Thanks!

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/d159686d279c ensure elements are attached to all windows before onReady, r=Gijs

Please nominate this for Beta approval when you get a chance.

Still happening for me, when I updated Nightly from 2017-08-24 to 25, and just now 25 to 26.

This is still happening for me with 2017-08-28. The Tab sidebar doesn't reopen, when using Tab Center Redux. Not sure if we want to reopen this bug or start a new bug, but it appears to not be fixed at least in some cases.

I have managed to reproduce this issue using the latest Nightly (57.0a1-20170830220349) on Windows 10 x64 with Tab Center Redux and also with Firefox Notes. I have performed a regression, here are the results: Last good revision: 3aa4bc482372340ca890b1b0b5fd0937d21b725d First bad revision: b80466887686fbefe5a67aebc86f6f4aaf5feac8 Pushlog: https://goo.gl/zGTduu Looks like the following bug has the changes which introduced the regression: https://bugzilla.mozilla.org/show_bug.cgi?id=1384714 Kris can you take a look at this?

Should we back this change out given it regressed bug 1374048?

(In reply to :Gijs (queue backed up, slow) from comment #36) > Should we back this change out given it regressed bug 1374048? yes. Though I'm out till Tuesday, so feel free to do it if you want to. On related note. Yesterday I noticed sidebars no longer opened on install. I was tinkering and it started working and can no longer repro. More to the point, I was digging around history and noticed there were changes removing some attributes (bug 1391280) on some elements related to session restore, I wonder if that is related but didn't dig deeper.

Pushed by gijskruitbosch@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/1470c550ab6d Backed out changeset d159686d279c for regressing bug 1374048, rs=mixedpuppy

Backed out in remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/1470c550ab6d543cbeaeac953fba00df9cb69626

Merged backout: https://hg.mozilla.org/mozilla-central/rev/1470c550ab6d Gijs, can you resolve/reopen this bug and the other one as needed?

(In reply to Wes Kocher (:KWierso) from comment #40) > Merged backout: https://hg.mozilla.org/mozilla-central/rev/1470c550ab6d > > Gijs, can you resolve/reopen this bug and the other one as needed? This one is already open, the other one is still closed, so I think we're good, thanks for checking! :-)

Anecdotal experience: I tend to lose the sidebar when I restart due to a Nightly update. If I restore the sidebar, then quit and reopen without an update, I tend to keep the sidebar.

Quitting and restarting, without an update, also gives me the Containers test pilot icon back. I think the restart path used by updates is breaking both the sidebar and the Containers test pilot.

I wonder if an update restart does something different. On shutdown we leave UI in place so that it is handled by session restore for the next startup. http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-sidebarAction.js#102-106

I have a solid STR. - install an addon with a sidebar, make sure it is open - shutodwn firefox - delete compatibility.ini from the profile - start firefox expected: - sidebar is open to addon sidebar actual: - sidebar is closed This is caused because the startup cache is invalidated on an app update. In that scenario, extensions are not started until after SidebarUI.startDelayedLoad is already called, thus no chance to allow session restore to work.

thanks kmag for the clue on testing this.

Comment on attachment 8900032 [details] Bug 1385630 fix extension sidebar restore after app update, https://reviewboard.mozilla.org/r/171362/#review183048 Ahhhhh. It all makes so much sense now. Thanks! Even though this probably conflicts with my patch on bug 1391280. :-)

mstriemer, sorry for taking over, forgot this was passed on to you. Comments 43 & 44 joggled my brain on this.

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/8b53358a5d66 fix extension sidebar restore after app update, r=Gijs

(In reply to :Gijs from comment #49) > Ahhhhh. It all makes so much sense now. Thanks! Even though this probably > conflicts with my patch on bug 1391280. :-) Yeah, but it's a single line in there...

Comment on attachment 8900032 [details] Bug 1385630 fix extension sidebar restore after app update, Approval Request Comment [Feature/Bug causing the regression]: extension sidebar [User impact if declined]: sidebars are not persisted across browser update restarts [Is this code covered by automated tests?]: no [Has the fix been verified in Nightly?]: yes [Needs manual test from QE? If yes, steps to reproduce]: yes, str in comment 46 [List of other uplifts needed for the feature/fix]: no [Is the change risky?]: low [Why is the change risky/not risky?]: fix is relatively trivial [String changes made/needed]:

Andrei, can your team verify this in nightly? I'd like to get it into beta 12 later this week. Thanks.

I tested this in today's Nightly on Mac 10.12. Now I get my sidebar back (Tree Tabs). Huzzah!

Comment on attachment 8900032 [details] Bug 1385630 fix extension sidebar restore after app update, Looks like this fixed the issue in Nightly, let's take it for beta 12.

Could not reproduce the initial issue using 56.0a1 build from 2017-07-29 (when the bug was reported) on Windows 10 x64, macOS 10.12 and Mac OS X 10.9.5. I used the steps provided in comment 0 and comment 46. Aki, could you please verify if the bug is still reproducible on 56.0b12 build1 (20170914024831)?

This seems to work now, in general (yay!), and has for the last number of days. This morning I updated to last night's nightly and the sidebar disappeared. Then I got another update to 20170915100121, and the sidebar stayed. I think in general this is fixed. I'm not sure what happened in the 1st update I restarted for this morning.

I can confirm that it's also working for me. Thanks, triagers!

(In reply to Aki Sasaki [:aki] from comment #60) > This seems to work now, in general (yay!), and has for the last number of > days. > This morning I updated to last night's nightly and the sidebar disappeared. Are you saying it was open before the restart and then it closed? I'm a little worried that I overlooked an upgrade path when I fixed bug 1391280.

That's correct. The only thing I can think of is that there was an update ready to apply (staged update?) and another ready on Balrog.

Removing the qe-verify+ flag as there's nothing else QE can do here to help.

I haven't been able to replicate the missing sidebar again so far. I am able to consistently lose the containers addon icon on update restart; it consistently reappears on quit/reopen. Should I open another bug for that?

(In reply to Aki Sasaki [:aki] from comment #65) > I haven't been able to replicate the missing sidebar again so far. > I am able to consistently lose the containers addon icon on update restart; > it consistently reappears on quit/reopen. Should I open another bug for that? Yes please. Where is the icon? Can you run the following in the browser console ( https://developer.mozilla.org/en-US/docs/Tools/Browser_Console ): CustomizableUI.getWidgetIdsInArea( ); with one of the following in the (): - if the icon is in the main toolbar: "nav-bar" - if it's in the tabstrip: "TabsToolbar" - if it's in the bookmarks toolbar: "PersonalToolbar" - if it's in the menubar: "toolbar-menubar" I suspect this is the same issue as bug 1399517, but I'd like to be sure. Please leave a reply in the bug you end up filing. Thank you!

Done, bug 1401217.

(In reply to Aki Sasaki [:aki] from comment #63) > That's correct. The only thing I can think of is that there was an update > ready to apply (staged update?) and another ready on Balrog. I think there *is* a small leftover issue relating to updates & sidebars, unrelated to webextensions. I filed bug 1401232 for this. Gonna mark this verified per the other feedback here.