862371 - Create a test that checks that private windows are not restored from normal windows

Status: RESOLVED FIXED

(Firefox :: Session Restore, defect)

Description

[Ioana (away)]

STR:

1. Launch Firefox.
2. Open a private window (Ctrl+Shift+P / Cmd+Shift+P)
3. Load a website in the private window.
4. Close the private window (Ctrl+W / Cmd+W)
5. While in Normal Broswing try to reopen the previously closed window by pressing Ctrl+Shift+N / Cmd+Shift+N.

Expected Results:
The private window is not reopened at step 6.

Notes:
This test is available in MozTrap as test case #6720.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I wonder why this has to be a mozmill test. Everything should be perfectly doable as a browser chrome test. Are we sure that there is not such a test around already?

Comment 3

[u279076]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> I wonder why this has to be a mozmill test. Everything should be perfectly
> doable as a browser chrome test. Are we sure that there is not such a test
> around already?

Ehsan, do you know if a Private Browsing browser chrome test exists for this use case?

Comment 4

[(no longer active)]

(In reply to comment #3)
> (In reply to Henrik Skupin (:whimboo) from comment #2)
> > I wonder why this has to be a mozmill test. Everything should be perfectly
> > doable as a browser chrome test. Are we sure that there is not such a test
> > around already?
> 
> Ehsan, do you know if a Private Browsing browser chrome test exists for this
> use case?

There isn't.  How would we test this in a browser chrome test though?  It seems to me like testing this requires a restart?

Comment 5

[u279076]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #4)
> How would we test this in a browser chrome test though?  It
> seems to me like testing this requires a restart?

I don't think this requires a restart, based on the original steps. I think the steps imply trying to restore a previously closed private window.

Comment 6

[Ioana (away)]

I can write a browser chrome test for this if you prefer it this way.

Just so I know from now on: how do I decide what kind of test I should create? (excepting the obvious cases where you need something only one framework supports).

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+2]]

If no restart is involved in the test and no other localizations could benefit from it, it's always the best option to create an in-tree test.

So it seems like that for Ehsan it was not clear that no restart is involved. Lets wait for his additional reply but I think this would be a perfect candidate for a browser chrome test.

Comment 8

[(no longer active)]

Yeah I was wrong, a browser chrome test would suffice here!  Sorry for the confusion!

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Thanks Ehsan! So existing browser chrome tests for private browsing can be found here:
http://mxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/test/browser/

Moving the bug to the appropriate component.

Comment 10

[Ioana (away)]

Attachment: testPrivateWindowRestore-v0

Comment 11

[Ioana (away)]

Attachment: testPrivateWindowRestore-v01

I added a clearHistory() at the start, just in case other tests open multiple windows. I ran the test with the whole suite and they all passed.

Comment 12

[(no longer active)]

Comment on attachment 739620 [details] [diff] [review]
testPrivateWindowRestore-v01

Review of attachment 739620 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowRestore.js
@@ +8,5 @@
> +
> +  //Try to restore a private window
> +  var win = OpenBrowserWindow({private: true});
> +  win.close();
> +  EventUtils.synthesizeKey("N", { ctrlKey: true, shiftKey: true });

This doesn't give the sessionstore component enough time to register the window, but check with Tim to make sure.

@@ +11,5 @@
> +  win.close();
> +  EventUtils.synthesizeKey("N", { ctrlKey: true, shiftKey: true });
> +
> +  //Check that the private window was not restored
> +  var windows = Application.windows;

I have no idea what Application.windows is!

Comment 13

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #12)
> > +  //Check that the private window was not restored
> > +  var windows = Application.windows;
> 
> I have no idea what Application.windows is!

It's an old FUEL thing: http://mxr.mozilla.org/mozilla-central/source/browser/fuel/src/fuelApplication.js#775

We shouldn't use it in new code - just use the window mediator (Services.wm.*) directly.

Comment 14

[Ioana (away)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #13)
> We shouldn't use it in new code - just use the window mediator
> (Services.wm.*) directly.

When trying to get the windows with Services.wm.getEnumerator("navigator:browser"), it only gets the initial window. E.g. The initial browser window is opened when I start the test, then I open a new private window and/or a new regular window. => The enumerator only has the initial window.

Do you have any ideas about why this happens or how it can be fixed?

Comment 15

[:Gavin Sharp [email: [e-mail]]]

You need to get a new enumerator each time, existing enumerators are not "live".

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 739620 [details] [diff] [review]
testPrivateWindowRestore-v01

Review of attachment 739620 [details] [diff] [review]:
-----------------------------------------------------------------

This feels more like a sessionstore test to me because sessionstore has to be aware of the private window. It's not something the privatebrowsing code has control over.

::: browser/components/privatebrowsing/test/browser/browser_privatebrowsing_windowRestore.js
@@ +3,5 @@
> + */
> +
> +function test() {
> +  waitForExplicitFinish();
> +  clearHistory();

You could also call ss.forgetClosedWindow() until ss.getClosedWindowCount() == 0.

@@ +8,5 @@
> +
> +  //Try to restore a private window
> +  var win = OpenBrowserWindow({private: true});
> +  win.close();
> +  EventUtils.synthesizeKey("N", { ctrlKey: true, shiftKey: true });

Yeah, we definitely need to wait until after the window has loaded. See http://mxr.mozilla.org/mozilla-central/source/browser/components/sessionstore/test/browser_819510_perwindowpb.js#175

@@ +13,5 @@
> +
> +  //Check that the private window was not restored
> +  var windows = Application.windows;
> +  ok(windows, "Check access to browser windows");
> +  is(windows.length, 1, "The private window should not be restored");

You could check if ss.getClosedWindowCount() is still zero. and we'd be done.

Comment 17

[Ioana (away)]

Attachment: testNoPrivateWindowRestore-v2

Patch modified per previous comments.

Comment 18

[Ioana (away)]

Changing the component since I agree with Tim and have also added the test in the Session Restore suite.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 743050 [details] [diff] [review]
testNoPrivateWindowRestore-v2

Review of attachment 743050 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks Ioana,

the test looks good but there's a couple of things we need to fix. But in general, the test does exactly what it should do!

(For some strange reason the patch doesn't apply on my working copy? I just says the whole patch failed instead of separate chunks...)

::: browser/components/sessionstore/test/Makefile.in
@@ +141,5 @@
>  	browser_819510_perwindowpb.js \
>  	$(filter disabled-for-intermittent-failures--bug-766044, browser_459906_empty.html) \
>  	$(filter disabled-for-intermittent-failures--bug-766044, browser_459906_sample.html) \
>  	$(filter disabled-for-intermittent-failures--bug-765389, browser_461743_sample.html) \
> +        browser_windowRestore_perwindowpb.js \

Nit: Let's add this test after browser_pageshow.js i.e. before the numbers start.

::: browser/components/sessionstore/test/browser_windowRestore_perwindowpb.js
@@ +7,5 @@
> +function test() {
> +  waitForExplicitFinish();
> +
> +  //Clean up the previous open pages
> +  while(ss.getClosedWindowCount() != 0)

while (ss.getClosedWindowCount() > 0)

The closed window count never drops below zero so should be a little more explicit here.

@@ +8,5 @@
> +  waitForExplicitFinish();
> +
> +  //Clean up the previous open pages
> +  while(ss.getClosedWindowCount() != 0)
> +	ss.forgetClosedWindow();

Nit: indentation is a little off.

@@ +14,5 @@
> +  //Load a private window, then close it 
> +  //and verify it doesn't get remembered for restoring
> +  var win = OpenBrowserWindow({private: true});
> +  win.addEventListener("load", function onload() {
> +    win.removeEventListener("load", onload, false);

You can use the helper function whenWindowLoaded() like this:

whenWindowLoaded(win, function onload() {
  ...
  finish();
});

@@ +15,5 @@
> +  //and verify it doesn't get remembered for restoring
> +  var win = OpenBrowserWindow({private: true});
> +  win.addEventListener("load", function onload() {
> +    win.removeEventListener("load", onload, false);
> +    ok(true, "The private window got loaded");

You can use info() like |info("The private window got loaded")| if you just want to print a debug message.

@@ +18,5 @@
> +    win.removeEventListener("load", onload, false);
> +    ok(true, "The private window got loaded");
> +    win.close();
> +    is (ss.getClosedWindowCount(), 0, "The private window should not have been stored");
> +    finish();

It's better to make sure sessionstore has processed the win.close() call. We should listen for the SSWindowClosing event sent by sessionstore when a window closes. executeSoon() makes sure we proceed on the next tick so that all other event handlers can do their work.

win.addEventListener("SSWindowClosing", function onclosing() {
  win.removeEventListener("SSWindowClosing", onclosing, false);
  executeSoon(function () {
    is (ss.getClosedWindowCount(), 0, "The private window should not have been stored");
    finish();
  });
});

win.close()

Comment 20

[Ioana (away)]

Attachment: testNoPrivateWindowRestore-v3

Patch updated per previous review.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 743637 [details] [diff] [review]
testNoPrivateWindowRestore-v3

Review of attachment 743637 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks! r=me with the issues below fixed.

::: browser/components/sessionstore/test/browser_windowRestore_perwindowpb.js
@@ +6,5 @@
> +
> +function test() {
> +  waitForExplicitFinish();
> +
> +  //Clean up the previous open pages

We're purging the list of closed windows here.

@@ +8,5 @@
> +  waitForExplicitFinish();
> +
> +  //Clean up the previous open pages
> +  while(ss.getClosedWindowCount() > 0)
> +    ss.forgetClosedWindow();

That should be: ss.forgetClosedWindow(0);

@@ +24,5 @@
> +            "The private window should not have been stored");
> +      });
> +    }, false);
> +    win.close();
> +    finish();

This works only because SSWindowClosing is sent synchronously but we shouldn't rely on that. The finish() call should be in executeSoon() of the SSWindowClosing listener.

Comment 22

[Ioana (away)]

Attachment: testNoPrivateWindowRestore-v4

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 744524 [details] [diff] [review]
testNoPrivateWindowRestore-v4

The patch needs a summary that contains the bug number (like "Bug 862371 - Test that closed ...") or else we can't check it in. Please use the 'checkin-needed' keyword to signal that your patch is ready to be checked in. Thanks!

Comment 24

[Ioana (away)]

Attachment: testNoPrivateWindowRestore-v4

Same patch, added the bug number in the summary.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/64fb2941a3f5

Comment 26

[Joe Walker [:jwalker]]

https://hg.mozilla.org/mozilla-central/rev/64fb2941a3f5