Comment on attachment 8865525 [details] [diff] [review] 906076_tests_V4.diff >+ ok(!lazyTabWasInstantiated, "No lazy browsers were prematurely inserted.\n"); The message shouldn't end with . nor \n Can you just use an unload event listener to check browser states? Otherwise, if you keep using the LazyTabPrematurelyInserted event, you'll also want to add a test for that event being dispatched when it should be, otherwise if we break both the event and lazy browsers, your test will still pass.

(In reply to Dão Gottwald [::dao] from comment #2) > Comment on attachment 8865525 [details] [diff] [review] > 906076_tests_V4.diff > > Can you just use an unload event listener to check browser states? I'm not seeing how that would work for multi-process. What would you use for a target?

That listener unload would be on the chrome window, not a content window, so multi-process isn't going to make a difference.

(In reply to Dão Gottwald [::dao] from comment #4) > That listener unload would be on the chrome window, not a content window, so > multi-process isn't going to make a difference. Read this as "That unload listener..." :) Basically, add an unload listener and have it check the window's browser.isConnected states.

(In reply to Dão Gottwald [::dao] from comment #5) > (In reply to Dão Gottwald [::dao] from comment #4) > > That listener unload would be on the chrome window, not a content window, so > > multi-process isn't going to make a difference. > > Read this as "That unload listener..." :) > > Basically, add an unload listener and have it check the window's > browser.isConnected states. Do we know for sure that that unload event would only fire after any possibility of some code instantiating browsers?

It would have caught bug 1360940. I think it's close enough, although it's probably possible to mess with browsers after that. To improve this, you could either: - check window.XULBrowserWindow == null in your test to make sure that your unload listener runs after gBrowserInit.onUnload, or - make tabbrowser refuse inserting browsers if window.closed.

(In reply to Dão Gottwald [::dao] from comment #7) > It would have caught bug 1360940. I think it's close enough, although it's > probably possible to mess with browsers after that. To improve this, you > could either: > - check window.XULBrowserWindow == null in your test to make sure that your > unload listener runs after gBrowserInit.onUnload, or > - make tabbrowser refuse inserting browsers if window.closed. Option B sounds like a good idea just in principle.

Ah, yes, that's much better.

Realized I had the wrong timeout in the last one.

I added a check that a lazy browser can't be inserted after the window is closed. Also checkTabs helper function now returns a boolean instead of displaying info.

Comment on attachment 8865661 [details] [diff] [review] 906076_tests_V7.diff >+Cu.import("resource://gre/modules/Services.jsm", this); Already available here, don't need to import. >+function waitForObserver(topic, timeout) { You can use waitForTopic instead. >+add_task(function* test() { >+ Services.prefs.setBoolPref("browser.sessionstore.restore_on_demand", true); >+ Services.prefs.setBoolPref("browser.sessionstore.restore_tabs_lazily", true); >+ >+ let backupState = SessionStore.getBrowserState(); >+ let obsRetval; >+ >+ SessionStore.setBrowserState(JSON.stringify(TEST_STATE)); >+ obsRetval = yield waitForObserver("sessionstore-state-write-complete", 30000); >+ ok(obsRetval, "Sessionstore write completed before timeout"); >+ >+ info("Check that no lazy browsers get inserted after sessionstore write"); >+ ok(checkTabs(), "window has only 1 non-lazy tab"); Please let checkTabs return the number of connected tabs and use is(checkTabs(), 1, ...); here. >+ yield waitForTimeout(30000); >+ >+ info("Check that no lazy browsers get inserted after 30 second delay"); This will make the test take too long to finish. It's also pretty arbitrary. Should probably just drop this part. >+ // Check if any lazy tabs got inserted when window closes. >+ window.open(); >+ let newWindow = Services.wm.getMostRecentWindow("navigator:browser"); Please use promiseNewWindowLoaded. >+ SessionStore.setWindowState(newWindow, JSON.stringify(TEST_STATE)); >+ obsRetval = yield waitForObserver("sessionstore-state-write-complete", 30000); >+ ok(obsRetval, "Sessionstore write completed before timeout"); >+ >+ let tabbrowser = newWindow.gBrowser; >+ let tabContainer = tabbrowser.tabContainer; >+ >+ newWindow.addEventListener("unload", function onWindowUnload() { >+ tabContainer.removeEventListener("unload", onWindowUnload); >+ >+ info("Check that no lazy browsers get inserted when window closes"); >+ ok(checkTabs(newWindow), "window has only 1 non-lazy tab"); >+ >+ info("Check that it is not possible to insert a lazy browser after the window closed"); >+ newWindow.gBrowser._insertBrowser(newWindow.gBrowser.tabs[1]); Can you also add a test for _insertBrowser working as expected when the window hasn't closed yet? Instead of removing the event listener, please use newWindow.addEventListener("unload", () => {}, { once: true }); >+ yield BrowserTestUtils.domWindowClosed(newWindow); Instead of this, please use a promise that the unload listener resolves.

Update with requested changes.

Used arrow functions where possible.

Comment on attachment 8866194 [details] [diff] [review] 906076_tests_V9.diff >+++ b/browser/components/sessionstore/test/browser_906076_lazy_browser_tabs.js nit: just browser_906076_lazy_browsers.js or browser_906076_lazy_tabs.js will be fine >+function checkForNonLazyTabs(win) { nit: countNonLazyTabs or countNonLazyBrowsers >+ win = win || window; >+ let tabs = win.gBrowser.tabs; >+ >+ let count = 0; >+ for (let tab of tabs) { >+ if (tab.linkedBrowser.isConnected) { Use win.gBrowser.browsers? >+ Services.prefs.setBoolPref("browser.sessionstore.restore_on_demand", true); >+ Services.prefs.setBoolPref("browser.sessionstore.restore_tabs_lazily", true); You should reset these prefs when you're done with the test. SpecialPowers.pushPrefEnv will do this for you automatically. >+ info("Check that lazy browser gets inserted properly"); >+ ok(!gBrowser.browsers[1].isConnected, "The browser that we're attempting to insert is indeed lazy"); nit: too much indentation >+ SessionStore.setWindowState(newWindow, JSON.stringify(TEST_STATE)); >+ yield new Promise((resolve, reject) => { >+ waitForTopic("sessionstore-state-write-complete", 30 * 1000, success => { >+ ok(success, "Sessionstore write completed before timeout"); >+ resolve(); >+ }); >+ }); Use promiseBrowserState?

Update with requested changes.

Comment on attachment 8866424 [details] [diff] [review] 906076_tests_V10.diff >+ let obsRetval; unused variable >+ promiseBrowserState(TEST_STATE); Need to use yield here. >+ SessionStore.setWindowState(newWindow, JSON.stringify(TEST_STATE)); >+ yield new Promise((resolve, reject) => { >+ waitForTopic("sessionstore-state-write-complete", 30 * 1000, success => { >+ ok(success, "Sessionstore write completed before timeout"); >+ resolve(); >+ }); >+ }); I don't think sessionstore-state-write-complete is the right notification to wait for here. How about sessionstore-windows-restored or sessionstore-single-window-restored?

(In reply to Dão Gottwald [::dao] from comment #17) > Comment on attachment 8866424 [details] [diff] [review] > 906076_tests_V10.diff > > I don't think sessionstore-state-write-complete is the right notification to > wait for here. How about sessionstore-windows-restored or > sessionstore-single-window-restored? I am specifically waiting for sessionstore-state-write-complete here. A lot of stuff happens in TabStateCache and others when a write happens, and I want to ensure nothing inserts the browser.

(In reply to Kevin Jones from comment #18) > (In reply to Dão Gottwald [::dao] from comment #17) > > Comment on attachment 8866424 [details] [diff] [review] > > 906076_tests_V10.diff > > > > I don't think sessionstore-state-write-complete is the right notification to > > wait for here. How about sessionstore-windows-restored or > > sessionstore-single-window-restored? > > I am specifically waiting for sessionstore-state-write-complete here. A lot > of stuff happens in TabStateCache and others when a write happens, and I > want to ensure nothing inserts the browser. Hmm, okay. Please add a comment in the test to that end. And let's use TestUtils.topicObserved, which seems nicer than waitForTopic.

Comment on attachment 8866714 [details] [diff] [review] 906076_tests_V11.diff >+ yield TestUtils.topicObserved("sessionstore-state-write-complete") nit: semicolon Thanks!

Fixed ESLint errors. I've really been running you ragged lately, haven't I Dao? I'm sorry I haven't been more careful.

Comment on attachment 8866915 [details] [diff] [review] 906076_tests_V13.diff No worries!

Pushed by dgottwald@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/69a43060a9b9 Refuse to insert lazy browsers in closed windows. r=dao