758138 - sessionStore starts observe browser.sessionstore.max_tabs_undo only after tab closed

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[tabmix.onemen]

Bug 542032 add lazy getter for _max_tabs_undo.

The lazy getter for _max_tabs_undo trigger only by onTabClose
So, until user closed the first tab in a session, any changes to browser.sessionstore.max_tabs_undo have no effect on closed tab list

Reproducible: Always

Steps to Reproduce:
1. start Firefox, make sure browser.sessionstore.max_tabs_undo > 0, if not set it and restart Firefox.
2. Open a bunch of tab, close few tabs.
3. Restart Firefox.
4. set browser.sessionstore.max_tabs_undo to 0 (zero)

Actual Results:  
History > Recently Closed Tabs list contained the tabs you closed before

Expected Results:  
Recently Closed Tabs list should be empty after you set browser.sessionstore.max_tabs_undo to 0 (zero)

Comment 1

[Michael Kohler [:mkohler]]

What would be the best solutions to fix this? I think this is one of the disadvantages of lazy getters.

Comment 2

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

Clearing by setting the pref was never really a supported way of clearing out your closed tabs, so I'm not sure what we should do here, if anything. It's possible (if a bit awkward) to do via the API. Maybe the better path would be to improve the sessionstore API to make this easier.

Comment 3

[tabmix.onemen]

Just move the addObserver out of the lazy getters

+ this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
  XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
-   this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
    return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
  });

+ this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
  XPCOMUtils.defineLazyGetter(this, "_max_windows_undo", function () {
-   this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
    return this._prefBranch.getIntPref("sessionstore.max_windows_undo");
  });

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

I think it would be best if we do it like for gDebbugingEnabled here:

http://hg.mozilla.org/mozilla-central/file/3d20597e0a07/browser/components/sessionstore/src/SessionStore.jsm#l523

Read and store the current pref value, and then register the observer.

Comment 5

[tabmix.onemen]

this link doesn't point to gDebbugingEnabled

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

It does for me...

Comment 7

[Amod [:AMoz]]

I would like to work on this bug. I have resumed to bugzilla after a very long time. So might take some time to grasp it. I would request you to help me getting started. Thanks !

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Hey Amod, that's great to hear. I gave a hint on how this can be done in comment #4. Should there be any questions feel free to ask here or on IRC.

Comment 9

[Amod [:AMoz]]

hello. when i observed the occurences here : http://mxr.mozilla.org/mozilla-central/search?string=%22sessionstore.max_tabs_undo%22&find=%2F&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central I found the code similar to that in link comment #4

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Sorry, I don't understand. Is that a question? Do you need more information from me? Please feel free to ask. Thanks!

Comment 11

[Amod [:AMoz]]

Attachment: patchv1

Still I am bit unclear with the concept. Forgive me in case of out of bounds code.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 811543 [details] [diff] [review]
patchv1

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

Thanks, Amod! I annotated your patch below:

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +537,5 @@
>        gDebuggingEnabled = this._prefBranch.getBoolPref("sessionstore.debug");
>      }, false);
>  
>      XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
> +      gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

We want to save the value of 'sessionstore.max_tabs_undo' to this._max_tabs_undo, like:

this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

@@ +538,5 @@
>      }, false);
>  
>      XPCOMUtils.defineLazyGetter(this, "_max_tabs_undo", function () {
> +      gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +      Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);

This line and the line above should be outside of the lazy getter. In fact, we want to remove the lazy getter. Also, we don't want to register 'this' as the observer but an anonymous function (like on line 536 above) that updates the value stored in this._max_tabs_undo.

Comment 13

[Amod [:AMoz]]

Attachment: patchv2

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 811546 [details] [diff] [review]
patchv2

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +530,5 @@
>  
>    _initPrefs : function() {
>      this._prefBranch = Services.prefs.getBranch("browser.");
>  
> +    this._max_tabs_undo = this._prefBranch.getBoolPref("sessionstore.debug");

No, this needs to stay gDebuggingEnabled.

@@ +541,2 @@
>        return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>      });

This is just leftover code and can be removed now.

@@ +541,3 @@
>        return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>      });
> +    gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

This line should be modifying this._max_tabs_undo.

@@ +541,4 @@
>        return this._prefBranch.getIntPref("sessionstore.max_tabs_undo");
>      });
> +    gDebuggingEnabled = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);

The 'this' in here is the observer argument. We need to pass an anonymous function that modifies this._max_tabs_undo. You can look at line 536-538 to see how that can be done.

Comment 15

[Amod [:AMoz]]

Attachment: patchv3

Explanation why i used this:

>  Services.prefs.addObserver("browser.sessionstore.max_tabs_undo", () => {
>   this._max_tabs_undo=this._prefBranch.getBoolPref("sessionstore.max_tabs_undo");
>    }, false);

The anonymous function should modify the value of  | this |, thats why i took the | this._max_tabs_undo | to the left side of the assignment. Rest i followed the same given in line 536-538

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 811552 [details] [diff] [review]
patchv3

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

Thanks Amod. This looks a lot better but we're not quite there yet :)

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +537,5 @@
>        gDebuggingEnabled = this._prefBranch.getBoolPref("sessionstore.debug");
>      }, false);
>  
> +
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");

This pref is an 'int'. You need to use getIntPref().

Did you try to run the test suite? Or just Firefox with your patch? The parenthesis don't match here.

@@ +538,5 @@
>      }, false);
>  
> +
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    //Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);

Nit: that commented out code should be removed

@@ +539,5 @@
>  
> +
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    //Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);
> +    

Nit: please remove the white space left on the line here

@@ +541,5 @@
> +    this._max_tabs_undo = this._prefBranch.getBoolPref(("sessionstore.max_tabs_undo");
> +    //Services.prefs.addObserver("sessionstore.max_tabs_undo", this, true);
> +    
> +    Services.prefs.addObserver("browser.sessionstore.max_tabs_undo", () => {
> +    this._max_tabs_undo = this._prefBranch.getBoolPref("sessionstore.max_tabs_undo");

This pref is an 'int'. You need to use getIntPref().

Nit: this line should be a tad more indented, i.e. two spaces

@@ +547,5 @@
>  
>      XPCOMUtils.defineLazyGetter(this, "_max_windows_undo", function () {
>        this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);
>        return this._prefBranch.getIntPref("sessionstore.max_windows_undo");
>      });

While we're at it, can you please do the same for _max_windows_undo?

Comment 17

[Amod [:AMoz]]

Attachment: patchv4

modified all the suggested changes.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 811966 [details] [diff] [review]
patchv4

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

Another thing I noticed: on line 1771, can you please change

  curWinState._closedTabs.splice(this._prefBranch.getIntPref("sessionstore.max_tabs_undo"), curWinState._closedTabs.length);

to:

  curWinState._closedTabs.splice(this._max_tabs_undo, curWinState._closedTabs.length);

Thanks!

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +544,5 @@
> +
> +    this._max_windows_undo = this._prefBranch.getIntPref("sessionstore.max_windows_undo");
> +    Services.prefs.addObserver("browser.sessionstore.max_windows_undo", () => {
> +      this._max_windows_undo = this._prefBranch.getIntPref("sessionstore.max_windows_undo");
> +    }, false);

Sorry, totally my fault. I forgot that we actually need to handle changes to these preferences in that we might need to truncate the list of tabs and windows to restore. The easiest way to fix this is to just add 'this' as the observer like we had it before, that way onPrefChange() handles value changes for us:

  this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
  this._prefBranch.addObserver("sessionstore.max_windows_undo", this, true);

Something like this should work.

Comment 19

[Amod [:AMoz]]

Attachment: patchv5

please let me know in case of additional changes. :)

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 812165 [details] [diff] [review]
patchv5

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +537,5 @@
>        gDebuggingEnabled = this._prefBranch.getBoolPref("sessionstore.debug");
>      }, false);
>  
> +    this._prefBranch.addObserver("sessionstore.max_tabs_undo", this, true);
> +    Services.prefs.addObserver("browser.sessionstore.max_tabs_undo", () => {

We can remove the 'Services.prefs.addObserver' line. But we need to keep the line that sets the property. It was included in the previous patch but you unfortunately removed it. We need that back :)

The 'this._prefBranch.addObserver' line should ideally come after the line that sets the property. Same thing for max_windows_undo below.

Comment 21

[Amod [:AMoz]]

Attachment: patchv6

I am extremely sorry for the mistake.. Even I dont believe that I made such stupid mistake. This patch should be correct.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 812769 [details] [diff] [review]
patchv6

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

Thanks! Sorry for the late response, I was out for a while after the Summit. Can you please prepare the patch for checkin? [1]

[1] http://blog.bonardo.net/2010/06/22/so-youre-about-to-use-checkin-needed

Comment 23

[Amod [:AMoz]]

Attachment: bug-758138-fix

I am unable to set myself as the author of the patch. 
here i have pastebin-ed my .hgrc file contents: http://pastebin.mozilla.org/3311273

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 820271 [details] [diff] [review]
bug-758138-fix

Why are you unable to do that? Using 'hg qref -u "Your Name <[e-mail]>"' should work, no?

Also, please add 'r=ttaubert' at the end of the patch description. Thanks!

Comment 25

[Amod [:AMoz]]

Attachment: bug-758138-fix

Comment 26

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 821131 [details] [diff] [review]
bug-758138-fix

Thank you, Amod! No need to review this again.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/9f6f1acf4290

Thanks a lot for your contribution, Amod! I just pushed this to the fx-team branch, from there on it should be merged to our main branch (mozilla-central) soon and will then be in Nightly.

Comment 28

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9f6f1acf4290

Comment 29

[Amod [:AMoz]]

Thank you very much Tim. Had a great time working with you. :)Looking forward to work with you again.