Closed Bug 665702 Opened 9 years ago Closed 9 years ago

expose session information in getBrowserState

Categories

(Firefox :: Session Restore, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 10

People

(Reporter: zpao, Assigned: alastair)

References

Details

(Whiteboard: [good first bug][mentor=zpao])

Attachments

(1 file, 4 obsolete files)

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.
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.
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.
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.
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.
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)
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.
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.
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?
(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.
Attached patch Patch 2 - added test (obsolete) — Splinter Review
Attachment #551915 - Attachment is obsolete: true
Attachment #552953 - Flags: review?(paul)
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).
Attachment #552953 - Flags: review?(paul) → review+
Attached patch Patch 3 (obsolete) — Splinter Review
Attachment #552953 - Attachment is obsolete: true
+  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).
Attached patch Patch 3.1 - changed var to let (obsolete) — Splinter Review
Attachment #558194 - Attachment is obsolete: true
Assignee: nobody → alastairrob
Keywords: checkin-needed
Whiteboard: [good first bug][mentor=zpao] → [good first bug][mentor=zpao][inbound]
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.
Whiteboard: [good first bug][mentor=zpao][inbound] → [good first bug][mentor=zpao]
Status: NEW → ASSIGNED
(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.
Just taking advantage of the delay to change my email address.
Attachment #558416 - Attachment is obsolete: true
(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.
Re-landed onto fx-team (sorry for the delay!). https://hg.mozilla.org/integration/fx-team/rev/402ffedeff99
Whiteboard: [good first bug][mentor=zpao] → [good first bug][mentor=zpao][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/402ffedeff99
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug][mentor=zpao][fixed-in-fx-team] → [good first bug][mentor=zpao]
Target Milestone: --- → Firefox 10
Blocks: 698557
You need to log in before you can comment on or make changes to this bug.