Closed Bug 1369456 Opened 3 years ago Closed Last year

nsSessionStartup.js is loaded too early during startup

Categories

(Firefox :: Session Restore, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
Firefox 63
Tracking Status
firefox63 --- fixed

People

(Reporter: florian, Assigned: dao)

References

(Blocks 1 open bug)

Details

(Whiteboard: [fxperf:p2])

Attachments

(1 file)

nsSessionStartup.js is currently loaded during app-startup (before a profile is selected) only to add a final-ui-startup observer.

It seems we should instantiate this component from nsBrowserGlue.js instead. I'm not sure we could move this to after first paint though, so we may not win much.
(In reply to Florian Quèze [:florian] [:flo] from comment #0)
> It seems we should instantiate this component from nsBrowserGlue.js instead.
> I'm not sure we could move this to after first paint though, so we may not
> win much.

We need to start reading the session file as soon as possible, right?
Whiteboard: [fxperf]
Priority: -- → P3
This is now becoming slightly more important in the context of bug 1336227, as anything loaded before nsBrowserGlue.js delays the early blank window.
Blocks: 1336227
Priority: P3 → P2
Priority: P2 → --
Whiteboard: [fxperf] → [fxperf:p2]
Why did you remove P2?
Flags: needinfo?(paolo.mozmail)
Because we were incorrectly using the priority field to indicate in which bucket the bug was for the front-end performance team, instead of using a whiteboard tag like we are doing now. By using the whiteboard tag, we keep the priority field available for the separate triage process of the bug component. Hope this helps!
Flags: needinfo?(paolo.mozmail)
It helps. Thank you.
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED
See Also: → 1450559
Attachment #8997649 - Flags: review?(florian)
Comment on attachment 8997649 [details]
Bug 1369456 - Replace nsSessionStartup.js with SessionStartup.jsm.

https://reviewboard.mozilla.org/r/261338/#review268496

Nice cleanup, thanks! :-)

::: browser/components/sessionstore/SessionStartup.jsm
(Diff revision 1)
> -    case "quit-application":
> -      // no reason for initializing at this point (cf. bug 409115)
> -      Services.obs.removeObserver(this, "final-ui-startup");
> -      Services.obs.removeObserver(this, "quit-application");
> -      if (this._sessionType != Ci.nsISessionStartup.NO_SESSION)
> -        Services.obs.removeObserver(this, "browser:purge-session-history");

Can you explain why it's fine to remove these 2 lines?
Attachment #8997649 - Flags: review?(florian) → review+
(In reply to Florian Quèze [:florian] from comment #8)
> Comment on attachment 8997649 [details]
> Bug 1369456 - Replace nsSessionStartup.js with SessionStartup.jsm.
> 
> https://reviewboard.mozilla.org/r/261338/#review268496
> 
> Nice cleanup, thanks! :-)
> 
> ::: browser/components/sessionstore/SessionStartup.jsm
> (Diff revision 1)
> > -    case "quit-application":
> > -      // no reason for initializing at this point (cf. bug 409115)
> > -      Services.obs.removeObserver(this, "final-ui-startup");
> > -      Services.obs.removeObserver(this, "quit-application");
> > -      if (this._sessionType != Ci.nsISessionStartup.NO_SESSION)
> > -        Services.obs.removeObserver(this, "browser:purge-session-history");
> 
> Can you explain why it's fine to remove these 2 lines?

Because the observer service is using a weak reference.
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/409ecda4588d
Replace nsSessionStartup.js with SessionStartup.jsm. r=florian
https://hg.mozilla.org/mozilla-central/rev/409ecda4588d
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 63
You need to log in before you can comment on or make changes to this bug.