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.

Mike, is this still applicable?

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).

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.

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).

I actually think I've found a more robust way of doing this. I'll have patches up early tomorrow.

I wait to test these changes. :)

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 == ...) || ...) { ```

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.

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.

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.

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.

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 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 :)

(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.

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 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.

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 }`

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!

(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!

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 != ...)` :-)

(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

(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!

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)

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]

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 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

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

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.

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

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

\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...

(In reply to The 8472 from comment #60) > \o/ I'm seeing significantly faster startup times now. < 1 minute now where > it took several before. (In reply to Dietrich Ayala (:dietrich) from comment #61) > Confirmed. Restored a profile with 300+ tabs. Browser UI was up and visible > *minutes* sooner than normal. > > Nice work everyone, thanks so much. > \o/

@ Mike Conley (:mconley) - Awesome job! As tab hoarder thank you very much!

June cannot come soon enough for those on beta channel

This results in all content processes starting up to the limit, if you have a bunch of unloaded tabs as I usually do. I don't think that's healthy. And it appears to be actually slower in my case with a severe impact on memory usage (and cpu usage at startup). dom.ipc.processCount x 77MB (in my case it's 44*77MB) is wasted instantly with a single tab loaded. Also, it looks like this brought up some severe issues with video playback to me: https://bugzilla.mozilla.org/show_bug.cgi?id=1349991#c5 Plus, it's only now I realize that it's not just video playback but any page loading results in all content processes starting to consume cpu time. The ones which have no loaded tabs around 1%, which adds up. So you'll might get all your cores maxed out for quite a while, especially on heavier, slow-loading pages. So apart from having all those content processes open pointlessly for unloaded tabs, they actually severely decrease performance, by eating up cpu time without any reason. I realize that the default value of dom.ipc.processCount is 4. But that said, I've been using many content processes for many months (years perhaps), but it's effectively impossible now, with the recent changes. And I remember one process per tab being an ultimate goal.

frankly speaking, tough luck. you chose to use an excessive amount of content processes and deviating outside of the developers design, so you don't get to complain about issues that come of it.

(In reply to Danial Horton from comment #66) > frankly speaking, tough luck. > > you chose to use an excessive amount of content processes and deviating > outside of the developers design, so you don't get to complain about issues > that come of it. You don't get to decide.

indeed, but don't expect the developers to punish those of us who the change is a good thing for because your choice of settings no longer works well for you.

(In reply to Danial Horton from comment #68) > indeed, but don't expect the developers to punish those of us who the change > is a good thing for because your choice of settings no longer works well for > you. They might eventually come up with something more efficient than starting all content processes. On the other hand the content processes being active with tabs not even being loaded is clearly a bug. If the devs don't know about these issues they will certainly not improve...

I had that once on the first start after installing the 55 build, and no issues after a restart. There is work to lazy restore tabs, instead of binding them to a content or the main process. This change is just an interim win on session starts.

(In reply to Danial Horton from comment #70) > I had that once on the first start after installing the 55 build, and no > issues after a restart. > > There is work to lazy restore tabs, instead of binding them to a content or > the main process. This change is just an interim win on session starts. Do you know any bug numbers?

bug 906076 bug 776928 bug 1243707

woops, bug 776928 is unrelated.