Closed Bug 1363078 Opened 3 years ago Closed 3 years ago

Refuse to insert lazy browsers in closed windows

Categories

(Firefox :: Tabbed Browser, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox55 --- fixed

People

(Reporter: u462496, Assigned: u462496)

References

Details

Attachments

(1 file, 10 obsolete files)

Attached patch 906076_tests_V3.diff (obsolete) — Splinter Review
No description provided.
Attachment #8865522 - Flags: feedback?(mdeboer)
Attachment #8865522 - Flags: feedback?(dao+bmo)
Attached patch 906076_tests_V4.diff (obsolete) — Splinter Review
Attachment #8865522 - Attachment is obsolete: true
Attachment #8865522 - Flags: feedback?(mdeboer)
Attachment #8865522 - Flags: feedback?(dao+bmo)
Attachment #8865525 - Flags: feedback?(mdeboer)
Attachment #8865525 - Flags: feedback?(dao+bmo)
Blocks: lazytabs
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.
Attachment #8865525 - Flags: feedback?(dao+bmo)
(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?
Flags: needinfo?(dao+bmo)
That listener unload would be on the chrome window, not a content window, so multi-process isn't going to make a difference.
Flags: needinfo?(dao+bmo)
(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.
Attached patch 906076_tests_V5.diff (obsolete) — Splinter Review
Ah, yes, that's much better.
Attachment #8865525 - Attachment is obsolete: true
Attachment #8865525 - Flags: feedback?(mdeboer)
Attachment #8865642 - Flags: review?(dao+bmo)
Attached patch 906076_tests_V6.diff (obsolete) — Splinter Review
Realized I had the wrong timeout in the last one.
Attachment #8865642 - Attachment is obsolete: true
Attachment #8865642 - Flags: review?(dao+bmo)
Attachment #8865644 - Flags: review?(dao+bmo)
Attached patch 906076_tests_V7.diff (obsolete) — Splinter Review
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.
Attachment #8865644 - Attachment is obsolete: true
Attachment #8865644 - Flags: review?(dao+bmo)
Attachment #8865661 - Flags: review?(dao+bmo)
Component: Untriaged → Tabbed Browser
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.
Attachment #8865661 - Flags: review?(dao+bmo)
Attached patch 906076_tests_V8.diff (obsolete) — Splinter Review
Update with requested changes.
Attachment #8865661 - Attachment is obsolete: true
Attachment #8866135 - Flags: review?(dao+bmo)
Attached patch 906076_tests_V9.diff (obsolete) — Splinter Review
Used arrow functions where possible.
Attachment #8866135 - Attachment is obsolete: true
Attachment #8866135 - Flags: review?(dao+bmo)
Attachment #8866194 - Flags: review?(dao+bmo)
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?
Attachment #8866194 - Flags: review?(dao+bmo)
Attached patch 906076_tests_V10.diff (obsolete) — Splinter Review
Update with requested changes.
Attachment #8866194 - Attachment is obsolete: true
Attachment #8866424 - Flags: review?(dao+bmo)
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?
Attachment #8866424 - Flags: review?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
(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.
Flags: needinfo?(dao+bmo)
Assignee: nobody → kevinhowjones
Attached patch 906076_tests_V11.diff (obsolete) — Splinter Review
Attachment #8866424 - Attachment is obsolete: true
Attachment #8866714 - Flags: review?(dao+bmo)
Comment on attachment 8866714 [details] [diff] [review]
906076_tests_V11.diff

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

nit: semicolon

Thanks!
Attachment #8866714 - Flags: review?(dao+bmo) → review+
Attached patch 906076_tests_V12.diff (obsolete) — Splinter Review
Attachment #8866714 - Attachment is obsolete: true
Attachment #8866720 - Flags: review?(dao+bmo)
Attachment #8866720 - Flags: review?(dao+bmo) → review+
Fixed ESLint errors.

I've really been running you ragged lately, haven't I Dao?  I'm sorry I haven't been more careful.
Attachment #8866720 - Attachment is obsolete: true
Attachment #8866915 - Flags: review?(dao+bmo)
Comment on attachment 8866915 [details] [diff] [review]
906076_tests_V13.diff

No worries!
Attachment #8866915 - Flags: review?(dao+bmo) → review+
Flags: in-testsuite+
Summary: Test for bug 906076 → Refuse to insert lazy browsers in closed windows
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/69a43060a9b9
Refuse to insert lazy browsers in closed windows. r=dao
https://hg.mozilla.org/mozilla-central/rev/69a43060a9b9
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
You need to log in before you can comment on or make changes to this bug.