Last Comment Bug 862371 - Create a test that checks that private windows are not restored from normal windows
: Create a test that checks that private windows are not restored from normal w...
Status: RESOLVED FIXED
:
Product: Firefox
Classification: Client Software
Component: Session Restore (show other bugs)
: unspecified
: All All
-- normal (vote)
: Firefox 23
Assigned To: Ioana (away)
:
: Mike de Boer [:mikedeboer]
Mentors:
: 862370 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-04-16 08:05 PDT by Ioana (away)
Modified: 2013-05-11 10:22 PDT (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testPrivateWindowRestore-v0 (2.05 KB, patch)
2013-04-19 08:01 PDT, Ioana (away)
no flags Details | Diff | Splinter Review
testPrivateWindowRestore-v01 (2.07 KB, patch)
2013-04-19 08:25 PDT, Ioana (away)
no flags Details | Diff | Splinter Review
testNoPrivateWindowRestore-v2 (2.21 KB, patch)
2013-04-29 07:06 PDT, Ioana (away)
ttaubert: feedback+
Details | Diff | Splinter Review
testNoPrivateWindowRestore-v3 (2.24 KB, patch)
2013-04-30 08:17 PDT, Ioana (away)
ttaubert: review+
Details | Diff | Splinter Review
testNoPrivateWindowRestore-v4 (2.29 KB, patch)
2013-05-02 01:55 PDT, Ioana (away)
no flags Details | Diff | Splinter Review
testNoPrivateWindowRestore-v4 (2.31 KB, patch)
2013-05-07 04:05 PDT, Ioana (away)
no flags Details | Diff | Splinter Review

Description User image Ioana (away) 2013-04-16 08:05:30 PDT
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 1 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2013-04-16 09:59:14 PDT
*** Bug 862370 has been marked as a duplicate of this bug. ***
Comment 2 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2013-04-16 10:01:00 PDT
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 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-04-16 10:04:08 PDT
(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 User image :Ehsan Akhgari 2013-04-16 13:04:14 PDT
(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 User image Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2013-04-16 14:27:47 PDT
(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 User image Ioana (away) 2013-04-17 00:44:46 PDT
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 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2013-04-17 01:38:57 PDT
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 User image :Ehsan Akhgari 2013-04-17 06:51:30 PDT
Yeah I was wrong, a browser chrome test would suffice here!  Sorry for the confusion!
Comment 9 User image Henrik Skupin (:whimboo) [away 02/18 - 02/27] 2013-04-17 06:59:42 PDT
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 User image Ioana (away) 2013-04-19 08:01:27 PDT
Created attachment 739614 [details] [diff] [review]
testPrivateWindowRestore-v0
Comment 11 User image Ioana (away) 2013-04-19 08:25:00 PDT
Created attachment 739620 [details] [diff] [review]
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 User image :Ehsan Akhgari 2013-04-19 08:54:44 PDT
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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-19 13:35:15 PDT
(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 User image Ioana (away) 2013-04-24 09:31:51 PDT
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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 User image :Gavin Sharp [email: gavin@gavinsharp.com] 2013-04-25 01:08:40 PDT
You need to get a new enumerator each time, existing enumerators are not "live".
Comment 16 User image Tim Taubert [:ttaubert] 2013-04-26 06:57:09 PDT
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 User image Ioana (away) 2013-04-29 07:06:20 PDT
Created attachment 743050 [details] [diff] [review]
testNoPrivateWindowRestore-v2

Patch modified per previous comments.
Comment 18 User image Ioana (away) 2013-04-29 07:07:20 PDT
Changing the component since I agree with Tim and have also added the test in the Session Restore suite.
Comment 19 User image Tim Taubert [:ttaubert] 2013-04-29 09:26:46 PDT
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 User image Ioana (away) 2013-04-30 08:17:35 PDT
Created attachment 743637 [details] [diff] [review]
testNoPrivateWindowRestore-v3

Patch updated per previous review.
Comment 21 User image Tim Taubert [:ttaubert] 2013-04-30 09:33:44 PDT
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 User image Ioana (away) 2013-05-02 01:55:06 PDT
Created attachment 744524 [details] [diff] [review]
testNoPrivateWindowRestore-v4
Comment 23 User image Tim Taubert [:ttaubert] 2013-05-07 04:00:37 PDT
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 User image Ioana (away) 2013-05-07 04:05:55 PDT
Created attachment 746335 [details] [diff] [review]
testNoPrivateWindowRestore-v4

Same patch, added the bug number in the summary.
Comment 25 User image Tim Taubert [:ttaubert] 2013-05-07 04:10:08 PDT
https://hg.mozilla.org/integration/fx-team/rev/64fb2941a3f5
Comment 26 User image Joe Walker [:jwalker] (needinfo me or ping on irc) 2013-05-11 10:22:04 PDT
https://hg.mozilla.org/mozilla-central/rev/64fb2941a3f5

Note You need to log in before you can comment on or make changes to this bug.