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

RESOLVED FIXED in Firefox 23

Status

()

Firefox
Session Restore
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: Ioana (away), Assigned: Ioana (away))

Tracking

unspecified
Firefox 23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → ioana.budnar
Status: NEW → ASSIGNED
Duplicate of this bug: 862370
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?
(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?
Flags: needinfo?(ehsan)
(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?
Flags: needinfo?(ehsan)
(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.
(Assignee)

Comment 6

4 years ago
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).
Flags: needinfo?(hskupin)
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.
Flags: needinfo?(hskupin)
Yeah I was wrong, a browser chrome test would suffice here!  Sorry for the confusion!
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.
Component: Mozmill Tests → Private Browsing
Product: Mozilla QA → Firefox
QA Contact: hskupin
Summary: Create a Mozmill 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 windows
(Assignee)

Comment 10

4 years ago
Created attachment 739614 [details] [diff] [review]
testPrivateWindowRestore-v0
Attachment #739614 - Flags: review?(ehsan)
Attachment #739614 - Flags: feedback?(ehsan)
(Assignee)

Comment 11

4 years ago
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.
Attachment #739614 - Attachment is obsolete: true
Attachment #739614 - Flags: review?(ehsan)
Attachment #739614 - Flags: feedback?(ehsan)
Attachment #739620 - Flags: review?(ehsan)
Attachment #739620 - Flags: feedback?(ehsan)
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!
Attachment #739620 - Flags: review?(ttaubert)
Attachment #739620 - Flags: review?(ehsan)
Attachment #739620 - Flags: feedback?(ehsan)
(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.
(Assignee)

Comment 14

4 years ago
(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?
You need to get a new enumerator each time, existing enumerators are not "live".
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.
Attachment #739620 - Flags: review?(ttaubert)
(Assignee)

Comment 17

4 years ago
Created attachment 743050 [details] [diff] [review]
testNoPrivateWindowRestore-v2

Patch modified per previous comments.
Attachment #739620 - Attachment is obsolete: true
Attachment #743050 - Flags: review?(ttaubert)
(Assignee)

Comment 18

4 years ago
Changing the component since I agree with Tim and have also added the test in the Session Restore suite.
Component: Private Browsing → Session Restore
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()
Attachment #743050 - Flags: review?(ttaubert) → feedback+
(Assignee)

Comment 20

4 years ago
Created attachment 743637 [details] [diff] [review]
testNoPrivateWindowRestore-v3

Patch updated per previous review.
Attachment #743050 - Attachment is obsolete: true
Attachment #743637 - Flags: review?(ttaubert)
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.
Attachment #743637 - Flags: review?(ttaubert) → review+
(Assignee)

Comment 22

4 years ago
Created attachment 744524 [details] [diff] [review]
testNoPrivateWindowRestore-v4
Attachment #743637 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Attachment #744524 - Flags: checkin?
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!
Attachment #744524 - Flags: checkin?
(Assignee)

Comment 24

4 years ago
Created attachment 746335 [details] [diff] [review]
testNoPrivateWindowRestore-v4

Same patch, added the bug number in the summary.
Attachment #744524 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/64fb2941a3f5
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/64fb2941a3f5
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
You need to log in before you can comment on or make changes to this bug.