1214158 - [Session Restore] Measure time until all tabs that are restored eagerly are restored

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Comment 1

[David Teller [:Yoric] - still alive but not very active]

It seems that this code has changed a lot since the last time I looked at it. Do you remember where we store the tabs that we wish to restore eagerly, tim?

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

I'm not sure what you mean? Since we introduced restore_on_demand I don't think there have been a lot of changes to that code. Tabs that are not pinned, not selected or hidden will be restored on demand. IIRC that's handled via the TabRestoreQueue.

Comment 3

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Bug 1214158 - Measure the time for Session Restore to restore all tabs in the initial set;r?ttaubert

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review20077

Kicking off the load at this point seems flawed, if I restore a session with only two tabs I might be able to trigger an erroneous value just by hitting Ctrl+Shift+T a couple of times, maybe. We definitely don't guard against this being measured after startup.

What about recording this value when we finished creating tabs for the initial window, in restoreWindow()? This function takes a few options, one of those is |firstWindow| which we can check. restoreNextTab() is called off of a few messages we receive so that might not be a very accurate measurement anyway.

Comment 5

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review20077

> Kicking off the load at this point seems flawed, if I restore a session with only two tabs I might be able to trigger an erroneous value just by hitting Ctrl+Shift+T a couple of times, maybe. We definitely don't guard against this being measured after startup.

I assumed that most users wouldn't do that and that this wouldn't show up in statistics.


> What about recording this value when we finished creating tabs for the initial window, in restoreWindow()? This function takes a few options, one of those is |firstWindow| which we can check. restoreNextTab() is called off of a few messages we receive so that might not be a very accurate measurement anyway.

The main objective of this measure is to find out if/how much e10s makes time-to-usable-browser slower. I believe that measuring time until tabs are created (without checking that they are effectively restored) would measure something different. Or do I misunderstand your suggestion?

Comment 6

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Bug 1214158 - Measure the time for Session Restore to restore all tabs in the initial set;r?ttaubert

This patch adds a new measure to the timeline: sessionRestoreTabsRestored. This measure is updated only for users who have chosen to restore tabs automatically, and is set once all the tabs set to be restored immediately have had their content restored. In case of crash, this measure remains unset.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Tim, this is rather high-priority, any chance you could get to it quickly?

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review20987

Wow, this is way too complicated. We shouldn't make the restore process even harder to read, and all that only because we want telemetry.

I think I liked the first version better, minus the concerns I already voiced. Looking at v1 again though, I don't think it does what you want. It reports a value at the time when three tabs are restoring concurrently, not when the "initial set" is fully restored.

Without extensive tracking of restoring tabs I don't think we have a good way of reporting the value you currently want. Why not just report a value whenever the first "restoreTabContentComplete" message is received? That marks the time we need to restore the first tab, which is in the initial set. The more tabs we wait for to be restored, the more variance the values reported will have anyway probably.

Comment 9

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review20987

I just looked at the code and if I read it correctly, restoreTabContentComplete is actually sent *before* we start loading the contents of the tab. So we probably don't want to measure time until `restoreTabContentComplete`, but time until `gContentRestore.restoreDocument()` is complete. This will require a little patch, but let's assume that we know how to do this.

You are probably right that waiting until the first (or first three) `restoreDocumentComplete` (assuming that's how we call the message) would be an interesting measure, as it would help us pinpoint the cause of regressions. But we also want the gritty version that encompasses all real-world issues that could regress behind our back, including tabs, windows, pinned tabs, etc. If you wish, we can start with the former, but we'll need to work on the latter in a followup bug pretty quickly, as such regression-tracking tool is a blocker for e10s.

Comment 10

[David Teller [:Yoric] - still alive but not very active]

Ok, I think I'm mostly wrong about `restoreTabContentComplete`. I am not 100% sure, but it might be fired once contents are indeed complete. It's still missing form data & scroll position, but tracking that might be done in a followup bug.

On the other hand, I have a fun fact to report: if your have at least three about: pages among your eagerly restored tabs (which was my case when I tested), the first `restoreTabContentComplete` is received only when you click on yet another tab. I can't find a way to fix this without one of 1/ ugly hacks; or 2/ my review request from comment 6.

So I'd rather pursue with that review request, since it might actually be the simplest option. I don't feel that returning a Promise makes the code harder to read. If you feel that it does, do you have alternative strategies to suggest?

Comment 11

[Tim Taubert [:ttaubert] (inactive)]

(In reply to David Rajchenbach-Teller [:Yoric] (use "needinfo") from comment #10)
> Ok, I think I'm mostly wrong about `restoreTabContentComplete`. I am not
> 100% sure, but it might be fired once contents are indeed complete. It's
> still missing form data & scroll position, but tracking that might be done
> in a followup bug.

gContentRestore.restoreDocument() is a sync operation. I don't think it makes sense to measure it, whether that happens before or after we send |restoreTabContentComplete|.

> On the other hand, I have a fun fact to report: if your have at least three
> about: pages among your eagerly restored tabs (which was my case when I
> tested), the first `restoreTabContentComplete` is received only when you
> click on yet another tab. I can't find a way to fix this without one of 1/
> ugly hacks; or 2/ my review request from comment 6.

I guess we should find out why those aren't sent. That seems not expected.

> So I'd rather pursue with that review request, since it might actually be
> the simplest option. I don't feel that returning a Promise makes the code
> harder to read. If you feel that it does, do you have alternative strategies
> to suggest?

I am not okay with the current patch as it is, spraying promises all over the code only to collect a single
telemetry value isn't something that I would consider worth the maintenance impact. Maybe you should consider code that isn't in SessionStore, after all you only want to know how long the initial set of tabs takes to load.

SessionStoreInternal.onBeforeBrowserWindowShown() is where we wait for the first window to load and initialize, then we call restoreWindow() to kick off loading the initial set of tabs. You could write an external component that waits for the first |browser-window-before-show| notification and then tracks |SSTabRestoring| and/or |SSTabRestored| notifications to find the point at which the last initial tab finished restoring.

Or something along those lines, maybe there's an even better option out there. You can just as well track |TabOpen| events and simply listen for |load| events.

Comment 12

[David Teller [:Yoric] - still alive but not very active]

I can look in that direction.

How do you propose I find out the number of `SSTabRestored`/`TabOpen` messages we should expect? `MAX_CONCURRENT_TAB_RESTORES` is not reliable, as it is affected by the number of windows. I suppose I could expose that number as a semi-public API, but that's not very nice.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

So MAX_CONCURRENT_TAB_RESTORES=3 by default and shared between windows, yes. Maybe I'm not totally clear yet about what you exactly call the 'initial set' of tabs. Is it all tabs that are automatically restored, i.e. 5 pinned tabs and one selected tab? Would that only be in the first window? Is it the first MAX_CONCURRENT_TAB_RESTORES tabs, restored in any window?

Comment 14

[David Teller [:Yoric] - still alive but not very active]

I want a good measure of "user who has selected 'Show windows and tabs from last time' can browse wish said windows and tabs". Since we don't know which window the user will use (or even which one will pop to front, if my memory serves), my first take on this is to wait until all tabs that are not deferred are loaded. Alternatively, we could probably wait until one tab has been restored for each window, but that seems actually more complicated.

So, are you saying that I should be waiting for `MAX_CONCURRENT_TAB_RESTORES` * number_of_windows notifications of `SSTabRestored`?

Comment 15

[David Teller [:Yoric] - still alive but not very active]

Note that we also don't know the number of windows to be restored. 
I guess I could publish the data either with an API or by broadcasting it when we start session restore.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

So... one thing that you could do is:

Early, when the window is initialized, i.e. browser.js loads, you install a SSWindowStateReady listener. If that fires then SessionStore has finished restoring into this window, and now you could go and register SSTabRestored listeners for every tab out there. Or maybe only the selected and the pinned tabs the are restored automatically.

If the SSTabRestored listeners then call some function in a JSM that keeps track of the last notification it received then that would be the time it took to restore all 'initial' tabs. A little after startup (1-2 mins) we could then simply report that value.

Comment 17

[David Teller [:Yoric] - still alive but not very active]

I can try that.

Comment 18

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Bug 1214158 - WIP - New module StartupPerformance to monitor the duration of restoration of initial tabs;f?ttaubert

Comment 19

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Something along these lines?

Comment 20

[Tim Taubert [:ttaubert] (inactive)]

https://reviewboard.mozilla.org/r/22433/#review22187

::: browser/components/sessionstore/SessionStore.jsm:489
(Diff revision 3)
> +    Services.obs.notifyObservers(null, NOTIFY_SESSION_RESTORE_INIT, "");

Instead of creating another point for add-ons to hook onto I think this could just as well listen for browser-window-before-show, or delayed-startup-finished, or something else if that doesn't work.

::: browser/components/sessionstore/StartupPerformance.jsm:45
(Diff revision 3)
> +      for (let tab of win.gBrowser.tabbrowser.tabs) {
> +        tab.addEventListener("SSTabRestored", function observer() {
> +          tab.removeEventListener("SSTabRestored", observer);

Not all of those tabs will be restored, some of them might be pending for a while. The countdown would never be zero.

It's better to register a single listener on the tabContainer to catch all SSTabRestored events. Each of those could then call an internal function that simply stores Date.now().

As soon as there haven't been any SSTabRestored events for a while (maybe 10s or more?) we should go and report that value.

Comment 21

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review22187

> Instead of creating another point for add-ons to hook onto I think this could just as well listen for browser-window-before-show, or delayed-startup-finished, or something else if that doesn't work.

I'm not a big fan of relying upon `browser-window-before-show` or `delayed-startup-finished`, which would introduce yet another implicit loading-order dependency. I have spent enough time debugging implicit shutdown-order dependencies as is. In addition, since we are phasing out XPCOM add-ons in favor of Web Extensions, I am not that concerned about adding a hook.

> Not all of those tabs will be restored, some of them might be pending for a while. The countdown would never be zero.
> 
> It's better to register a single listener on the tabContainer to catch all SSTabRestored events. Each of those could then call an internal function that simply stores Date.now().
> 
> As soon as there haven't been any SSTabRestored events for a while (maybe 10s or more?) we should go and report that value.

Ah, right, this is going to fail if we have fewer than 3 tabs in a window. Ok, I'll do that.

Comment 22

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review22187

> Ah, right, this is going to fail if we have fewer than 3 tabs in a window. Ok, I'll do that.

Note that we will probably end up capturing additional `SSTabRestored` events if the user clicks on another tab within 10 seconds. I guess we can live with this.

Comment 23

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/3-4/

Comment 24

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/4-5/

Comment 25

[David Teller [:Yoric] - still alive but not very active]

Attachment: MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r?ttaubert

Comment 26

[David Teller [:Yoric] - still alive but not very active]

Review ping?

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review23613

::: browser/components/sessionstore/SessionStore.jsm:20
(Diff revision 5)
> +// A new window has just been restored. At this stage, tabs are generally
> +// not restored.
> +const NOTIFY_SINGLE_WINDOW_RESTORED = "sessionstore-single-window-restored";

Looks like this isn't fired anywhere but the new JSM listens for it. Why not call the JSM directly instead of introducing another add-on hook?

::: browser/components/sessionstore/SessionStore.jsm:2123
(Diff revision 5)
> -
> +    Services.obs.notifyObservers(null, NOTIFY_WILL_RESTORE_SESSION, "");

That's not defined, right? Isn't that just another manual restore?

::: browser/components/sessionstore/StartupPerformance.jsm:11
(Diff revision 5)
> +// The maximal number of tabs that can be restored concurrently.
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm", this);

That comment is probably a remnant.

::: browser/components/sessionstore/StartupPerformance.jsm:36
(Diff revision 5)
> +  init: function() {
> +    for (let topic of TOPICS) {
> +      Services.obs.addObserver(this, topic, false);
> +    }
> +  },

Why do we need that? Can't we just call start() or record() or something when we start restoring?

::: browser/components/sessionstore/StartupPerformance.jsm:49
(Diff revision 5)
> +      this._deadlineTimer = Cc["@mozilla.org/timer;1"].createInstance(Ci.nsITimer);
> +      this._deadlineTimer.initWithCallback(() => {

Timer.jsm?

::: browser/components/sessionstore/StartupPerformance.jsm:62
(Diff revision 5)
> +      let histogramName = isAutoRestore ? "FX_SESSION_RESTORE_AUTO_RESTORE_DURATION_UNTIL_EAGER_TABS_RESTORED_MS" : "FX_SESSION_RESTORE_MANUAL_RESTORE_DURATION_UNTIL_EAGER_TABS_RESTORED_MS";

That's quite a long line, and also there's just a single word difference.

::: browser/components/sessionstore/StartupPerformance.jsm:68
(Diff revision 5)
> +  observe: function(subject, topic, details) {

We should get rid of that and just call the JSM directly.

::: browser/components/sessionstore/StartupPerformance.jsm:71
(Diff revision 5)
> +        case "sessionstore-restoring-on-startup":
> +          this._onRestorationStarts(true);
> +          break;

That topic isn't fired, right?

::: browser/components/sessionstore/StartupPerformance.jsm:104
(Diff revision 5)
> +          if (!this._deadlineTimer) {
> +            throw new Error("Internal Error: We are still restoring windows, but " +
> +                            "SessionRestore StartupPerformance has already collected" +
> +                             "what it thought were the final results");
> +          }

If you never remove observers then you'll surely get errors here with add-ons or Cmd+Shift+N, no?

::: browser/components/sessionstore/StartupPerformance.jsm:117
(Diff revision 5)
> +          let observer = () => {
> +            this._latestRestoredTimeStamp = Date.now();
> +          };
> +          win.gBrowser.tabContainer.addEventListener("SSTabRestored", observer);

Let's use handleEvent() to have cleaner code.

Alternatively we could also just call the JSM directly from SessionStore.jsm.

::: browser/components/sessionstore/content/aboutSessionRestore.js:115
(Diff revision 5)
> +  Services.obs.notifyObservers(null, "sessionstore-initiating-manual-restore", "");

As said before, just call the JSM.

Comment 28

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review23613

> Looks like this isn't fired anywhere but the new JSM listens for it. Why not call the JSM directly instead of introducing another add-on hook?

Good catch, cleanup roadkill.

Now, I'm not worried about add-ons, as WebExtensions will not even have observer support. Also, I figured this would be the method that would spaghettize the code the least. If you wish, I can call the .jsm directly, but only if you promise that you will review quickly.

> Why do we need that? Can't we just call start() or record() or something when we start restoring?

Since restoration can come from two different places, I figured that `init()` was simpler.

> Timer.jsm?

Well, it is easier to reset a timer with `nsITimer`.

> If you never remove observers then you'll surely get errors here with add-ons or Cmd+Shift+N, no?

Fair enough, I'll remove observers.

> Let's use handleEvent() to have cleaner code.
> 
> Alternatively we could also just call the JSM directly from SessionStore.jsm.

I don't know what you mean with `handleEvent()`.

Comment 29

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/5-6/

Comment 30

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/1-2/

Comment 31

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/6-7/

Comment 32

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/2-3/

Comment 33

[David Teller [:Yoric] - still alive but not very active]

Tired of waiting for review from ttaubert. Bill, could you take over?

Comment 34

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/6-7/

Comment 35

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/2-3/

Comment 36

[David Teller [:Yoric] - still alive but not very active]

Thanks for offering to review, Mike.

Comment 37

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review24619

Okay, I think I understand what you're doing here, but I'm curious to understand why we can't make things easier for you by adding the notifications you need inside SessionStore, instead of doing this timer business? I feel like knowing when the first window has started restoring and when the last tab has finished restoring is something that SessionStore can easily record itself without having this external component.

I'm possibly missing some context here though.

::: browser/components/sessionstore/SessionStore.jsm:2795
(Diff revision 6)
> +    Services.obs.notifyObservers(aWindow, NOTIFY_SINGLE_WINDOW_RESTORED, "");

Out of curiosity, is there some advantage to passing the empty string instead of null as the data?

::: browser/components/sessionstore/StartupPerformance.jsm:37
(Diff revision 6)
> +  // Function `resolve()` for `_promiseFinished`.

Nit: newline above here.

::: browser/components/sessionstore/StartupPerformance.jsm:54
(Diff revision 6)
> +  // Behavior is unspecified if there was already an ongoing measure.

If there's an ongoing measure, perhaps we should just bail out? Or at least, log that we seemed to have a measure already in progress before we clobber it.

::: browser/components/sessionstore/StartupPerformance.jsm:131
(Diff revision 6)
> +          // becomes usable, experience shows that this is too invasive. Rather,

Can you quickly run down for me what you mean by invasive? I'm not familiar with that.

::: browser/components/sessionstore/StartupPerformance.jsm:170
(Diff revision 6)
> +          throw new Error(`Unexpected topic ${topic}\n`);

Kinda weird to put a newline at the end of an error message like this.

::: toolkit/components/telemetry/Histograms.json:4390
(Diff revision 6)
> +    "description": "Session restore: If the browser is setup to auto-restore tabs, this probe measures the time elapsed between the instant we start Session Restore and the instant we have finished restoring tabs eagerly. At these stage, the tabs that are restored on demand are not restored yet."

"At these stage" -> "At this stage"

::: toolkit/components/telemetry/Histograms.json:4399
(Diff revision 6)
> +    "description": "Session restore: If a session is restored by the user clicking on 'Restore Session', this probe measures the time elapsed between the instant the user has clicked and the instant we have finished restoring tabs eagerly. At these stage, the tabs that are restored on demand are not restored yet."

"At these stage" -> "At this stage"

Comment 38

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

https://reviewboard.mozilla.org/r/25649/#review24621

This looks okay, but it's landing is dependant on the previous patch obviously, and I'm not 100% convinced that we need this separate component to do the recording when it could potentially be done in SessionStore itself.

::: browser/components/sessionstore/StartupPerformance.jsm:177
(Diff revision 2)
> +          this._totalNumberOfTabs += win.gBrowser.tabContainer.itemCount;

Out of curiosity, why not win.gBrowser.tabs.length?

Comment 39

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review24619

This was requested in comment 13, comment 16, ...

Basically, it complicates less the code of SessionStore.jsm. One of the issues is that the code of SessionStore.jsm is really not designed to find out when a given tab is restored. Another issue is that some tabs never send a message stating that they are effectively restored.

> Out of curiosity, is there some advantage to passing the empty string instead of null as the data?

Well, per specifications, it's a `wstring`. If we send `null`, I don't know what will happen if someone adds a C++ observer.

> If there's an ongoing measure, perhaps we should just bail out? Or at least, log that we seemed to have a measure already in progress before we clobber it.

I'm not sure. First, if we have two restorations, we don't know if a tab restored belongs to the old restoration or the new one, so bailing out will actually not help us keep the data sane. I can log it, if you think it's useful.

> Can you quickly run down for me what you mean by invasive? I'm not familiar with that.

See https://reviewboard.mozilla.org/r/22433/diff/2/ for some of the changes needed to be able to track the tabs. As I found out later, this does not cover all cases.

> Kinda weird to put a newline at the end of an error message like this.

Good catch.

Comment 40

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/25649/#review24621

> Out of curiosity, why not win.gBrowser.tabs.length?

No specific reason. I was using `tabContainer` on the line above, so I kept using `tabContainer`. I can change if you prefer.

Comment 41

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/6-7/

Comment 42

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/2-3/

Comment 43

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/7-8/

Comment 44

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/3-4/

Comment 45

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review26527

This looks really good, but I'm wondering if we're not handling the about:home case. Are we?

::: browser/components/sessionstore/SessionStore.jsm:2795
(Diff revision 8)
> +    Services.obs.notifyObservers(aWindow, NOTIFY_SINGLE_WINDOW_RESTORED, "");

I wonder if this should go above this.\_sendRestoreCompletedNotifications, since otherwise we might send NOTIFY_SINGLE_WINDOW_RESTORED after NOTIFY_BROWSER_STATE_RESTORED, which doesn't make a whole lot of sense.

::: browser/components/sessionstore/StartupPerformance.jsm:29
(Diff revision 8)
> +  // Instant at which we have started restoration (notification "sessionstore-startup")

notification "sessionstore-restoring-on-startup"

::: browser/components/sessionstore/StartupPerformance.jsm:57
(Diff revision 8)
> +    if (this._startTimeStamp != null) {
> +      console.log("StartupPerformance: we have two restorations in progress at the same time, statistics may be skewed");
> +    }

Thinking about it more, I don't know if re-entry is possible. The first time we enter this function, we'll remove the observers, on line 65-67 - and none of the calls between the start of this function and 65 should cause re-entry.

So I think we can probably just remove this. Sorry about that.

::: browser/components/sessionstore/StartupPerformance.jsm:116
(Diff revision 8)
> +        console.error("StartupPerformance: Error in timeout handler", ex);

Should we do the clean-up in a `finally` to reasonably ensure it occurs?

::: browser/components/sessionstore/content/aboutSessionRestore.js:115
(Diff revision 8)
> +  Services.obs.notifyObservers(null, "sessionstore-initiating-manual-restore", "");

What about the case where the user has initiated manual session restore from the about:home button? That's not captured here, is it?

Comment 46

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

https://reviewboard.mozilla.org/r/25649/#review26557

Please run eslint on this before committing. Thanks!

::: browser/components/sessionstore/StartupPerformance.jsm:100
(Diff revision 4)
> +

Remove newline

Comment 47

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review26527

Oh, good point.
Sadly, there isn't a good unique entry point into Session Restore, so I'll have to proceed with sprinkling `sessionstore-initiating-manual-restore` everywhere :/

> I wonder if this should go above this.\_sendRestoreCompletedNotifications, since otherwise we might send NOTIFY_SINGLE_WINDOW_RESTORED after NOTIFY_BROWSER_STATE_RESTORED, which doesn't make a whole lot of sense.

Fair enough.

> Thinking about it more, I don't know if re-entry is possible. The first time we enter this function, we'll remove the observers, on line 65-67 - and none of the calls between the start of this function and 65 should cause re-entry.
> 
> So I think we can probably just remove this. Sorry about that.

Ah, good point. This was possible in some earlier iteration, but we should be fine now.

> What about the case where the user has initiated manual session restore from the about:home button? That's not captured here, is it?

Good catch, fixed.

Comment 48

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/8-9/

Comment 49

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/4-5/

Comment 50

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review26695

::: browser/base/content/browser.js:7811
(Diff revision 9)
> +  Services.obs.notifyObservers(null, "sessionstore-initiating-manual-restore", "");

Can we not just put this somewhere in SessionStoreInternal.restoreLastSession instead of each callsite?

Comment 51

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/9-10/

Comment 52

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/5-6/

Comment 53

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/22433/#review26695

> Can we not just put this somewhere in SessionStoreInternal.restoreLastSession instead of each callsite?

Ok, this was easier than I thought. Thanks for insisting :)

Comment 54

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

https://reviewboard.mozilla.org/r/22433/#review26845

I suggest running these through ESLint to make sure I haven't missed any style things. Otherwise, this looks good to me. Thanks Yoric!

Comment 55

[David Teller [:Yoric] - still alive but not very active]

Eslint passed.
Thanks for the reviews :)

Comment 56

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22433/diff/10-11/

Comment 57

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25649/diff/6-7/

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a447e705ccc5

Comment 59

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a447e705ccc5

Comment 60

[Brad Lassey [:blassey] (use needinfo?)]

Yoric, can you get this uplifted to 45?

Comment 61

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: Not enough details to determine the impact of e10s on Session Restore startup.
[Describe test coverage new/current, TreeHerder]: It has been on Nightly for a few days.
[Risks and why]: None that I can think of.
[String/UUID change made/needed]:

Comment 62

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8689471 [details]
MozReview Request: Bug 1198898 - Determining number of tabs/windows restored by Session Restore;r=mconley

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]:
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 63

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8675811 [details]
MozReview Request: Bug 1214158 - New module StartupPerformance to monitor the duration of restoration of initial tabs;r=mconley

Merci, taking it for the experiment

Comment 64

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4b8cf2401fd

Comment 65

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
STR: 
Measure the time to restore the previous session

Component: 
Name 			 Firefox
Version 		 46.0b9
Build ID 		 20160322075646
Update Channel 	         beta
User Agent 		 Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS                       Windows 7 SP1 x86_64

Expected Results: 
Pinned tabs should also behave in similar way as that of other tabs.


Actual Results: 
Let me share my experience.
1. Session restarting:
  a.Browser hangs mainly because of pinned tabs restoration and then selected tabs clicked 
    which are not pinned
  b.Though page does not get loaded in pinned tab; it constantly loads without any content which  
    sometimes contributes to increasing browser memory.
2. Browser crashed:
  a. This is option where one gets option whether to load previous tabs or not.
  b. So it gives reasonable speed up compared to previous case.