885054 - Sidebars should not migrate across private / non-private windows

Status: RESOLVED FIXED

(Firefox :: Private Browsing, defect)

Description

[Erik Vold [:erikvold] (please needinfo? me)]

Web Panel sidebars are meant to be used by extensions to display remote content in the sidebar, and when I create one then open a private window, it is displayed there, and when I open a non-private window from a private one, the web panel is opened in the new window too.

I don't think that the web panel, and even the sidebar should be reopened when the parent window has an opposite privacy state.

Comment 1

[Erik Vold [:erikvold] (please needinfo? me)]

Note: http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#833

Comment 2

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Sidebars should not migrate across private / non-private window

Comment 3

[Erik Vold [:erikvold] (please needinfo? me)]

Not sure how best to test this, I do have a jetpack test for it already though.  This patch could probably use more testing tho, and I'm not sure if it breaks any tests..

Comment 4

[(no longer active)]

Comment on attachment 770527 [details] [diff] [review]
Sidebars should not migrate across private / non-private window

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

Makes sense, but I'd like Gavin to also take a look at this.

For testing this patch, you can write a browser-chrome test which opens a sidebar, and then opens a private window, and makes sure that the sidebar is not open in that window.  The test can also open a regular window and ensure that the sidebar is present in that window.

Comment 5

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

Comment on attachment 770527 [details] [diff] [review]
Sidebars should not migrate across private / non-private window

Can you just add this new condition to the existing one, to avoid re-indenting everything? You could also lose the comment since it's pretty self-documenting.

I doubt we have existing test coverage for this, but it seems like something that would be relatively easy to add as a browser chrome test. See e.g. browser/base/content/test/browser_private_browsing_window.js for a test that tests similar situations.

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: patch and tests

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: patch and tests

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: patch and tests

Comment 9

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: screen shot of messy Make.in file

Just in case you are wondering why I changed the Make file, this is why..

Comment 10

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

Comment on attachment 772847 [details] [diff] [review]
patch and tests

>diff --git a/browser/base/content/browser.js b/browser/base/content/browser.js

>-    if (window.opener && !window.opener.closed) {
>+    if (window.opener && !window.opener.closed
>+        && PrivateBrowsingUtils.isWindowPrivate(window) == PrivateBrowsingUtils.isWindowPrivate(window.opener)) {

nit: we keep operators at the end of the line when wrapping conditions

>-    else {
>+    else if (!window.opener || window.opener.closed) {

I don't think you need to add this condition - if we're not restoring the opener's sidebox because of PB differences, we might as well open any default persisted ones, I figure. In any case this is an edge case so we shouldn't worry too much about it.

>diff --git a/browser/components/privatebrowsing/test/browser/Makefile.in b/browser/components/privatebrowsing/test/browser/Makefile.in

The reformatting should probably happen in a separate changeset.

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js

>+  let window = Cc['@mozilla.org/appshell/window-mediator;1'].
>+                 getService(Ci.nsIWindowMediator).
>+                 getMostRecentWindow('navigator:browser');

Services.wm.getMostRecentWindow

>+  ok(!!window, 'there is a window open at the start of these tests');

Though actually, this is kind of a silly check - you can just use "window" directly, and you don't need to assert its existence (browser chrome tests run in a chrome window scope).

>+  function openSidebar(win) {
>+    let { promise, resolve } = defer();
>+    let { document: doc } = win;

I really hate this style - destructuring assignment for a single property is way harder to read than a simple property access (let doc = win.document) :)

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: patch, tests, review fixes

Comment 12

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

Comment on attachment 773592 [details] [diff] [review]
patch, tests, review fixes

>diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js

>+function test() {

>+  let window = Services.wm.getMostRecentWindow('navigator:browser');

As I tried to explain in my review comment, this line isn't necessary - this test code is running in a chrome browser window scope, so |window| already refers to the browser chrome window (and this variable is just shadowing the existing reference that has the same value). So you can just remove it.
 
>+  function closeCachedWindows () {
>+    if (windowCache.length == 0) {
>+      return;
>+    }
>+    return closeWindow(windowCache.pop()).then(closeCachedWindows);

Didn't notice this before but this is a rather inefficient way of closing windows. I suppose it doesn't matter too much for a test file, but I would skip the "wait for unload" bits and just put a windowCache.forEach(w => w.close()) at the end of the test.

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #12)
> Comment on attachment 773592 [details] [diff] [review]
> patch, tests, review fixes
> 
> >diff --git a/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js b/browser/components/privatebrowsing/test/browser/browser_privatebrowsing_sidebar.js
> 
> >+function test() {
> 
> >+  let window = Services.wm.getMostRecentWindow('navigator:browser');
> 
> As I tried to explain in my review comment, this line isn't necessary - this
> test code is running in a chrome browser window scope, so |window| already
> refers to the browser chrome window (and this variable is just shadowing the
> existing reference that has the same value). So you can just remove it.

hmm weird, I am sure I tried that..

> >+  function closeCachedWindows () {
> >+    if (windowCache.length == 0) {
> >+      return;
> >+    }
> >+    return closeWindow(windowCache.pop()).then(closeCachedWindows);
> 
> Didn't notice this before but this is a rather inefficient way of closing
> windows. I suppose it doesn't matter too much for a test file, but I would
> skip the "wait for unload" bits and just put a windowCache.forEach(w =>
> w.close()) at the end of the test.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: fix, tests, review changes v2

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: fix, tests, review comments v2

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

I don't have commit privileges to push the change, so someone will need to do that for me, assuming they won't take authorship with mercurial.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3d9e6b1005d2

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3d9e6b1005d2

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

Thanks for the help everyone!