The default bug view has changed. See this FAQ.

Try to reclaim session restore gains by restoring tabs lazily

RESOLVED FIXED in Firefox 55

Status

()

Firefox
Session Restore
RESOLVED FIXED
a year ago
5 hours ago

People

(Reporter: mconley, Assigned: mconley, NeedInfo)

Tracking

(Blocks: 1 bug, {perf})

unspecified
Firefox 55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments, 1 obsolete attachment)

Bug 1227275 was originally filed to track a regression that was opened against e10s. What happened was that the behaviour for creating background unrestored tabs used to be done in the content process. This was changed to load in the parent process instead (to avoid the possibility of crashing unrestored background tabs).

There was a nice win in session restore performance when we created those unrestored background tabs in the content process. We should try to do that again if we can to regain those wins.

So to be clear, there is no regression against e10s compared to non-e10s. These were wins that we had to give up, and we want to reclaim them.
Keywords: perf
See Also: → bug 1209689
Blocks: 1330635
Mike, is this still applicable?
Flags: needinfo?(mconley)
I think this work is kinda already underway in bug 906076, where Kevin Jones is working on making it possible to restore tabs "lazily" (without creating <xul:browser>'s at all).
Status: NEW → RESOLVED
Last Resolved: 14 days ago
Flags: needinfo?(mconley)
Resolution: --- → DUPLICATE
Duplicate of bug: 906076
I'm re-opening this because I _think_ we're at a point where we might be able to safely go back to the old model of restoring tabs in a content process.

In bug 1209689, I made it so that restore-on-demand tabs are loaded in the parent process. This makes it so that they cannot crash if the content process goes down.

In bug 1241459, I added infrastructure to allow users to send crash reports for tabs that have crashed in the background.

I believe I can use bug 1241459 as a springboard to "undo" bug 1209689 so that we can restore in the content process again, but if the content process for a pending tab goes down, we re-restore it immediately. At least, I _think_ we can do that.

For the purposes of session restore perceived performance, we should at least try.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Comment hidden (mozreview-request)
Most of this review request is for mikedeboer, but I'm hoping that Gijs can check my thinking on the wasSettingBrowserState bit, since I'm doing that for the userTypedValue stuff (in order to pass both browser_bug522545.js and browser_parentProcessRestoreHash.js).
Attachment #8846855 - Flags: review?(mdeboer)
Attachment #8846855 - Flags: review?(gijskruitbosch+bugs)
I actually think I've found a more robust way of doing this. I'll have patches up early tomorrow.
Assignee: nobody → mconley

Comment 7

10 days ago
I wait to test these changes. :)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Attachment #8846855 - Attachment is obsolete: true

Comment 11

9 days ago
mozreview-review
Comment on attachment 8847266 [details]
Bug 1256472 - Make sure checkEmptyPageOrigin checks the browser documentURI for about:blank along with the currentURI.

https://reviewboard.mozilla.org/r/120294/#review122192

Scary, but I think this makes sense...

::: browser/base/content/browser.js:6660
(Diff revision 1)
> +    let uriString = browser.documentURI ? browser.documentURI.spec
> +                                        : uri.spec;
> +    if ((uriString == "about:blank" && contentPrincipal.isNullPrincipal) ||

Nit:

```js
let uriToCheck = browser.documentURI || uri;
if ((uriToCheck.spec == ...) || ...) {
```
Attachment #8847266 - Flags: review?(gijskruitbosch+bugs) → review+

Comment 12

9 days ago
mozreview-review
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122190

I'm struggling to understand how this works. Which might be my cold-symptoms-addled brain, so please don't take it personally, but:
- it looks like some consumers now pass isRemotenessUpdate, some pass isRemotenessUpdateForNavigation, and some pass both. That's confusing. Logically, the latter implies the former, but that doesn't seem to be the case in the code. Maybe the latter should just be called "isNavigating" or something like that?
- you updated some callsites to now pass isRemotenessUpdateForNavigation where they weren't passing isRemotenessUpdate before
- you changed some code that checked this value to check the other thing

The result is a change in behaviour... but what kind of change? And why? This feels extra tricky because I know there are other uses of isRemotenessUpdate that this patch leaves alone. So what's the change in behaviour there?

Maybe this is just a convoluted way of asking for a rename of the new thing you're adding, and a longer commit message that spells it out more for dimwits like me. I tried looking at the middle commit which you didn't ask me to review to see if there was more context there, but it didn't help me, possibly because I don't know enough about session restore. Most concisely: what problem is this solving? What does "as opposed to sessionstore set state" really mean? We're clearly trying to distinguish 2 cases but I don't really understand which, or why - and AIUI the code really creates 4 cases (both flags, 2x 1 of them, or neither, being set to true).

::: browser/components/sessionstore/SessionStore.jsm:3688
(Diff revision 1)
>        new ViewSourceBrowser(browser);
>      }
>  
>      browser.messageManager.sendAsyncMessage("SessionStore:restoreTabContent",
> -      {loadArguments: aLoadArguments, isRemotenessUpdate});
> +      {loadArguments: aLoadArguments, isRemotenessUpdate,
> +       isRemotenessUpdateForNavigation: aIsRemotenessUpdateForNavigation});

My kingdom for an end to aArgs in JS.
Attachment #8847264 - Flags: review?(gijskruitbosch+bugs)

Comment 13

8 days ago
mozreview-review
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122506

I agree with Gijs here. On my part it's probably not really understanding what this commit gives us and why.
Attachment #8847264 - Flags: review?(mdeboer)

Comment 14

8 days ago
mozreview-review
Comment on attachment 8847265 [details]
Bug 1256472 - When restoring a window, initialize each browser tab as remote if possible.

https://reviewboard.mozilla.org/r/120292/#review122510

Almost there, I think! Thanks :)

::: browser/components/sessionstore/SessionStore.jsm:3218
(Diff revision 1)
>                                                    {skipAnimation: true,
> -                                                   forceNotRemote,
>                                                     userContextId});
>  
> +      if (reuseExisting) {
> +        this._maybeUpdateBrowserRemoteness({

If you change the options object signature to contain a tab, instead of a browser, you can make it return the tab and fold this clause together with the conditional above.

::: browser/components/sessionstore/SessionStore.jsm:3942
(Diff revision 1)
> -   *          true if the tab is going to have its content
> -   *          restored immediately by the caller.
> -   *
>     */
> -  _maybeUpdateBrowserRemoteness({ tabbrowser, tab,
> -                                  willRestoreImmediately }) {
> +  _maybeUpdateBrowserRemoteness({ tabbrowser, browser }) {
> +    let win = tabbrowser.ownerGlobal;

If you change this to just `tab`, can't you reference `tabbrowser` from it? I think there's `browser.ownerGlobal` - are they not the same?

::: browser/components/sessionstore/SessionStore.jsm
(Diff revision 1)
> -    // will soon be loading content.
> -    let willRestore = willRestoreImmediately ||
> -                      TabRestoreQueue.willRestoreSoon(tab);
> -
> -    if (browser.isRemoteBrowser && !willRestore) {
> -      tabbrowser.updateBrowserRemoteness(browser, false);

<3 - Even though you've worked on this specific method quite a bit, pouring sweat and tears in it, I do really appreciate having it dumbed down.
Attachment #8847265 - Flags: review?(mdeboer) → review-
(Assignee)

Comment 15

8 days ago
mozreview-review-reply
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122190

I guess I'm trying to make a distinction between two different reasons why restoreTabContent might be called after a remoteness flip. The two reasons are:

1. The user has navigated somewhere that's going to result in a remoteness flip. This is the common case.
2. SessionStore restores browser, window or tab state into a pre-existing set of <xul:browser>'s which might result in some remoteness flips. Here, the user hasn't technically navigated anywhere - the state is just being restored.

All of this is for the userTypedValue value. In the case of (1), we want to avoid setting the userTypedValue (just like you were doing with the `} else if (!data.isRemotenessUpdate) {`), but in (2) we _do_ want to set the userTypedValue in the event that we're setting state (using `SessionStore.setBrowserState`, `SessionStore.setWindowState` or `SessionStore.setTabState`) - even if that has resulted in a remoteness flip.

Ultimately, all of this is to pass two tests:

browser/base/components/sessionstore/bug_522545.js (the test_existingSHEnd_noClear test) and
browser/base/components/sessionstore/browser_parentProcessRestoreHash.js

browser_parentProcessRestoreHash.js exercises (1), and test_existingSHEnd_noClear in bug_522545.js exercises (2).

I had a previous revision of this patch (all kinda mixed in here: https://reviewboard.mozilla.org/r/119850/diff/1/) which took the opposite tack, and told content-sessionStore.js if restoreTabContent was being called due to us setting browser state. I ended up ditching that approach because the `SessionStoreInternal._browserSetState` property was only being set when setting the _whole_ browser state. We'd still have the same problem if we were just restoring a single window's state.

Does the above make sense? I agree that the name isn't optimal.

Any suggestions on how I can achieve the above distinction in a cleaner way? Question goes to both you and mikedeboer. :)

> My kingdom for an end to aArgs in JS.

Heh. :) Y'know, if somebody wrote a linter rule for that...
ni?ing Gijs and mikedeboer for comment 15.
Flags: needinfo?(mdeboer)
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 17

8 days ago
mozreview-review-reply
Comment on attachment 8847265 [details]
Bug 1256472 - When restoring a window, initialize each browser tab as remote if possible.

https://reviewboard.mozilla.org/r/120292/#review122510

> If you change the options object signature to contain a tab, instead of a browser, you can make it return the tab and fold this clause together with the conditional above.

Good idea, thanks. :)

Comment 18

8 days ago
mozreview-review
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122560

::: browser/components/sessionstore/SessionStore.jsm:910
(Diff revision 1)
>        case "SessionStore:restoreTabContentStarted":
>          if (browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE) {
>            // If a load not initiated by sessionstore was started in a
>            // previously pending tab. Mark the tab as no longer pending.
>            this.markTabAsRestoring(tab);
> -        } else if (!data.isRemotenessUpdate) {
> +        } else if (!data.isRemotenessUpdateForNavigation) {

OK, I think I understand now. So how about making `data.isRemotenessUpdate` hold a string?
So it'd be `false` if it's not a remoteness update and `"navigation"` for, well, navigation and so forth.
If it's doable to work with constants that both scripts can access, even better. It'll work like an enum. Bit-flags allowed :)

Comment 19

8 days ago
(In reply to Mike Conley (:mconley) (Catching up on reviews and needinfos) from comment #15)
> Comment on attachment 8847264 [details]
> Bug 1256472 - Add isRemotenessUpdateForNavigation to restoreTabContent
> message.
> 
> https://reviewboard.mozilla.org/r/120290/#review122190
> 
> I guess I'm trying to make a distinction between two different reasons why
> restoreTabContent might be called after a remoteness flip. The two reasons
> are:
> 
> 1. The user has navigated somewhere that's going to result in a remoteness
> flip. This is the common case.
> 2. SessionStore restores browser, window or tab state into a pre-existing
> set of <xul:browser>'s which might result in some remoteness flips. Here,
> the user hasn't technically navigated anywhere - the state is just being
> restored.

Are there other cases where this code is running (maybe cases where we restore sessionrestore content but don't do a remoteness flip? Or where this code triggers because of navigation without a remoteness flip?)? The fact that we have 1 bool now, and we're not content with that 1 bool, for 2 cases, makes me suspicious that there are other cases. :-)

What other cases exist that we're trying to cater for ultimately affects how I'd name things... Perhaps we need a 'reason' parameter that we set to const strings/ints? Or maybe we really do need 2 bools that have more orthogonal names, and we should check for specific combinations? (Ni for this part rather than the style stuff below)

> All of this is for the userTypedValue value. In the case of (1), we want to
> avoid setting the userTypedValue (just like you were doing with the `} else
> if (!data.isRemotenessUpdate) {`), but in (2) we _do_ want to set the
> userTypedValue in the event that we're setting state (using
> `SessionStore.setBrowserState`, `SessionStore.setWindowState` or
> `SessionStore.setTabState`) - even if that has resulted in a remoteness flip.
> 
> Ultimately, all of this is to pass two tests:
> 
> browser/base/components/sessionstore/bug_522545.js (the
> test_existingSHEnd_noClear test) and
> browser/base/components/sessionstore/browser_parentProcessRestoreHash.js
> 
> browser_parentProcessRestoreHash.js exercises (1), and
> test_existingSHEnd_noClear in bug_522545.js exercises (2).
> 
> I had a previous revision of this patch (all kinda mixed in here:
> https://reviewboard.mozilla.org/r/119850/diff/1/) which took the opposite
> tack, and told content-sessionStore.js if restoreTabContent was being called
> due to us setting browser state. I ended up ditching that approach because
> the `SessionStoreInternal._browserSetState` property was only being set when
> setting the _whole_ browser state. We'd still have the same problem if we
> were just restoring a single window's state.
> 
> Does the above make sense? I agree that the name isn't optimal.
> 
> Any suggestions on how I can achieve the above distinction in a cleaner way?
> Question goes to both you and mikedeboer. :)
> 
> > My kingdom for an end to aArgs in JS.
> 
> Heh. :) Y'know, if somebody wrote a linter rule for that...

I think there's still disagreement about removing it, and so people try to adhere to the style of the file they're modifying, but of course then we just end up with files with mixed style like these.

I'm fairly sure an eslint rule has already been written, but we don't use it (for these modules?) at the moment.
Flags: needinfo?(gijskruitbosch+bugs) → needinfo?(mconley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 23

8 days ago
mozreview-review-reply
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122560

> OK, I think I understand now. So how about making `data.isRemotenessUpdate` hold a string?
> So it'd be `false` if it's not a remoteness update and `"navigation"` for, well, navigation and so forth.
> If it's doable to work with constants that both scripts can access, even better. It'll work like an enum. Bit-flags allowed :)

You and Gijs seem to be in sync regarding some kind of "reason" being expressed for the remoteness update. :) I'll try that route, thanks.

Comment 24

8 days ago
mozreview-review
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122580

Brilliant! Hi-fives all-around, lads.

::: browser/components/sessionstore/SessionStore.jsm:3628
(Diff revision 2)
> +   * @param aWasNavigating
> +   *        true if the tab content is being restored after a call
> +   *        to navigateAndRestore.
>     */
> -  restoreTabContent: function (aTab, aLoadArguments = null, aReloadInFreshProcess = false) {
> +  restoreTabContent: function (aTab, aLoadArguments = null, aReloadInFreshProcess = false,
> +                               aWasNavigating = false) {

Yeah, now my perk is that I get to choose for sessionstore specifically: if you feel like it, please remove the `aSomethingSomething` style from this method.
If not, we'll do it later ;-)

::: browser/components/sessionstore/content/content-sessionStore.js:209
(Diff revision 2)
>        // since it restores a max of MAX_CONCURRENT_TAB_RESTORES at a time.
>        sendAsyncMessage("SessionStore:restoreTabContentComplete", {epoch, isRemotenessUpdate});
>      });
>  
> -    sendAsyncMessage("SessionStore:restoreTabContentStarted", {epoch, isRemotenessUpdate});
> +    sendAsyncMessage("SessionStore:restoreTabContentStarted", {
> +      epoch,

nit: if it were me, I'd put the properties on a single line.
Attachment #8847264 - Flags: review+

Comment 25

8 days ago
mozreview-review
Comment on attachment 8847265 [details]
Bug 1256472 - When restoring a window, initialize each browser tab as remote if possible.

https://reviewboard.mozilla.org/r/120292/#review122584

r=me with comment addressed and a green try run! Thanks, Mike!

::: browser/components/sessionstore/SessionStore.jsm:3932
(Diff revision 2)
> -   *        willRestoreImmediately (bool):
> -   *          true if the tab is going to have its content
> -   *          restored immediately by the caller.
> -   *
>     */
> -  _maybeUpdateBrowserRemoteness({ tabbrowser, tab,
> +  _maybeUpdateBrowserRemoteness({ tab }) {

Please don't forget to make this just `tab`, not `{ tab }`
Attachment #8847265 - Flags: review?(mdeboer) → review+
Flags: needinfo?(mdeboer)
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

Heh, I was actually going to go with the "reason" route, but I only read that after I had posted this patch. I still think the "reason" route is a better, more explicit one, so I'll try that instead.

Thanks for the r+ though!
Attachment #8847264 - Flags: review+
(In reply to :Gijs from comment #19)
> 
> Are there other cases where this code is running (maybe cases where we
> restore sessionrestore content but don't do a remoteness flip? Or where this
> code triggers because of navigation without a remoteness flip?)? The fact
> that we have 1 bool now, and we're not content with that 1 bool, for 2
> cases, makes me suspicious that there are other cases. :-)
> 
> What other cases exist that we're trying to cater for ultimately affects how
> I'd name things... Perhaps we need a 'reason' parameter that we set to const
> strings/ints? Or maybe we really do need 2 bools that have more orthogonal
> names, and we should check for specific combinations? (Ni for this part
> rather than the style stuff below)
> 

Yeah, that jives with what mikedeboer was asking for. I'll start passing a reason around. Thanks!
Flags: needinfo?(mconley)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 31

7 days ago
mozreview-review
Comment on attachment 8847264 [details]
Bug 1256472 - Add a reason to the restoreTabContent function and message.

https://reviewboard.mozilla.org/r/120290/#review122938

::: browser/components/sessionstore/SessionStore.jsm:930
(Diff revision 3)
>        case "SessionStore:restoreTabContentStarted":
>          if (browser.__SS_restoreState == TAB_STATE_NEEDS_RESTORE) {
>            // If a load not initiated by sessionstore was started in a
>            // previously pending tab. Mark the tab as no longer pending.
>            this.markTabAsRestoring(tab);
> -        } else if (!data.isRemotenessUpdate) {
> +        } else if (!data.reason == RESTORE_TAB_CONTENT_REASON.NAVIGATE_AND_RESTORE) {

Uh, I think you meant `data.reason != ...)` :-)
Attachment #8847264 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #31)
> > +        } else if (!data.reason == RESTORE_TAB_CONTENT_REASON.NAVIGATE_AND_RESTORE) {
> 
> Uh, I think you meant `data.reason != ...)` :-)

LOL, I recommend doing a try push before landing, Mike ;-P

Comment 33

7 days ago
(In reply to Mike de Boer [:mikedeboer] from comment #32)
> (In reply to :Gijs from comment #31)
> > > +        } else if (!data.reason == RESTORE_TAB_CONTENT_REASON.NAVIGATE_AND_RESTORE) {
> > 
> > Uh, I think you meant `data.reason != ...)` :-)
> 
> LOL, I recommend doing a try push before landing, Mike ;-P

Well, the effect is actually the same here because the values of data.reason are 0 and 1. So it doesn't matter while we have 2 values, but obviously the comparison should be correct even if/when we add other reasons. :-)
(In reply to :Gijs from comment #31)
> Uh, I think you meant `data.reason != ...)` :-)

How embarrassing! And that's why we review. :) Good catch!

(In reply to Mike de Boer [:mikedeboer] from comment #32)
> LOL, I recommend doing a try push before landing, Mike ;-P

Good call. In my defense, I _did_ do a try push (https://treeherder.mozilla.org/#/jobs?repo=try&revision=afc58dc35c2fc89b64eb0788d9f823ee6e97b960), and it came back green - but as Gijs points out in comment 33, it's not detecting the brittleness of my patch. :)

Change coming up! Thanks you too!
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 38

7 days ago
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 2b9a37562a05 -d 300754a3b772: rebasing 382365:2b9a37562a05 "Bug 1256472 - Add a reason to the restoreTabContent function and message. r=Gijs,mikedeboer"
merging browser/components/sessionstore/SessionStore.jsm
merging browser/components/sessionstore/content/content-sessionStore.js
warning: conflicts while merging browser/components/sessionstore/SessionStore.jsm! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 42

7 days ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ba60dcd190ad
Add a reason to the restoreTabContent function and message. r=Gijs,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/27f5fcaca7fa
When restoring a window, initialize each browser tab as remote if possible. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/010ca7d23406
Make sure checkEmptyPageOrigin checks the browser documentURI for about:blank along with the currentURI. r=Gijs
Backed out for leaks in browser_windowStateContainer.js:

https://hg.mozilla.org/integration/autoland/rev/7c68b66341e4ae1df0b2b021e6ba73bd7aa6b69a
https://hg.mozilla.org/integration/autoland/rev/41820294ab73fbb62ddd0c1f902e4fee30d6e77e
https://hg.mozilla.org/integration/autoland/rev/56ea6de066f46732ecd83080ca6e6dcc4d149d5a

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=010ca7d234069da78c57b17df8f8624e680b30b4&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=84331199&repo=autoland

08:58:47     INFO - TEST-START | Shutdown
08:58:47     INFO - Browser Chrome Test Summary
08:58:47     INFO - Passed:  1450
08:58:47     INFO - Failed:  0
08:58:47     INFO - Todo:    0
08:58:47     INFO - Mode:    e10s
08:58:47     INFO - *** End BrowserChrome Test Results ***
08:58:47     INFO - GECKO(1786) | --DOCSHELL 0x12667e800 == 4 [pid = 1786] [id = {4d999a53-ecf9-714a-adb9-3c80f59da37b}]
08:58:47     INFO - GECKO(1786) | --DOCSHELL 0x12c62a000 == 3 [pid = 1786] [id = {4aa93437-c40e-0b45-9bd3-d564e3d1c06c}]
08:58:47     INFO - GECKO(1786) | --DOCSHELL 0x121a1a000 == 2 [pid = 1786] [id = {93169096-86f4-0d43-a896-3b4c2384f88f}]
08:58:48     INFO - GECKO(1786) | --DOCSHELL 0x126680800 == 1 [pid = 1786] [id = {a0811c19-08b9-ec47-b699-8cd48f8b90f8}]
08:58:48     INFO - GECKO(1786) | --DOCSHELL 0x119e22000 == 0 [pid = 1786] [id = {916f22b1-beb8-9a4a-9b4a-3cc87ec064af}]
08:58:48     INFO - GECKO(1786) | [Child 1822] WARNING: nsAppShell::Exit() called redundantly: file /home/worker/workspace/build/src/widget/cocoa/nsAppShell.mm, line 684
08:58:48     INFO - GECKO(1786) | [Child 1822] WARNING: NS_ENSURE_TRUE(maybeContext) failed: file /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp, line 1014
08:58:48     INFO - GECKO(1786) | [Child 1822] WARNING: NS_ENSURE_TRUE(aObserver) failed: file /home/worker/workspace/build/src/modules/libpref/nsPrefBranch.cpp, line 715
08:58:48     INFO - GECKO(1786) | [Child 1822] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/worker/workspace/build/src/modules/libpref/Preferences.cpp, line 1709
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: nsAppShell::Exit() called redundantly: file /home/worker/workspace/build/src/widget/cocoa/nsAppShell.mm, line 684
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: NS_ENSURE_TRUE(maybeContext) failed: file /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp, line 1014
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: NS_ENSURE_TRUE(maybeContext) failed: file /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp, line 1014
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: NS_ENSURE_TRUE(aObserver) failed: file /home/worker/workspace/build/src/modules/libpref/nsPrefBranch.cpp, line 715
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/worker/workspace/build/src/modules/libpref/Preferences.cpp, line 1709
08:58:48     INFO - GECKO(1786) | --DOCSHELL 0x1171f0000 == 0 [pid = 1818] [id = {0d7646f9-66b5-3e4d-a10e-a938c6c81dab}]
08:58:48     INFO - GECKO(1786) | [Child 1822] WARNING: '!compMgr', file /home/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp, line 63
08:58:48     INFO - GECKO(1786) | nsStringStats
08:58:48     INFO - GECKO(1786) |  => mAllocCount:          26194
08:58:48     INFO - GECKO(1786) |  => mReallocCount:          527
08:58:48     INFO - GECKO(1786) |  => mFreeCount:           26194
08:58:48     INFO - GECKO(1786) |  => mShareCount:          22482
08:58:48     INFO - GECKO(1786) |  => mAdoptCount:           3989
08:58:48     INFO - GECKO(1786) |  => mAdoptFreeCount:       3989
08:58:48     INFO - GECKO(1786) |  => Process ID: 1822, Thread ID: 140735244833536
08:58:48     INFO - GECKO(1786) | --DOMWINDOW == 3 (0x117aee000) [pid = 1818] [serial = 430] [outer = 0x0] [url = about:blank]
08:58:48     INFO - GECKO(1786) | --DOMWINDOW == 2 (0x12a9cec00) [pid = 1818] [serial = 442] [outer = 0x0] [url = http://example.com/]
08:58:48     INFO - GECKO(1786) | [Parent 1786] WARNING: NS_ENSURE_TRUE(mDB) failed: file /home/worker/workspace/build/src/netwerk/cache/nsDiskCacheDeviceSQL.cpp, line 1426
08:58:48     INFO - GECKO(1786) | [NPAPI 1789] WARNING: '!compMgr', file /home/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp, line 52
08:58:48     INFO - GECKO(1786) | [Parent 1786] WARNING: '!aObserver', file /home/worker/workspace/build/src/xpcom/ds/nsObserverService.cpp, line 234
08:58:48     INFO - GECKO(1786) | nsStringStats
08:58:48     INFO - GECKO(1786) |  => mAllocCount:             88
08:58:48     INFO - GECKO(1786) |  => mReallocCount:            1
08:58:48     INFO - GECKO(1786) |  => mFreeCount:              88
08:58:48     INFO - GECKO(1786) |  => mShareCount:            231
08:58:48     INFO - GECKO(1786) |  => mAdoptCount:              0
08:58:48     INFO - GECKO(1786) |  => mAdoptFreeCount:          0
08:58:48     INFO - GECKO(1786) |  => Process ID: 1789, Thread ID: 140735244833536
08:58:48     INFO - GECKO(1786) | [Parent 1786] WARNING: nsAppShell::Exit() called redundantly: file /home/worker/workspace/build/src/widget/cocoa/nsAppShell.mm, line 684
08:58:48     INFO - GECKO(1786) | --DOMWINDOW == 1 (0x11fd56800) [pid = 1818] [serial = 468] [outer = 0x0] [url = about:blank]
08:58:48     INFO - GECKO(1786) | --DOMWINDOW == 0 (0x115ca8000) [pid = 1818] [serial = 467] [outer = 0x0] [url = about:blank]
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!mMainThread', file /home/worker/workspace/build/src/xpcom/threads/nsThreadManager.cpp, line 314
08:58:48     INFO - GECKO(1786) | [Child 1818] WARNING: '!compMgr', file /home/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp, line 63
08:58:48     INFO - GECKO(1786) | nsStringStats
08:58:48     INFO - GECKO(1786) |  => mAllocCount:         315296
08:58:48     INFO - GECKO(1786) |  => mReallocCount:         8900
08:58:48     INFO - GECKO(1786) |  => mFreeCount:          315296
08:58:48     INFO - GECKO(1786) |  => mShareCount:         396884
08:58:48     INFO - GECKO(1786) |  => mAdoptCount:          83001
08:58:48     INFO - GECKO(1786) |  => mAdoptFreeCount:      83001
08:58:48     INFO - GECKO(1786) |  => Process ID: 1818, Thread ID: 140735244833536
08:58:49     INFO - GECKO(1786) | [Parent 1786] WARNING: NS_ENSURE_TRUE(maybeContext) failed: file /home/worker/workspace/build/src/xpcom/threads/nsThread.cpp, line 1014
08:58:49     INFO - GECKO(1786) | [Parent 1786] WARNING: 'NS_FAILED(RemovePermissionChangeObserver())', file /home/worker/workspace/build/src/dom/notification/Notification.cpp, line 667
08:58:51     INFO - GECKO(1786) | [Parent 1786] WARNING: NS_ENSURE_TRUE(aObserver) failed: file /home/worker/workspace/build/src/modules/libpref/nsPrefBranch.cpp, line 715
08:58:51     INFO - GECKO(1786) | [Parent 1786] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057: file /home/worker/workspace/build/src/modules/libpref/Preferences.cpp, line 1709
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 9 (0x122b0e800) [pid = 1786] [serial = 4] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 8 (0x119e22800) [pid = 1786] [serial = 3] [outer = 0x0] [url = chrome://browser/content/browser.xul]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 7 (0x12c62c000) [pid = 1786] [serial = 11] [outer = 0x0] [url = chrome://mochikit/content/browser-harness.xul]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 6 (0x12c62d000) [pid = 1786] [serial = 12] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 5 (0x121a1b800) [pid = 1786] [serial = 2] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 4 (0x12667f000) [pid = 1786] [serial = 5] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 3 (0x119b1c000) [pid = 1786] [serial = 9] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 2 (0x126681000) [pid = 1786] [serial = 6] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 1 (0x1291c0800) [pid = 1786] [serial = 1752] [outer = 0x0] [url = about:blank]
08:58:51     INFO - GECKO(1786) | --DOMWINDOW == 0 (0x121a1a800) [pid = 1786] [serial = 1] [outer = 0x0] [url = chrome://browser/content/hiddenWindow.xul]
08:58:51     INFO - GECKO(1786) | [Parent 1786] WARNING: '!compMgr', file /home/worker/workspace/build/src/xpcom/components/nsComponentManagerUtils.cpp, line 63
08:58:51     INFO - GECKO(1786) | nsStringStats
08:58:51     INFO - GECKO(1786) |  => mAllocCount:        3467380
08:58:51     INFO - GECKO(1786) |  => mReallocCount:       357551
08:58:51     INFO - GECKO(1786) |  => mFreeCount:         3467380
08:58:51     INFO - GECKO(1786) |  => mShareCount:        3739639
08:58:51     INFO - GECKO(1786) |  => mAdoptCount:         231545
08:58:51     INFO - GECKO(1786) |  => mAdoptFreeCount:     231545
08:58:51     INFO - GECKO(1786) |  => Process ID: 1786, Thread ID: 140735244833536
08:58:51     INFO - TEST-INFO | Main app process: exit 0
08:58:51    ERROR - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_windowStateContainer.js | leaked 2 window(s) until shutdown [url = http://example.com/]
08:58:51    ERROR - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_windowStateContainer.js | leaked 1 window(s) until shutdown [url = about:blank]
Flags: needinfo?(mconley)
See Also: → bug 1096013
https://treeherder.mozilla.org/#/jobs?repo=try&revision=9682b52f2eb8
https://treeherder.mozilla.org/#/jobs?repo=try&revision=377a6057f5cb
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 48

3 days ago
mozreview-review
Comment on attachment 8848774 [details]
Bug 1256472 - Make ShutdownLeaksCollector do more aggressive GCing and CCing to avoid erroneous shutdown leak reports in tests.

https://reviewboard.mozilla.org/r/121654/#review124028

::: testing/mochitest/ShutdownLeaksCollector.jsm:44
(Diff revision 1)
>  
> +        let shutdownCleanup = aCallback => {
> -        Cu.schedulePreciseShrinkingGC(() => {
> +          Cu.schedulePreciseShrinkingGC(() => {
> +            // Run the GC and CC a few times to make sure that as much
> +            // as possible is freed.
> +            let numCycles = 3;

This is a lot of GCing and CCing. We GC+CC at least once for the memory pressure already, then you are GCing another 4 times and CCing 3 times each time you call shutdownCleanup, and then you call that twice for a total of 9 GCs and 7 CCs! On every test. Do you really need this many?

Please do some try testing to see if this causes horrible increases in test times, or causes more frequent timeouts. It looks like your try runs have a ton of timeouts.

I guess I see what you mean about this just being the same as the parent process, but I don't think it is a great idea to just copy it without trying out some smaller values.

Comment 49

3 days ago
mozreview-review
Comment on attachment 8848773 [details]
Bug 1256472 - Account for the possibility that the selectedTab has not yet presented when initting async tab switcher.

https://reviewboard.mozilla.org/r/121652/#review124070
Attachment #8848773 - Flags: review?(wmccloskey) → review+

Comment 50

3 days ago
mozreview-review
Comment on attachment 8848774 [details]
Bug 1256472 - Make ShutdownLeaksCollector do more aggressive GCing and CCing to avoid erroneous shutdown leak reports in tests.

https://reviewboard.mozilla.org/r/121654/#review124144
Attachment #8848774 - Flags: review?(continuation) → review+
https://treeherder.mozilla.org/#/jobs?repo=try&revision=451c946a92da
(Assignee)

Comment 52

2 days ago
mozreview-review-reply
Comment on attachment 8848774 [details]
Bug 1256472 - Make ShutdownLeaksCollector do more aggressive GCing and CCing to avoid erroneous shutdown leak reports in tests.

https://reviewboard.mozilla.org/r/121654/#review124028

> This is a lot of GCing and CCing. We GC+CC at least once for the memory pressure already, then you are GCing another 4 times and CCing 3 times each time you call shutdownCleanup, and then you call that twice for a total of 9 GCs and 7 CCs! On every test. Do you really need this many?
> 
> Please do some try testing to see if this causes horrible increases in test times, or causes more frequent timeouts. It looks like your try runs have a ton of timeouts.
> 
> I guess I see what you mean about this just being the same as the parent process, but I don't think it is a great idea to just copy it without trying out some smaller values.

Yeah, that's a good call. Apparently, I can side-step the intermittent leak report by doing a single GC / CC pass instead of 3 cycles.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 58

2 days ago
Pushed by mconley@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbfcb54b0d56
Add a reason to the restoreTabContent function and message. r=Gijs,mikedeboer
https://hg.mozilla.org/integration/autoland/rev/46d75f535951
When restoring a window, initialize each browser tab as remote if possible. r=mikedeboer
https://hg.mozilla.org/integration/autoland/rev/b0b9e64f6edf
Make sure checkEmptyPageOrigin checks the browser documentURI for about:blank along with the currentURI. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/32aa11934b3a
Account for the possibility that the selectedTab has not yet presented when initting async tab switcher. r=billm
https://hg.mozilla.org/integration/autoland/rev/0975e301ce64
Make ShutdownLeaksCollector do more aggressive GCing and CCing to avoid erroneous shutdown leak reports in tests. r=mccr8

Comment 59

2 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/dbfcb54b0d56
https://hg.mozilla.org/mozilla-central/rev/46d75f535951
https://hg.mozilla.org/mozilla-central/rev/b0b9e64f6edf
https://hg.mozilla.org/mozilla-central/rev/32aa11934b3a
https://hg.mozilla.org/mozilla-central/rev/0975e301ce64
Status: REOPENED → RESOLVED
Last Resolved: 14 days ago2 days ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55

Comment 60

21 hours ago
\o/ I'm seeing significantly faster startup times now. < 1 minute now where it took several before.
Confirmed. Restored a profile with 300+ tabs. Browser UI was up and visible *minutes* sooner than normal.

Nice work everyone, thanks so much.

Oh wait, I haven't tried my 1800+ tab profile yet...
You need to log in before you can comment on or make changes to this bug.