1363078 - Refuse to insert lazy browsers in closed windows

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[u462496]

Attachment: 906076_tests_V3.diff

Comment 1

[u462496]

Attachment: 906076_tests_V4.diff

Comment 2

[Dão Gottwald [:dao]]

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.

Comment 3

[u462496]

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

Comment 4

[Dão Gottwald [:dao]]

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

Comment 5

[Dão Gottwald [:dao]]

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

Comment 6

[u462496]

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

Comment 7

[Dão Gottwald [:dao]]

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.

Comment 8

[u462496]

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

Comment 9

[u462496]

Attachment: 906076_tests_V5.diff

Ah, yes, that's much better.

Comment 10

[u462496]

Attachment: 906076_tests_V6.diff

Realized I had the wrong timeout in the last one.

Comment 11

[u462496]

Attachment: 906076_tests_V7.diff

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 12

[Dão Gottwald [:dao]]

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.

Comment 13

[u462496]

Attachment: 906076_tests_V8.diff

Update with requested changes.

Comment 14

[u462496]

Attachment: 906076_tests_V9.diff

Used arrow functions where possible.

Comment 15

[Dão Gottwald [:dao]]

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?

Comment 16

[u462496]

Attachment: 906076_tests_V10.diff

Update with requested changes.

Comment 17

[Dão Gottwald [:dao]]

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?

Comment 18

[u462496]

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

Comment 19

[Dão Gottwald [:dao]]

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

[u462496]

Attachment: 906076_tests_V11.diff

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8866714 [details] [diff] [review]
906076_tests_V11.diff

>+  yield TestUtils.topicObserved("sessionstore-state-write-complete")

nit: semicolon

Thanks!

Comment 22

[u462496]

Attachment: 906076_tests_V12.diff

Comment 23

[Dão Gottwald [:dao]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=edf9dc429c0575ed542520f0051c5fe3a5faf9c0

Comment 24

[u462496]

Attachment: 906076_tests_V13.diff

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 25

[Dão Gottwald [:dao]]

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

No worries!

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a43060a9b9
Refuse to insert lazy browsers in closed windows. r=dao

Comment 27

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/69a43060a9b9