[Session Restore] Don't collect state right after startup when restoring the initial session

RESOLVED FIXED in Firefox 25

Status

()

Firefox
Session Restore
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: ttaubert, Assigned: ttaubert)

Tracking

({perf})

Trunk
Firefox 25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/src/SessionStore.jsm#712

Right after restoring the initial state for the first window loaded after startup, we call .saveState() only to set the session state from STOPPED to RUNNING to count startup crashes correctly.

It would be great to just change the state without writing the whole file but we cannot do that. What we can do is at least not collect the browser state at a point where probably nothing has even finished restoring, yet.
(Assignee)

Comment 1

5 years ago
Created attachment 767900 [details] [diff] [review]
don't collect state right after startup when restoring the initial session
Attachment #767900 - Flags: review?(dteller)
Comment on attachment 767900 [details] [diff] [review]
don't collect state right after startup when restoring the initial session

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

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +714,5 @@
>  
>            // _loadState changed from "stopped" to "running"
>            // force a save operation so that crashes happening during startup are correctly counted
> +          this._initialState.session.state = STATE_RUNNING_STR;
> +          this._saveStateObject(this._initialState);

Do we actually need the call to saveStateObject?

@@ +715,5 @@
>            // _loadState changed from "stopped" to "running"
>            // force a save operation so that crashes happening during startup are correctly counted
> +          this._initialState.session.state = STATE_RUNNING_STR;
> +          this._saveStateObject(this._initialState);
> +          delete this._initialState;

Could we take the opportunity to remove that |delete| and replace it with an assignment? Calling |delete| destroys inline caching.
Attachment #767900 - Flags: review?(dteller) → feedback+
Summary: Don't collect state right after startup when restoring the initial session → [Session Restore] Don't collect state right after startup when restoring the initial session
Blocks: 887780
(Assignee)

Comment 3

5 years ago
Created attachment 768277 [details] [diff] [review]
don't collect state right after startup when restoring the initial session
Attachment #767900 - Attachment is obsolete: true
Attachment #768277 - Flags: review?(dteller)
Comment on attachment 768277 [details] [diff] [review]
don't collect state right after startup when restoring the initial session

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

Looks good.
It might also prove useful for the day saveState becomes async.
Attachment #768277 - Flags: review?(dteller) → review+
(Assignee)

Comment 5

5 years ago
https://hg.mozilla.org/integration/fx-team/rev/086465524970
https://hg.mozilla.org/mozilla-central/rev/086465524970
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25

Updated

5 years ago
Depends on: 890421

Updated

5 years ago
Depends on: 889711
Depends on: 889206
Depends on: 890034
Duplicate of this bug: 891023

Updated

4 years ago
Depends on: 919532
You need to log in before you can comment on or make changes to this bug.