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)
:
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 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 Henrik Skupin (:whimboo) 2013-04-16 09:59:14 PDT
*** Bug 862370 has been marked as a duplicate of this bug. ***
Comment 2 Henrik Skupin (:whimboo) 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 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 :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 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 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 Henrik Skupin (:whimboo) 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 :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 Henrik Skupin (:whimboo) 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 Ioana (away) 2013-04-19 08:01:27 PDT
Created attachment 739614 [details] [diff] [review]
testPrivateWindowRestore-v0
Comment 11 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 :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 :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 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 :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 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 Ioana (away) 2013-04-29 07:06:20 PDT
Created attachment 743050 [details] [diff] [review]
testNoPrivateWindowRestore-v2

Patch modified per previous comments.
Comment 18 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 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 Ioana (away) 2013-04-30 08:17:35 PDT
Created attachment 743637 [details] [diff] [review]
testNoPrivateWindowRestore-v3

Patch updated per previous review.
Comment 21 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 Ioana (away) 2013-05-02 01:55:06 PDT
Created attachment 744524 [details] [diff] [review]
testNoPrivateWindowRestore-v4
Comment 23 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 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 Tim Taubert [:ttaubert] 2013-05-07 04:10:08 PDT
https://hg.mozilla.org/integration/fx-team/rev/64fb2941a3f5
Comment 26 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.