936061 - [Session Restore] restoreWindow() should only restore a single window

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Steven MacLeod [:smacleod]]

If a state with multiple windows is passed to |restoreWindow()|, it will open additional windows and restore them all: http://hg.mozilla.org/mozilla-central/file/7433abfef863/browser/components/sessionstore/src/SessionStore.jsm#l2242

It would be nice to clean this up and simplify |restoreWindow()| so that it will only restore a single window it is passed.

This method is called in a number of places, and is called by the following methods to actually restore state:

 * restoreLastSession - restoreWindow is called once for each window (is passed only the windows state), and acts as you'd expect.

 * onLoad - restoreWindow is called once but is passed all state

 * setBrowserState - restoreWindow is called once but restores multiple windows (is passed all state)

 * setWindowState - restoreWindow is called once but restores multiple windows (is passed all state)

The cases where |restoreWindow()| is used to restore multiple windows with a single call will need to be refactored to make multiple calls (similar to how |restoreLastSession()| is written).

Comment 1

[abhididdigi]

Hey Steven, Can I pick this bug up? Also, I need some help with this, as I'm just starting with Firefox. I'll contact you via irc.

Thanks.

Comment 2

[Steven MacLeod [:smacleod]]

Hi,

That would be great! Ping me on IRC and I can help you get started.

Comment 3

[Steven MacLeod [:smacleod]]

Hey Abhi,

How is progress? Do you have any more questions, or roadblocks I can help you get past?

Comment 4

[abhididdigi]

Hey steven, I'm now good with the build and the debug statements. Thanks to you for helping me out. I'm trying to reproduce the bug now.

Comment 5

[abhididdigi]

Attachment: rev1- Change to onLoad function.

Hi Steve, As discussed I've modified the onLoad with calls to restorwWindow one window at a time.

Please review and give me feedback, Thanks.

Comment 6

[Steven MacLeod [:smacleod]]

Comment on attachment 8334508 [details] [diff] [review]
rev1- Change to onLoad function.

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

Ahbi, sorry for this feedback taking so long. I've been thinking about this a
lot and I think we should go about it in a different way.

This bug is a little more complex than I initially thought. I think we should
take a different approach to the refactoring, instead of trying to sprinkle
loops with logic to restore multiple windows all over the place.

Since restoring multiple windows is a pretty common task it is probably best
to refactor |restoreWindow()| into two methods:

 a) restoreWindow(window, state, options): Restore a provided window state
    onto a single window.

 b) restoreWindows([windows], state, options): Restore a number of window states
    on top of the provided list of [windows]. If the number of windows in the
    state is greater than the number of provided windows, new windows will be
    opened.

This way |restoreWindow()| can focus on only the steps to take a window
object and apply a state to it, and |restoreWindows| can take care of the
logic for restoring multiple windows.

With that separation complete, the callsites to |restoreWindow()| could be
updated to call the appropriate method. |restoreWindow()| in the case where
they only restore a single window, and |restoreWindows()| if we need to restore
multiple at once.

Places where we should call |restoreWindow()|:
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l784
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l860
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l1516
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l1930 (Although in this case it might be better to refactor the whole method, and call |restoreWindows()|. We don't need to worry about that in this bug though)
  - Inside of |restoreWindows()| of course :D

Places where we should call |restoreWindows()|:
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l764
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l798
  - http://hg.mozilla.org/mozilla-central/file/118234ab24ed/browser/components/sessionstore/src/SessionStore.jsm#l1495

I think this will result in much more readable code. Ahbi, would you still like
to work on this bug, and move forward with the new plan?

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +687,5 @@
>     * @param aInitialState
>     *        The initial state to be loaded after startup (optional)
>     */
>    onLoad: function ssi_onLoad(aWindow, aInitialState = null) {
> +    

This patch introduces trailing whitespace in a number of places, which should be avoided. If you click "Review" for your patch in this bug, you'll see the trailing whitespace highlighted in red (such as on this line).

@@ +688,5 @@
>     *        The initial state to be loaded after startup (optional)
>     */
>    onLoad: function ssi_onLoad(aWindow, aInitialState = null) {
> +    
> +    console = Components.utils.import("resource://gre/modules/devtools/Console.jsm").console;

I know this is just a WIP patch to get some feedback on your approach, so leaving in debug stuff is fine. Just wanted to let you know you'll want to make sure things like this are removed in the final patch :D

@@ +734,5 @@
>            this._restoreCount = aInitialState.windows ? aInitialState.windows.length : 0;
>  
>            let overwrite = this._isCmdLineEmpty(aWindow, aInitialState);
>            let options = {firstWindow: true, overwriteTabs: overwrite};
> +          for(let i=0; i< aInitialState.length; i++){

The style for this for loop, and others like it, should be (Note the change in spacing):

|for (let i=0; i < aInitialState.length; i++) {|

@@ +737,5 @@
>            let options = {firstWindow: true, overwriteTabs: overwrite};
> +          for(let i=0; i< aInitialState.length; i++){
> +              let singleWindowItem = [];
> +              singleWindowItem.push(aInitialState.windows[i]);
> +              this.restoreWindow(aWindow,{windows:singleWindowItem},options);

There are issues with the way we're restoring here:

1) Restore window is being called on the same window each loop iteration (aWindow being passed each time is the same window). Which means that window would be restored to each window state, over and over, resulting in that window having the last windows state.

2) We still call |restoreWindow()| with the entire state after the loop has completed (which is probably masking the bugs in the loop restoration from you).

@@ +777,3 @@
>      }
>      // The user opened another, non-private window after starting up with
> +    // a sijngle private one. Let's restore the session we actually wanted to

introduced a typo here "sijngle"

Comment 7

[Steven MacLeod [:smacleod]]

Tim, does the approach I laid out in Comment 6 sound fine to you as well?

Comment 8

[abhididdigi]

Steve, I am very much okay with this way. I will work on this and attach the code(after removing the logs and all)

Thanks for your feedback.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Steven MacLeod [:smacleod] from comment #7)
> Tim, does the approach I laid out in Comment 6 sound fine to you as well?

Yup! :)

Comment 10

[Kegan Bouffard]

I'm not sure if this is the place to post this or not but it seemed like a good place to start. After my 12/09/2013 update to nightly I could no longer restore more then one windows with session restore. Windows 8.1 Enterprise Edition.

Comment 11

[Steven MacLeod [:smacleod]]

(In reply to abhididdigi from comment #8)
> Steve, I am very much okay with this way. I will work on this and attach the
> code(after removing the logs and all)

Ahbi, have you had a chance to work on this since my feedback? Are you still planning to complete it?

Comment 12

[abhididdigi]

Steven, Teribbly sorry. I was on a vacation. I just came back on 3-Jan-2014.I'm planning to work on it and complete it. I'll talk to you today.

Thank you.

Comment 13

[Zach Perrault]

Hey Steven, I talked to you on IRC and would like to work on this bug. Could I get assigned to this please?

Comment 14

[Steven MacLeod [:smacleod]]

Hey Zach, sure thing!

Comment 15

[Steven MacLeod [:smacleod]]

Hey Zach,

Just wanted to check in with you and see how things are going? Please let me know if there is anything I can do to help you move forward with this bug!

Comment 16

[Zach Perrault]

Hey Steven,

I actually left my laptop power adapter at work over the weekend so I couldn't work on this for the past few days. I am still going through the code and the debugging information you gave me is really helping. I will let you know when I have more questions.

Comment 17

[Steven MacLeod [:smacleod]]

(In reply to Zach Perrault from comment #16)
> Hey Steven,
> 
> I actually left my laptop power adapter at work over the weekend so I
> couldn't work on this for the past few days. I am still going through the
> code and the debugging information you gave me is really helping. I will let
> you know when I have more questions.

Cool, thanks for the update :D

Comment 18

[Roy Li]

Attachment: Refactor restoreWindow() into restoreWindow() and restoreWindows()

Hey Steven, I've worked on this for a couple of days and I found an issue with what you previously outlined: If we are to be able to call both removeWindow() and removeWindows() in other parts of the file, removeWindow() must deal with a lot of the logic that we want to move away from it.

I've experimented with what can be moved from removeWindow() to removeWindows(), and I've found that this for loop (1) and possibly (I did not test this one individually) this if statement (2) are not required by a standalone removeWindow(). However, if we make just make removeWindows() include those 1 or 2 blocks in addition to calling removeWindow(), it may be detrimental to refactor.

Instead, we could split out the parts of the original removeWindow() that are specific to restoring a single window and put that into the new removeWindow() and leave everything else in the new removeWindows(). We would always call removeWindows(), so removeWindow() would just be a helper function, which also might not be what we're looking for. I've attached a preliminary example of how this might look. Let me know what you think!

(1): http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2349
(2): http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/SessionStore.jsm#2332

Comment 19

[Steven MacLeod [:smacleod]]

Hey Roy, thanks for the patch!

I'll try to review this today, but if I can't you can expect something tomorrow.

Comment 20

[Steven MacLeod [:smacleod]]

(In reply to Steven MacLeod [:smacleod] from comment #19)
> I'll try to review this today, but if I can't you can expect something
> tomorrow.

Bah, I was really swamped today and didn't get around to this. It's my top priority for tomorrow though so you can expect a review very soon.

From my quick glance at things it does look like you're very much on the right track though, thanks for working on this :)

Comment 21

[Steven MacLeod [:smacleod]]

Attachment: annotated-restoreWindow.js

(In reply to Roy Li from comment #18)
> Hey Steven, I've worked on this for a couple of days and I found an issue
> with what you previously outlined: If we are to be able to call both
> removeWindow() and removeWindows() in other parts of the file,
> removeWindow() must deal with a lot of the logic that we want to move away
> from it.
So in the end the new restoreWindow *is* going to end up with most of the logic from the current implementation. What we really want to do though is focus that logic to just the window restoration bits and try and tear everything else not directly related to restoring a single window.

> I've experimented with what can be moved from removeWindow() to
> removeWindows(), and I've found that this for loop (1) and possibly (I did
> not test this one individually) this if statement (2) are not required by a
> standalone removeWindow(). However, if we make just make removeWindows()
> include those 1 or 2 blocks in addition to calling removeWindow(), it may be
> detrimental to refactor.
I've attached a js file containing the source code for the old restoreWindow, but I've annotated it with large comments denoting blocks of the code, and whether they should be in restoreWindow [window], or restoreWindows [windows].

> Instead, we could split out the parts of the original removeWindow() that
> are specific to restoring a single window and put that into the new
> removeWindow() and leave everything else in the new removeWindows().
That's exactly what we're looking to do :)

> We would always call removeWindows(), so removeWindow() would just be a helper
> function, which also might not be what we're looking for. I've attached a
> preliminary example of how this might look. Let me know what you think!

I should note that restoreWindow is an internal API only used inside SessionStore.jsm, so it already kind of is a helper function. We should feel free to modify it's arguments etc and add new helper methods where it makes sense, so long as we update all the callsites.

Your proposed plan from above is exactly what we're looking for, a highly focused restoreWindow used for restoring only a single window. What might really help with this though is changing the arguments to restoreWindow so it takes an actually parsed state for a single window, and doesn't deal with JSON etc.

I don't think we need to restrict ourselves to never calling restoreWindow outside of restoreWindows. Once it is refactored to just take an already parsed state and some options, we can just update the callsites that only restore a single window to pass the correct data. For starters though it's probably fine to just always call restoreWindows, and consider restoreWindow a helper, until it's refactored nicely - then the call sites could be updated.

Thanks for digging into all this! Definitely let me know if you have more questions about my annotations of the function, or anything else for that matter.

Comment 22

[Roy Li]

Thanks for your response! I hope I'm not taking too much of your time.

I changed the functions according to your attachment. I called restoreWindow() and restoreWindows() where you noted a long time ago in this thread. In setWindowState(), I had to change restoreWindows() back to restoreWindow() in order to pass a test (1) that deals with closed windows.

The way I have it now, we attempt to parse the state twice when restoreWindows() is called (once in restoreWindows() and once in restoreWindow()). Should I have restoreWindow() take a parsed state and parse states prior to calling it in the other parts of the code?

(1)http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_491577.js

Comment 23

[Steven MacLeod [:smacleod]]

(In reply to Roy Li from comment #22)
> Thanks for your response! I hope I'm not taking too much of your time.
Not at all, thanks for helping out!

> I changed the functions according to your attachment. I called
> restoreWindow() and restoreWindows() where you noted a long time ago in this
> thread.
It wouldn't surprise me if things have changed enough that some of the suggestions
I made about which to call are incorrect. It's definitely worth sanity checking
them.

> In setWindowState(), I had to change restoreWindows() back to
> restoreWindow() in order to pass a test (1) that deals with closed windows.
I would expect setWindowState to require "restoreWindows" rather than "restoreWindow"
as you've noted above, was the ordering a typo? Unfortunately setWindowState has
historically restored multiple windows and I don't think we should handle changing
that in this bug (It might be worth following up in another bug later though - it
will just be more difficult as addons etc. could be relying on the current behaviour).

If that test is failing when calling "restoreWindows" inside of setWindowState something
is going wrong, and "restoreWindows" itself will need to be fixed.

> The way I have it now, we attempt to parse the state twice when
> restoreWindows() is called (once in restoreWindows() and once in
> restoreWindow()). Should I have restoreWindow() take a parsed state and
> parse states prior to calling it in the other parts of the code?
I think parsing twice is fine for a first draft. After I've done a feedback pass of the
code we should be able to remove the parsing in "restoreWindow". Generally the places
"restoreWindow" gets called are given a parsed state I believe, so it will most likely
*just work*. The places that fail we might be able to just store the already parsed state
and pass it, rather than parsing at the call site.

Comment 24

[Roy Li]

Attachment: Refactor restoreWindow() into restoreWindow() and restoreWindows()

Yeah, that was a typo. setWindowState works with restoreWindows.

Here's what I have so far.

Comment 25

[Steven MacLeod [:smacleod]]

Comment on attachment 8572303 [details] [diff] [review]
Refactor restoreWindow() into restoreWindow() and restoreWindows()

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

Roy, this is looking great!

Now that we have the window restoration factored out let's remove the state parsing from "restoreWindow" and update the call sites where we only restore a single window to use "restoreWindow". I don't foresee this bewing too difficult, but you'll probably have to follow how the data flows through the different methods and events(such as openWindowWithState and onLoad). Let me know if you have any issues with this or have any questions.

Thanks for taking care of this, window restoration is going to be so much easier to follow with this fixed :D

::: browser/components/sessionstore/SessionStore.jsm
@@ +2554,3 @@
>    },
>  
> +    /**

This indentation change should be reverted.

Comment 26

[Roy Li]

When we call restoreWindow outside of restoreWindows, it always takes an element from the window array or _statesToRestore array. It looks like whenever we add elements to these arrays, they are always objects and never strings. Removing parsing from restoreWindow without changing call sites doesn't cause any test case to fail either. Is simply taking out the parsing in restoreWindow sufficient?

Comment 27

[Steven MacLeod [:smacleod]]

(In reply to Roy Li from comment #26)
> When we call restoreWindow outside of restoreWindows, it always takes an
> element from the window array or _statesToRestore array. It looks like
> whenever we add elements to these arrays, they are always objects and never
> strings. Removing parsing from restoreWindow without changing call sites
> doesn't cause any test case to fail either. Is simply taking out the parsing
> in restoreWindow sufficient?
Yup, that sounds fine then.

Comment 28

[Roy Li]

Attachment: Refactor restoreWindow() into restoreWindow() and restoreWindows()

Okay, here's the patch.

Comment 29

[Steven MacLeod [:smacleod]]

Comment on attachment 8579089 [details] [diff] [review]
Refactor restoreWindow() into restoreWindow() and restoreWindows()

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

Roy, this patch is awesome :D

I just have a few little comment issues, and then there is one more thing I think is worth fixing while we're at it.

Thanks for working on this, restoration is way easier to understand with this change!

::: browser/components/sessionstore/SessionStore.jsm
@@ +2325,5 @@
>      // We're not returning from this before we end up calling restoreTabs
>      // for this window, so make sure we send the SSWindowStateBusy event.
>      this._setWindowStateBusy(aWindow);
>  
> +    let winData = aState.windows[0];

Now that this is the only place we access aState, just to get the single windows data, lets just pass in the window data itself.

We'll want to remove this line, change the aState argument to be winData, and then update all the callsites to pass the windows data, rather than the state object with a list of windows.

This might also involve changing what we stick in _statesToRestore and making sure all the users expect the right thing.

@@ +2474,5 @@
> +    this._sendRestoreCompletedNotifications();
> +  },
> +
> +  /**
> +   * restore a window state

Lets go with something like "Restore multiple windows using the provided state."

@@ +2476,5 @@
> +
> +  /**
> +   * restore a window state
> +   * @param aWindow
> +   *        Window reference

"Window reference to the first window to use for restoration. Additionally required windows will be opened".

@@ +2478,5 @@
> +   * restore a window state
> +   * @param aWindow
> +   *        Window reference
> +   * @param aState
> +   *        JS object or its eval'able source

This comment is stale, we should change it now to "JS object or JSON string"

Comment 30

[Steven MacLeod [:smacleod]]

Roy, I think you should consider applying[1] for Level 1[2] Commit Access so that you are able to run tests on Mozilla's try server[3]. We'll want to run some of the automated tests on Try before landing this change and it would be great if you were able to do that.

If you'd prefer I just run the tests for you that is fine as well, just let me know.

[1] https://www.mozilla.org/en-US/about/governance/policies/commit/
[2] https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
[3] https://wiki.mozilla.org/Build:TryServer

Comment 31

[Roy Li]

Attachment: Refactor restoreWindow() into restoreWindow() and restoreWindows()

I changed the callsites slightly, but didn't change _statesToRestore. Let me know if it's alright.

I don't want to apply for commmit access at the moment. I think I might the next time I submit a patch, but for the meantime, it would be great if you could run the tests for me.

Comment 32

[Steven MacLeod [:smacleod]]

(In reply to Roy Li from comment #31)
> Created attachment 8582991 [details] [diff] [review]
Thanks for updating the patch, I didn't get around to reviewing this today but I'll give it a look tomorrow.

> I don't want to apply for commmit access at the moment. I think I might the
> next time I submit a patch, but for the meantime, it would be great if you
> could run the tests for me.

No problem!

Comment 33

[Steven MacLeod [:smacleod]]

Comment on attachment 8582991 [details] [diff] [review]
Refactor restoreWindow() into restoreWindow() and restoreWindows()

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

Looks good, I just have a few small comments. I've pushed the patch to try and if it passes all the tests we should be good to land with my few issues addressed :D

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bdbcba9f53e8

::: browser/components/sessionstore/SessionStore.jsm
@@ +930,5 @@
>            // Ensure that the window state isn't hidden
>            this._restoreCount = 1;
>            let state = { windows: [newWindowState] };
>            let options = {overwriteTabs: this._isCmdLineEmpty(aWindow, state)};
> +          this.restoreWindow(aWindow, state.windows[0], options);

We should remove the "let state = { windows: [newWindowState] };" line above and just pass in "newWindowState" here.

@@ +1972,2 @@
>          let options = {overwriteTabs: canOverwriteTabs, isFollowUp: true};
> +        this.restoreWindow(windowToUse, state.windows[0], options);

again rather then having a "let state ..." line above, just pass in winState.

@@ +2476,5 @@
> +
> +  /**
> +   * Restore multiple windows using the provided state.
> +   * @param aWindow
> +   *        Window reference to the first window to use for restoration. 

nit: please remove the trailing whitespace here.

Comment 34

[Roy Li]

Regarding the first issue: On line 933, we need something to pass into _isCmdLineEmpty. It could be:

let options = {overwriteTabs: this._isCmdLineEmpty(aWindow, { windows: [newWindowState] })};
this.restoreWindow(aWindow, newWindowState, options);

I think version with "let state" is preferable, but let me know if this new one would be better.

Comment 35

[Steven MacLeod [:smacleod]]

(In reply to Roy Li from comment #34)
> Regarding the first issue: On line 933, we need something to pass into
> _isCmdLineEmpty. It could be:
> 
> let options = {overwriteTabs: this._isCmdLineEmpty(aWindow, { windows:
> [newWindowState] })};
> this.restoreWindow(aWindow, newWindowState, options);
> 
> I think version with "let state" is preferable, but let me know if this new
> one would be better.

Lets go with a hybrid, leave the "let state = ..." line, but instead of passing "state.windows[0]" to restoreWindow, just pass "newWindowState".

Comment 36

[Roy Li]

Attachment: Refactor restoreWindow() into restoreWindow() and restoreWindows()

Okay, here are the changes.

Comment 37

[Steven MacLeod [:smacleod]]

Comment on attachment 8585573 [details] [diff] [review]
Refactor restoreWindow() into restoreWindow() and restoreWindows()

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

Looks good to me. Thanks for fixing this up Roy!

Comment 38

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/6c0b59c82006

Comment 39

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c0b59c82006