[Affected versions]: - Firefox 60.0a1 (20180208103049) - Firefox 59.0b7 (20180205211730) [Affected platforms]: - Win 7 64-bit - Mac OS X 10.13.2 [Steps to reproduce]: 1.Install https://addons.mozilla.org/en-US/firefox/addon/hide-tab/. 2.Open multiple tabs. 3.Hide the tabs. 4.Restart the browser. 5.Observe that the tabs are still hidden. 6.Remove the extension. [Expected results]: - The hidden tabs are restored. [Actual results]: - The hidden tabs are not restored. - If you do the steps without restarting the browser, you can see that the hidden tabs are displayed again. The both cases are included in the attached video.

UI is being worked on for that. Wait for it.

See also: 1408053

(In reply to brunoais from comment #1) > UI is being worked on for that. Wait for it. No, this is separate and it was meant to be handled in the initial implementation. I noticed this after a look at an earlier version of the hiding patches but then the review got redirected when I was on PTO :/

Updating the blocker so this is less likely to be overlooked

(In reply to Andrew Swan [:aswan] from comment #3) > (In reply to brunoais from comment #1) > > UI is being worked on for that. Wait for it. > > No, this is separate and it was meant to be handled in the initial > implementation. > I noticed this after a look at an earlier version of the hiding patches but > then the review got redirected when I was on PTO :/ Oh! Got it. My bad, then. (In reply to Andrew Swan [:aswan] from comment #4) > Updating the blocker so this is less likely to be overlooked Why not block both?

We'll have to decide what to do here...session data (which restores tabs into hidden state) doesn't track how the tab was hidden.

Ouch, I should have noticed that we were still missing this scenario. We could probably use SessionStore.setTabValue (https://searchfox.org/mozilla-central/source/browser/components/sessionstore/nsISessionStore.idl#189) to store the extension id of the extension which has hidden the tab in a way that ensures we can still retrieve it across Firefox restarts (and then use SessionStore.deleteTabValue to clear it when the tab has been unhidden, also if it is un-hidden by the user using the UI, instead of the the extension using the WE API).

The existing test that checks restored tabs after uninstall should be sufficient to cover the changes here.

(In reply to brunoais from comment #5) > Why not block both? Its fine by me if you want to add that back, I don't think anybody pays close attention to blockers on resolved bugs though.

(In reply to Andrew Swan [:aswan] from comment #10) > (In reply to brunoais from comment #5) > > Why not block both? > > Its fine by me if you want to add that back, I don't think anybody pays > close attention to blockers on resolved bugs though. I would but I don't have permissions to. The same way I would have added 1408053 to the "See also"

Comment on attachment 8949577 [details] Bug 1436720 use sessionstore to track controlling extension, https://reviewboard.mozilla.org/r/218948/#review224748 I think it'd be helpful to have a specific automated test for this. A simple way to test this working would presumably be to create a window with some tabs, close it, and then restore it via 'recently closed windows', then uninstall the add-on and verify that hidden tabs in the restored windows are re-shown. Separately, I seem to recall that we were considering giving add-ons access to these sessionstore values for tabs. Does that mean they could potentially access these values, or are all the add-on-accessible values namespaced in some way? I'm not sure how I'd feel about add-ons being able to mess with the internal state here. On the one hand, it would maybe help, on the other, it'd create scope for a whole host of separate bugs, so on balance I think I'd lean towards making sure that they aren't able to access these bits of metadata directly. ::: browser/base/content/tabbrowser.xml:3917 (Diff revision 1) > this.tabContainer._setPositionalAttributes(); > > let event = document.createEvent("Events"); > event.initEvent("TabHide", true, false); > aTab.dispatchEvent(event); > + SessionStore.setTabValue(aTab, "hiddenBy", aSource); We now only set this when the tab isn't already hidden. So in the situation: 1. hide tab with add-on A 2. attempt to hide same tab with add-on B 3. uninstall add-on A The tab would be unhidden. Is that something we care about?

(In reply to brunoais from comment #11) > The same way I would have added 1408053 to the "See also" I've added the other blocker, though as Andrew said I don't think it makes a difference at this point, given the bug has an active assignee and patch... I don't see the point of the see also - this bug has nothing to do with warning the user about implications of hidden tabs via separate bits of UI.

(In reply to :Gijs from comment #13) > (In reply to brunoais from comment #11) > > The same way I would have added 1408053 to the "See also" > I don't see the point of the see also - this bug has nothing to do with > warning the user about implications of hidden tabs via separate bits of UI. Fair enough.

So.. Just for the records, I wanted to add this also happens with respect to original Tab Groups extension (which ofc gets disabled after FF52) Which is *even* fine if you ask me, given it's way better for me to just downgrade (momentarily to backup, or not), or use an extension that can recognize the "different storing format", than just outright loosing tens of "tab divisions". Then, I'm not sure if whatever was being exploited by Tab Groups in the past (being after all some kind of continuation of panorama?) would apply even to these newer extensions.

(In reply to :Gijs from comment #12) > Comment on attachment 8949577 [details] > Bug 1436720 use sessionstore to track controlling extension, > > https://reviewboard.mozilla.org/r/218948/#review224748 > > I think it'd be helpful to have a specific automated test for this. A simple > way to test this working would presumably be to create a window with some > tabs, close it, and then restore it via 'recently closed windows', then > uninstall the add-on and verify that hidden tabs in the restored windows are > re-shown. I'll give that a spin. > Separately, I seem to recall that we were considering giving add-ons access > to these sessionstore values for tabs. Does that mean they could potentially > access these values, or are all the add-on-accessible values namespaced in > some way? Good question, I hadn't considered that. They are encoded per-extension using the extension id. https://searchfox.org/mozilla-central/source/browser/components/extensions/ext-sessions.js#60 > > + SessionStore.setTabValue(aTab, "hiddenBy", aSource); > > We now only set this when the tab isn't already hidden. So in the situation: > > 1. hide tab with add-on A > 2. attempt to hide same tab with add-on B > 3. uninstall add-on A > > The tab would be unhidden. Is that something we care about? I think this behavior is correct. Additionally, the tabs.hide api returns an array of tabs that were hidden by the extension, so that is the first point at which the extension knows.

Gijs, I had to make an additional change in SessionStore.jsm.

Comment on attachment 8949888 [details] Bug 1436720 use sessionstore to track controlling extension, https://reviewboard.mozilla.org/r/219202/#review225136 rs=me on the change in sessionstore.jsm (which makes sense to me) and the test (which looks great, thanks for writing that!). If you want a more thorough sessionstore-related review, I'd suggest mikedeboer. :-)

Comment on attachment 8949888 [details] Bug 1436720 use sessionstore to track controlling extension, https://reviewboard.mozilla.org/r/219202/#review225686 r=me on the WebExtensions part of the patch (follows some review comments related to just a couple of small nits in the test). ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:133 (Diff revision 1) > > + // Close the other window then restore it to test that the tabs are > + // restored with proper hidden state, and the correct extension id. > + await BrowserTestUtils.closeWindow(otherwin); > + > + // restored = TestUtils.topicObserved("sessionstore-closed-objects-changed"); Nit, I guess that this commented line is a left over. ::: browser/components/extensions/test/browser/browser_ext_tabs_hide.js:141 (Diff revision 1) > + let tabs = Array.from(otherwin.gBrowser.tabs.values()); > + ok(!tabs[0].hidden, "first tab not hidden"); > + for (let i = 1; i < tabs.length; i++) { > + ok(tabs[i].hidden, "tab hidden value is correct"); > + let id = SessionStore.getTabValue(tabs[i], "hiddenBy"); > + is(id, extension.id, "tab hiddenby value is correct"); Nit, hiddenby => hiddenBy

rb seems to have created a new attachment rather than adding a new change to the old one. The only changes in the last patch are the nits from comment 20. Currently looking at try issues.

Comment on attachment 8950782 [details] Bug 1436720 use sessionstore to track controlling extension, https://reviewboard.mozilla.org/r/220024/#review226130

Comment on attachment 8950782 [details] Bug 1436720 use sessionstore to track controlling extension, carry forward r+

Pushed by mixedpuppy@gmail.com: https://hg.mozilla.org/integration/autoland/rev/7d87cf855dea use sessionstore to track controlling extension, r=Gijs

Is there anything preventing backporting to current beta?

Some scenarios are still reproducing this issue: 1.The steps from description. 2. Hide some tabs -> restart the browser -> press the remove button and then refresh the about:addons page. (No hidden tabs are displayed) 3. Hide some tabs -> restart the browser -> press the remove button and close the about:addons page. (Only one tab is displayed) Tested on Firefox 60.0a1 (20180215095925) under Wind 7 64-bit.

Here is a video for the steps from description.

(In reply to CosminB from comment #28) > Some scenarios are still reproducing this issue: > > 1.The steps from description. > 2. Hide some tabs -> restart the browser -> press the remove button and then > refresh the about:addons page. > (No hidden tabs are displayed) > 3. Hide some tabs -> restart the browser -> press the remove button and > close the about:addons page. > (Only one tab is displayed) > > Tested on Firefox 60.0a1 (20180215095925) under Wind 7 64-bit. I'm unable to reproduce this, I am seeing what I expect. I also cannot fully see what you are pasting into the command shell to directly replicate how you are testing. If you shutdown the browser as a user would, do you still have the problem?

(In reply to brunoais from comment #27) > Is there anything preventing backporting to current beta? Because it is an experimental api right now I don't think it should be uplifted.

(In reply to Shane Caraveo (:mixedpuppy) from comment #31) > (In reply to brunoais from comment #27) > > Is there anything preventing backporting to current beta? > > Because it is an experimental api right now I don't think it should be > uplifted. I disagree but I accept your point of view. In my point of view, even when being experimental, a feature should be released at its (best effort) best state for the moment it is sent to public. This means, not delaying a release due to a bug to the feature but to a "good effort" to get the feature working without bugs. In this case, it means to uplift the bugfix (as long as it is not to a released version)

Uplifts to beta should be done with good reason. This is a feature that users have to pref-flip, and only affects them on removal of an extension using experimental apis. The fix includes a change to session restore which runs for all users. While the change a small and i don't see a way it would be a problem, I also don't see it as a necessary fix given the circumstance. OTOH, lets see what mconca thinks about it.

Here are the beta uplift criteria: https://wiki.mozilla.org/Release_Management/Uplift_rules#Beta_Uplift_.28approval-mozilla-beta.29 This doesn't appear to me to meet any of those

(In reply to Andrew Swan [:aswan] from comment #34) > Here are the beta uplift criteria: > https://wiki.mozilla.org/Release_Management/Uplift_rules#Beta_Uplift_. > 28approval-mozilla-beta.29 > > This doesn't appear to me to meet any of those "Must be landed on mozilla-central" It has "Ideally reproducible by QA so easily verified" It has been verified "Has 'baked' on m-c and demonstrated decrease in crash or reproducibility" Dunno the meaning of "baked" but it reduces the reproducibility "No string changes" It has string changes (for internal use only, though, not on user-side). If it fails on anything, it is in here. But it's neither a crash, regression or performance improvement ^ Is this what you mean, :aswan?

yes

This has been on my mind. The one reason it might be something to consider is that, given a user chooses to try tab hiding, then they remove it (after a restart), any hidden tabs will remain hidden. Since we don't have the UI in place yet to control that from Firefox, it's kind of PITA. However, it is still a pref'd-off experimental api. So I'm on the fence.

I'm on the fence as well but am leaning against a beta uplift. The API is pref'd off for a reason, MDN clearly indicates that it is an experimental API, and this doesn't strictly meet the standards that aswan pointed to for uplift. That said, we are actively encouraging developers to try this out and requesting feedback. If you get yourself into this situation with the current beta, it kind of sucks, and there is no easy way out. I'm going to send a NI to ddurst, since he'd likely be in a position of having to defend this uplift if we went that way.

In the browser console, I use the next snippet to restart the browser since the command Shift+F2 is not working 50% of the time: Components.classes["@mozilla.org/toolkit/app-startup;1"] .getService(Components.interfaces.nsIAppStartup).quit(Components.interfaces.nsIAppStartup.eRestart | Components.interfaces.nsIAppStartup.eAttemptQuit); I made two videos for the scenarios 2 and 3 that I mentioned in comment 28. I reproduce them on Firefox 60.0a1 (20180215095925) and Firefox 60.0a1 (20180215220507) under Wind 7 64-bit

After a chat with Luca, we finally discovered how the bug can still be reproduced. After the restart of the browser, if you don’t open the page_action popup, it will trigger the issue. Thank you Luca!

I took a look at the reasons why the issue is still reproducible if the page_action popup of the extension (the one used to verify this issue) has not been opened, and (as usually happen once you are finally able to reproduce an issue) the reason was pretty simple: The code that should show the hidden tabs is in the onShutdown lifecycle method of the tabs API class (https://searchfox.org/mozilla-central/rev/74b7ffee403c7ffd05b8b476c411cbf11d134eb9/browser/components/extensions/ext-tabs.js#84-101) and the onShutdown method can only be called if the tabs API has ever been instantiated. I didn't took a look at the sources of this extension yet, but I'm pretty sure that if the user doesn't open the popup (which lists the hidden tabs), or hide another tab from the contextMenu action added to the tab strip, the tabs API is never called and so it is had never been loaded yet.

Thanks Andrew, I briefly gave a try to define the static lifecycle methods, and double-check that their behavior cover the scenario that is still able to trigger the issue, and it also helped me to identify one more scenario that we have to be sure to cover: - when an extension which has the tabHide permission is updated to a new version which doesn't have a tabHide permission, we should show all the tabs previously hidden by the previous version of the extension The static lifecycle are definitely the way to go to fix this issue, but we should take into account that while the onShutdown lifecycle method is called when the extension is being disabled (and so when the user clicks the remove button, even if the extension has not been yet fully uninstalled, e.g. the user may still press the "undo" link and rollback the addon removal), the static onUninstall lifecycle method is being called only after the extension has been fully removed (and so once after the pending removal request is completed, e.g. when the user close or reload the about:addons tab). The following is a snippet that I applied locally as a quick experiment: ``` static onUninstall(id) { this.showHiddenTabs(id); } static onUpdate(id, manifest) { if (!manifest.permissions || !manifest.permissions.includes("tabHide")) { this.showHiddenTabs(id); } } static showHiddenTabs(id) { let windowsEnum = Services.wm.getEnumerator("navigator:browser"); while (windowsEnum.hasMoreElements()) { let win = windowsEnum.getNext(); if (win.closed || !win.gBrowser) { continue; } for (let tab of win.gBrowser.tabs) { if (tab.hidden && tab.ownerGlobal && SessionStore.getTabValue(tab, "hiddenBy") === id) { win.gBrowser.showTab(tab); } } } } ```

As much as I'm loathe to bend the rules for uplift, this is annoying and easier to do than any other mitigation (most of which won't appear until 61). Let's request uplift for 59 and buy ourselves some time (and karma) for the improved UX in 61.

(In reply to David Durst [:ddurst] from comment #44) > As much as I'm loathe to bend the rules for uplift, this is annoying and > easier to do than any other mitigation (most of which won't appear until 61). > > Let's request uplift for 59 and buy ourselves some time (and karma) for the > improved UX in 61. That doesn't seem to have happened?

see 1438985

Too late for 59 since we are a week away from the merge and release candidate build. I think these changes can wait for 60.

This issue is verified as fixed on Firefox 60.0a1 (20180304220118) under Windows 7 64-bit, Mac OS X 10.13.2. After I uninstall the extension, the hidden tabs are restored. Please see the attached video.