Closed Bug 895436 Opened 11 years ago Closed 10 years ago

SessionStore getNumberOfTabsClosedLast return wrong value after closing about:blank or about:newtab tabs

Categories

(Firefox :: Session Restore, defect)

Product:
Firefox
Component:
Session Restore
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED INVALID
Tracking Flags:
Tracking Status
firefox24 --- unaffected
firefox25 --- unaffected
firefox26 --- unaffected
firefox27 --- unaffected
firefox28 --- unaffected

People

(Reporter: tabmix.onemen, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(1 file, 2 obsolete files)

Patch v1.1
9.85 KB, patch
Details | Diff | Splinter Review 
After closing bunch of about:blank or about:newtab tabs ss.getNumberOfTabsClosedLast will always return the wrong number of tabs for undo!

since _shouldSaveTabState exclude about:blank and about:newtab we need to exclude such tabs when counting closed tabs.
we need to call ss.setNumberOfTabsClosedLast with the proper number
Component: Menus → Session Restore
Many extensions call gBrowser.removeTab to remove one or more tabs (also tabview.js do it)
we can't expect that all the extensions add ss.setNumberOfTabsClosedLast after the close operation finish.

This can lead to expecting results when user use undo closed tabs
(In reply to onemen.one from comment #1)
> Many extensions call gBrowser.removeTab to remove one or more tabs (also
> tabview.js do it)
> we can't expect that all the extensions add ss.setNumberOfTabsClosedLast
> after the close operation finish.

Is that still this bug? Sounds like it might be a separate issue?
(In reply to onemen.one from comment #1)
> Many extensions call gBrowser.removeTab to remove one or more tabs (also
> tabview.js do it)
> we can't expect that all the extensions add ss.setNumberOfTabsClosedLast
> after the close operation finish.
> 
> This can lead to expecting results when user use undo closed tabs

From tabbrowser.xml,
> <method name="removeTab">
>   <parameter name="aTab"/>
>   <parameter name="aParams"/>
>   <body>
>     <![CDATA[
>       if (aParams) {
>         var animate = aParams.animate;
>         var byMouse = aParams.byMouse;
>       }
>
>       let ss = Cc["@mozilla.org/browser/sessionstore;1"].
>                getService(Ci.nsISessionStore);
>       ss.setNumberOfTabsClosedLast(window, 1);

This was added in the patch for bug 887515. What are you recommending that I change?
Flags: needinfo?(onemen.one)
(In reply to onemen.one from comment #0)
> After closing bunch of about:blank or about:newtab tabs
> ss.getNumberOfTabsClosedLast will always return the wrong number of tabs for
> undo!
> 
> since _shouldSaveTabState exclude about:blank and about:newtab we need to
> exclude such tabs when counting closed tabs.
> we need to call ss.setNumberOfTabsClosedLast with the proper number

Can you please provide some steps to reproduce that show this problem? getNumberOfTabsClosedLast returns the minimum of (NumberOfTabsClosedLastPerWindow.get(aWindow) || 1) or  this.getClosedTabCount(aWindow).
(In reply to Jared Wein [:jaws] from comment #3)
> >
> >       let ss = Cc["@mozilla.org/browser/sessionstore;1"].
> >                getService(Ci.nsISessionStore);
> >       ss.setNumberOfTabsClosedLast(window, 1);
> 
> This was added in the patch for bug 887515. What are you recommending that I
> change?

Maybe new method to remove bunch of tabs
 - get tabs to remove in array
 - count only tabs that are not about:blank and about:newtab
 - remove the tabs
 - call setNumberOfTabsClosedLast

(In reply to Jared Wein [:jaws] from comment #4)
> (In reply to onemen.one from comment #0)
> > After closing bunch of about:blank or about:newtab tabs
> > ss.getNumberOfTabsClosedLast will always return the wrong number of tabs for
> > undo!
> > 
> > since _shouldSaveTabState exclude about:blank and about:newtab we need to
> > exclude such tabs when counting closed tabs.
> > we need to call ss.setNumberOfTabsClosedLast with the proper number
> 
> Can you please provide some steps to reproduce that show this problem?
> getNumberOfTabsClosedLast returns the minimum of
> (NumberOfTabsClosedLastPerWindow.get(aWindow) || 1) or 
> this.getClosedTabCount(aWindow).

- open few non blank tabs
- close all tabs, each tab separately, until you left with one about:newtab tab
- open few more about:newtab tabs
- use Close Other Tabs context menu

now if you use Ctrl-Shift-T to undo close tab it restore more then one tab
Flags: needinfo?(onemen.one)
(In reply to onemen.one from comment #5)
> (In reply to Jared Wein [:jaws] from comment #3)
> > >
> > >       let ss = Cc["@mozilla.org/browser/sessionstore;1"].
> > >                getService(Ci.nsISessionStore);
> > >       ss.setNumberOfTabsClosedLast(window, 1);
> > 
> > This was added in the patch for bug 887515. What are you recommending that I
> > change?
> 
> Maybe new method to remove bunch of tabs
>  - get tabs to remove in array
>  - count only tabs that are not about:blank and about:newtab
>  - remove the tabs
>  - call setNumberOfTabsClosedLast

This still isn't clear to me as to what is wrong with the implementation of removeTab.

> (In reply to Jared Wein [:jaws] from comment #4)
> > (In reply to onemen.one from comment #0)
> > > After closing bunch of about:blank or about:newtab tabs
> > > ss.getNumberOfTabsClosedLast will always return the wrong number of tabs for
> > > undo!
> > > 
> > > since _shouldSaveTabState exclude about:blank and about:newtab we need to
> > > exclude such tabs when counting closed tabs.
> > > we need to call ss.setNumberOfTabsClosedLast with the proper number
> > 
> > Can you please provide some steps to reproduce that show this problem?
> > getNumberOfTabsClosedLast returns the minimum of
> > (NumberOfTabsClosedLastPerWindow.get(aWindow) || 1) or 
> > this.getClosedTabCount(aWindow).
> 
> - open few non blank tabs
> - close all tabs, each tab separately, until you left with one about:newtab
> tab
> - open few more about:newtab tabs
> - use Close Other Tabs context menu
> 
> now if you use Ctrl-Shift-T to undo close tab it restore more then one tab

Thank you, I was able to reproduce this.
(In reply to Jared Wein [:jaws] from comment #6)
> > Maybe new method to remove bunch of tabs
> >  - get tabs to remove in array
> >  - count only tabs that are not about:blank and about:newtab
> >  - remove the tabs
> >  - call setNumberOfTabsClosedLast
> 
> This still isn't clear to me as to what is wrong with the implementation of
> removeTab.

I'm sorry, I wasn't clear enough.

The situation now is that if extension closed more than one tab without updating NumberOfTabsClosedLast, Ctrl-Shift+T will open only one tab.
User can be confuse, since not always they know if tabs was closed by Firefox or by extension.

Maybe adding new method for closing tabs as i proposed in comment 5 can simplify thing for extensions developers
(In reply to onemen.one from comment #7)
> (In reply to Jared Wein [:jaws] from comment #6)
> > > Maybe new method to remove bunch of tabs
> > >  - get tabs to remove in array
> > >  - count only tabs that are not about:blank and about:newtab
> > >  - remove the tabs
> > >  - call setNumberOfTabsClosedLast
> > 
> > This still isn't clear to me as to what is wrong with the implementation of
> > removeTab.
> 
> I'm sorry, I wasn't clear enough.
> 
> The situation now is that if extension closed more than one tab without
> updating NumberOfTabsClosedLast, Ctrl-Shift+T will open only one tab.
> User can be confuse, since not always they know if tabs was closed by
> Firefox or by extension.
> 
> Maybe adding new method for closing tabs as i proposed in comment 5 can
> simplify thing for extensions developers

OK, thank you for the clarification. This should be filed and discussed in a new bug. Please include mentions of any add-ons that you are aware of having this issue.
Status: NEW → ASSIGNED
Attached patch WIP Test (obsolete) — — Splinter Review 

        Attachment #790262 -
        Attachment description: Test → WIP Test

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #790262 -
        Attachment is obsolete: true

        Attachment #790340 -
        Flags: review?(dao)

        Attachment #790340 -
        Flags: review?(ttaubert)

    
    

    
  
Comment on attachment 790340 [details] [diff] [review]
Patch

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

I wonder if we should change the API to let SessionStore offer a undoCloseTabs() method. Or undoCloseLastBatchOfTabs(). Something like that. browser.js could then only call that if it wants to restore an unspecified number of tabs.

The advantage of that is that we don't need to expose the number of tabs closed last. If now we only could track the number of tabs closed last in SessionStore itself, that would be great. Every TabClose event could for example start counting and the counting would be interrupted by a any of the Tab* events handled in handleEvent() except another TabClose which would increase the counter again. A next tick should also interrupt the counter to not restore tabs in a batch that the user closed in succession.

What do you think about that suggestion?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +4149,5 @@
>    },
>  
> +  shouldSaveTab: function ssi_shouldSaveTab(aTab) {
> +    let tabState = this._collectTabData(aTab);
> +    return this._shouldSaveTabState(tabState);

We're currently working on getting rid of synchronous data collections so adding another one is certainly not what we want. Also I don't like exposing that kind of internal information should we someday decide to change that behavior. And we're currently thinking a lot about possibly changing how things work to move closer to our e10s goal.

        Attachment #790340 -
        Flags: review?(ttaubert)

    
    

    
  
(In reply to Tim Taubert [:ttaubert] from comment #11)
> Comment on attachment 790340 [details] [diff] [review]
> Patch
> 
> Review of attachment 790340 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I wonder if we should change the API to let SessionStore offer a
> undoCloseTabs() method. Or undoCloseLastBatchOfTabs(). Something like that.
> browser.js could then only call that if it wants to restore an unspecified
> number of tabs.
> 
> The advantage of that is that we don't need to expose the number of tabs
> closed last. If now we only could track the number of tabs closed last in
> SessionStore itself, that would be great. Every TabClose event could for
> example start counting and the counting would be interrupted by a any of the
> Tab* events handled in handleEvent() except another TabClose which would
> increase the counter again. A next tick should also interrupt the counter to
> not restore tabs in a batch that the user closed in succession.
> 
> What do you think about that suggestion?

I generally don't have an issue with introducing a new API, but this bug is tracking Fx25 so that doesn't seem like an acceptable solution for uplift, does it?
 
> ::: browser/components/sessionstore/src/SessionStore.jsm
> @@ +4149,5 @@
> >    },
> >  
> > +  shouldSaveTab: function ssi_shouldSaveTab(aTab) {
> > +    let tabState = this._collectTabData(aTab);
> > +    return this._shouldSaveTabState(tabState);
> 
> We're currently working on getting rid of synchronous data collections so
> adding another one is certainly not what we want. Also I don't like exposing
> that kind of internal information should we someday decide to change that
> behavior. And we're currently thinking a lot about possibly changing how
> things work to move closer to our e10s goal.

That stinks, I didn't realize this was an expensive synchronous operation.

    
    

    
  
(In reply to Jared Wein [:jaws] from comment #12)
> (In reply to Tim Taubert [:ttaubert] from comment #11)
>
> > ::: browser/components/sessionstore/src/SessionStore.jsm
> > @@ +4149,5 @@
> > >    },
> > >  
> > > +  shouldSaveTab: function ssi_shouldSaveTab(aTab) {
> > > +    let tabState = this._collectTabData(aTab);
> > > +    return this._shouldSaveTabState(tabState);
> > 
> > We're currently working on getting rid of synchronous data collections so
> > adding another one is certainly not what we want. Also I don't like exposing
> > that kind of internal information should we someday decide to change that
> > behavior. And we're currently thinking a lot about possibly changing how
> > things work to move closer to our e10s goal.
> 
> That stinks, I didn't realize this was an expensive synchronous operation.

Is it possible in shouldSaveTab:
  if tabData exist in TabStateCache - use it
  else get the data directly from browser.sessionHistory

  can we assume that all pending tabs and tabs with tabStillLoading are in TabStateCache?

    
  

        Attachment #790340 -
        Flags: review?(dao)

    
    

    
  
(In reply to Jared Wein [:jaws] from comment #12)
> I generally don't have an issue with introducing a new API, but this bug is
> tracking Fx25 so that doesn't seem like an acceptable solution for uplift,
> does it?

Can we just back it out from 25?

    
    

    
  
(In reply to Dão Gottwald [:dao] from comment #14)
> (In reply to Jared Wein [:jaws] from comment #12)
> > I generally don't have an issue with introducing a new API, but this bug is
> > tracking Fx25 so that doesn't seem like an acceptable solution for uplift,
> > does it?
> 
> Can we just back it out from 25?

Based on the number of outstanding regressions that seems like the best route. I'll file a bug and start working on the backout.

    
    

    
  
(In reply to Jared Wein [:jaws] from comment #15)
> (In reply to Dão Gottwald [:dao] from comment #14)
> > (In reply to Jared Wein [:jaws] from comment #12)
> > > I generally don't have an issue with introducing a new API, but this bug is
> > > tracking Fx25 so that doesn't seem like an acceptable solution for uplift,
> > > does it?
> > 
> > Can we just back it out from 25?
> 
> Based on the number of outstanding regressions that seems like the best
> route. I'll file a bug and start working on the backout.

Filed bug 914258.

    
    

    
  
Removing tracking from Firefox 25 since the patches that caused this regression have been backed out of Aurora 25 in bug 914258. Moved tracking to Firefox 26.

          status-firefox25:
          affected → unaffected

          status-firefox26:
          --- → affected

          tracking-firefox25:
          + → ---

          tracking-firefox26:
          --- → ?
Depends on: 931891

    
    

    
  
I've filed bug 931891 to back out the patches that caused this regression from Fx26 and Fx27. This is similar to what we did for bug 914258.

    
    

    
  
Removing tracking from Firefox 26 since the patches that caused this regression have been backed out of Beta 26 in bug 931891. The patches are also expected to be backed out of Fx27 soon, and this bug should get fixed on Fx28 while it is still on the Nightly train.

          status-firefox26:
          affected → unaffected

          status-firefox28:
          --- → affected

          tracking-firefox26:
          + → ---

          tracking-firefox28:
          --- → ?

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1.1
        — 
          — Splinter Review
      

    


  
(In reply to Tim Taubert [:ttaubert] from comment #11)
> Every TabClose event could for
> example start counting and the counting would be interrupted by a any of the
> Tab* events handled in handleEvent() except another TabClose which would
> increase the counter again.

This doesn't seem ideal. If the user did a batch closing operation and then switched tabs (as part of the closing operation or afterwards) we would receive a "TabSelect" event and wipe out the potential to undo the operation. Same goes for the other types of Tab* events. 

> > +  shouldSaveTab: function ssi_shouldSaveTab(aTab) {
> > +    let tabState = this._collectTabData(aTab);
> > +    return this._shouldSaveTabState(tabState);
> 
> We're currently working on getting rid of synchronous data collections so
> adding another one is certainly not what we want. Also I don't like exposing
> that kind of internal information should we someday decide to change that
> behavior. And we're currently thinking a lot about possibly changing how
> things work to move closer to our e10s goal.

I've used the approach described by onemen to not need to call this synchronous method.

        Attachment #790340 -
        Attachment is obsolete: true

        Attachment #8341299 -
        Flags: review?(ttaubert)

    
    

    
  
Comment on attachment 8341299 [details] [diff] [review]
Patch v1.1

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

Ok, here's another idea that might be easier and cleaner:

We already flush and collect tab data when a tab is removed. How about in onTabClose() we maintain a WeakMap of tabs whose tab states are pushed to _closedTabs like |undoCloseableTabs.set(tab, true)|. Now we can expose a method on SessionStore (not necessarily in the IDL) that allows to check whether we saved the state of a given tab - which is possible as long as something keeps a reference to it. This should ideally throw when a tab is passed with !tab.closing.

All the tabbrowser code would do is to call ss.insertGoodMethodNameHere(tab) to see whether the number of closed tabs needs to be increased.

I still don't particularly like that this feature exposes so much of how it works in sessionstore. I have two ideas of how we could clean it up:

(1) We could remove getNumberOfTabsClosedLast() and setNumberOfTabsClosedLast() and just use .get/setWindowValue() which is probably a better fit. At the same time this would have the advantage that we could read this value in the delayedStartup() routine and keep it around after restarts.

(2) Batch-closing tabs could emit an event like "BatchOfTabsClosed" and pass a list of tabs that were closed. SessionStore itself could then match the list against its internal WeakMap without exposing a method as suggested above and adjust its internal count of tabs closed last. The internal count should bet set to "1" in onTabClose(). To restore a batch of tabs we could now offer a method like ss.undoBatchOfClosedTabs() that restores up to "number of tabs closed last" tabs.

I actually like (2) more, is there any obstacles you see with this? Does that sound better to you than what we have currently? I just wanted to get this off my mind, this is probably stuff for a follow-up although converting to (2) would fix this bug as well.

::: browser/base/content/test/general/browser.ini
@@ +227,5 @@
>  [browser_bug839103.js]
>  [browser_bug880101.js]
>  [browser_bug882977.js]
>  [browser_bug887515.js]
> +[browser_bug895436.js]

Can you please give this a more descriptive name?

        Attachment #8341299 -
        Flags: review?(ttaubert)

    
    

    
  
No longer need to track this since the offending patch has been backed out (bug 931891).
Status: ASSIGNED → RESOLVED
Closed: 10 years ago

          status-firefox27:
          affected → unaffected

          status-firefox28:
          affected → unaffected

          tracking-firefox27:
          + → ---

          tracking-firefox28:
          + → ---
Resolution: --- → INVALID

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: