Last Comment Bug 665702 - expose session information in getBrowserState
: expose session information in getBrowserState
Status: RESOLVED FIXED
[good first bug][mentor=zpao]
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
: -- normal (vote)
: Firefox 10
Assigned To: Alastair Robertson
:
Mentors:
Depends on:
Blocks: 698557
  Show dependency treegraph
 
Reported: 2011-06-20 14:09 PDT by Paul O'Shannessy [:zpao] (not reading much bugmail, email directly)
Modified: 2011-10-31 13:10 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch 1 - moved oState.session.* to _getCurrentState (2.26 KB, patch)
2011-08-09 15:49 PDT, Alastair Robertson
no flags Details | Diff | Splinter Review
Patch 2 - added test (3.28 KB, patch)
2011-08-14 03:50 PDT, Alastair Robertson
paul: review+
Details | Diff | Splinter Review
Patch 3 (3.85 KB, patch)
2011-09-04 14:57 PDT, Alastair Robertson
no flags Details | Diff | Splinter Review
Patch 3.1 - changed var to let (3.85 KB, patch)
2011-09-06 00:53 PDT, Alastair Robertson
no flags Details | Diff | Splinter Review
Patch 3.2 - changed email address (3.84 KB, patch)
2011-09-14 05:17 PDT, Alastair Robertson
no flags Details | Diff | Splinter Review

Description Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-06-20 14:09:38 PDT
We currently only add state.session.* when writing to disk (in saveState). We should just add this data in _getCurrentState so it's exposed to consumers. Bug 665260 isn't super useful without this.
Comment 1 Sreenath 2011-07-23 03:24:54 PDT
Hi,

I am new to mozilla community and I am interested in starting with this bug, being a mentored one. I went through the code base and realized that the fix needs to be done in 
mozilla-central/browser/components/sessionstore/src/nsSessionStore.js file. 
I guess by adding : 
 oState.session= {
   state: this.loadState == STATE_RUNNING ? STATE_RUNNING_STR :STATE_STOPPED_STR
   lastUpdate: Date.now(),
   startTime: this._sessionStartTime
 }; as present in the saveState into _getCurrentState function, this can be fixed. Please give your suggestion on this. I could very well be wrong.
Comment 2 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-23 18:29:04 PDT
Hey Sreenath, welcome to the community and I'm glad you're interested in this bug :)

You're definitely on the right path there. We'll also want to make sure _recentCrashes is set, and clean up saveState. After that, this is something we can test, so you'll get to write a test :)

In case you haven't had a chance to build Firefox, you can follow the guide here: https://developer.mozilla.org/En/Simple_build (there are more complete build docs at https://developer.mozilla.org/En/Developer_Guide/Build_Instructions).

When adding a test for this, you'll add a file in https://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser/ and make sure that file is added to Makefile.in. I can help you more with testing when you're ready for that, but first step is to make a patch!

Feel free to jump on IRC and find me on IRC (zpao on irc.mozilla.org) and ask me an questions you have.
Comment 3 Sreenath 2011-07-24 01:09:02 PDT
Hi Paul,

Thanks a lot for your suggestion. I will get it patched soon and get back to you regarding the testing process. By the way, I have already builded the firefox. I am really glad that I 'll get my hands on to testing since its a new opportunity for me to learn more about testing methods and test scripts used by mozilla.
Comment 4 Sreenath 2011-07-24 10:16:23 PDT
Hi Paul,

I guess before adding oState.session, oState needs to be declared. I am confused regarding its declaration. I doubt a simple declaration of var oState will work since the oState.session needs to be declared as an object. I found its declaration in other functions as 
var oState = this._stateBackup;
But I didn't get that concept much. I couldn't reach to you through IRC since our timezones are different. It would be great if I could get some suggestion on this.
Comment 5 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-25 13:26:01 PDT
In saveState, oState is just a local variable defined as the object returned from _getCurrentState. oState in other cases aren't directly related (though you'll need to update the setting of oState.session around the code you mentioned in comment 4)
Comment 6 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-07-29 16:11:06 PDT
Alastair, I talked with Sreenath offline and he's not in a position to keep working on this right now. So if you'd like to take a go at it, feel free.
Comment 7 Alastair Robertson 2011-08-07 15:11:19 PDT
Sorry it took me so long to get this message - I just found it while looking through the list of session restore bugs. I've updated my preferences so I'll (hopefully) get emails when I'm CCed from now on.

As for the bug, I've had a quick look at it and I would like to have a go, thanks. I'll try to get to work on it soon.
Comment 8 Alastair Robertson 2011-08-09 15:49:05 PDT
Created attachment 551915 [details] [diff] [review]
Patch 1 - moved oState.session.* to _getCurrentState

Moved oState.session.* from saveState to _getCurrentState and added it to the returned obect in _getCurrentState.

What kind of things do we need to test for this bug?
Comment 9 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-11 09:52:25 PDT
(In reply to Alastair Robertson from comment #8)
> Created attachment 551915 [details] [diff] [review]
> Patch 1 - moved oState.session.* to _getCurrentState
> 
> Moved oState.session.* from saveState to _getCurrentState and added it to
> the returned obect in _getCurrentState.
> 
> What kind of things do we need to test for this bug?

That's looking good. Test should be pretty basic unit test - I just want to make sure that we calls to getBrowserState do in fact have the session data.
Comment 10 Alastair Robertson 2011-08-14 03:50:44 PDT
Created attachment 552953 [details] [diff] [review]
Patch 2 - added test
Comment 11 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-08-31 14:51:36 PDT
Comment on attachment 552953 [details] [diff] [review]
Patch 2 - added test

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

Looking good, just a few small things. Thanks (and sorry again for the delay)!

::: browser/components/sessionstore/src/nsSessionStore.js
@@ +2434,5 @@
>        ix = -1;
>  
> +    let session = {
> +      state: this._loadState == STATE_RUNNING ? STATE_RUNNING_STR : STATE_STOPPED_STR,
> +      lastUpdate: Date.now(),

This is going to be a little white lie, but that's ok. Historically it was (essentially) the last time the file was saved, now it won't necessarily be (though it will be accurate when actually saving to disk).

No change needed here, just remarking.

@@ +2438,5 @@
> +      lastUpdate: Date.now(),
> +      startTime: this._sessionStartTime
> +    };
> +    if (this._recentCrashes)
> +      session.recentCrashes = this._recentCrashes;

Let's just go ahead and put recent crashes in all of the time. It's only a few extra bytes, and it gives us consistency we can test.

::: browser/components/sessionstore/test/browser/browser_665702.js
@@ +1,3 @@
> +function test() {
> +  let currentState = JSON.parse(ss.getBrowserState());
> +  ok(currentState.session, "session data returned by getBrowserState");

Let's also check that we have the right keys and only those keys in currentState.session (you can get the keys out with Object.keys(currentState.session).

@@ +1,4 @@
> +function test() {
> +  let currentState = JSON.parse(ss.getBrowserState());
> +  ok(currentState.session, "session data returned by getBrowserState");
> +}

2 other things:
1. Make sure you have a license header (many of the tests are public domain now, but tri-license is also used)
2. Can you rename the file to browser_665702-state_session.js (I'm trying to introduce somewhat meaningful names for files).
Comment 12 Alastair Robertson 2011-09-04 14:57:23 PDT
Created attachment 558194 [details] [diff] [review]
Patch 3
Comment 13 Alastair Robertson 2011-09-05 01:33:16 PDT
+  for (var i = 0; i < a.length; i++) {
+    if (a[i] !== b[i]) {
+      return false;
+    }
+  }

I'll change this "var" to a "let" when I get a chance (should be later today).
Comment 14 Alastair Robertson 2011-09-06 00:53:41 PDT
Created attachment 558416 [details] [diff] [review]
Patch 3.1 - changed var to let
Comment 15 :Ehsan Akhgari 2011-09-08 08:34:45 PDT
This was backed out of mozilla-inbound as part of this push <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=9f150c4e1a48> which turned the 32-bit OS X reftests perma-orange.
Comment 16 Alastair Robertson 2011-09-14 05:16:27 PDT
(In reply to Ehsan Akhgari [:ehsan] from comment #15)
> This was backed out of mozilla-inbound as part of this push
> <https://tbpl.mozilla.org/?tree=Mozilla-
> Inbound&usebuildbot=1&rev=9f150c4e1a48> which turned the 32-bit OS X
> reftests perma-orange.

What do I need to do about this? (it's the first time I've been in this situation)

I don't see how my patch could cause problems with reftests, but I also don't have a Mac to check it on.
Comment 17 Alastair Robertson 2011-09-14 05:17:34 PDT
Created attachment 560137 [details] [diff] [review]
Patch 3.2 - changed email address

Just taking advantage of the delay to change my email address.
Comment 18 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-09-14 11:12:20 PDT
(In reply to Alastair Robertson from comment #16)
> (In reply to Ehsan Akhgari [:ehsan] from comment #15)
> > This was backed out of mozilla-inbound as part of this push
> > <https://tbpl.mozilla.org/?tree=Mozilla-
> > Inbound&usebuildbot=1&rev=9f150c4e1a48> which turned the 32-bit OS X
> > reftests perma-orange.
> 
> What do I need to do about this? (it's the first time I've been in this
> situation)
> 
> I don't see how my patch could cause problems with reftests, but I also
> don't have a Mac to check it on.

I wouldn't worry. It's pretty much impossible that it was this patch - it was probably one of those others that landed at the same time. I'm going to land this on try with a couple other patches first (then assuming that goes fine) land for real later.
Comment 19 Paul O'Shannessy [:zpao] (not reading much bugmail, email directly) 2011-10-25 15:15:02 PDT
Re-landed onto fx-team (sorry for the delay!). https://hg.mozilla.org/integration/fx-team/rev/402ffedeff99
Comment 20 :Margaret Leibovic 2011-10-27 11:37:43 PDT
https://hg.mozilla.org/mozilla-central/rev/402ffedeff99

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