Closed Bug 885054 Opened 7 years ago Closed 7 years ago

Sidebars should not migrate across private / non-private windows

Categories

(Firefox :: Private Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 25

People

(Reporter: evold, Assigned: evold)

References

(Depends on 1 open bug)

Details

Attachments

(1 file, 7 obsolete files)

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.
Assignee: nobody → evold
Summary: Web Panel Sidebars should not migrate across private / non-private windows → Sidebars should not migrate across private / non-private windows
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..
Attachment #770527 - Attachment is patch: true
Attachment #770527 - Attachment mime type: text/x-patch → text/plain
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.
Attachment #770527 - Flags: review?(ehsan) → review?(gavin.sharp)
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.
Attachment #770527 - Flags: review?(gavin.sharp) → review+
Attached patch patch and tests (obsolete) — Splinter Review
Attachment #770527 - Attachment is obsolete: true
Attachment #772844 - Attachment is patch: true
Attachment #772844 - Attachment mime type: text/x-patch → text/plain
Attached patch patch and tests (obsolete) — Splinter Review
Attached patch patch and tests (obsolete) — Splinter Review
Attachment #772844 - Attachment is obsolete: true
Attachment #772846 - Attachment is obsolete: true
Attachment #772847 - Flags: review?(gavin.sharp)
Attached image screen shot of messy Make.in file (obsolete) —
Just in case you are wondering why I changed the Make file, this is why..
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) :)
Attachment #772847 - Flags: review?(gavin.sharp) → review+
Attached patch patch, tests, review fixes (obsolete) — Splinter Review
Attachment #772847 - Attachment is obsolete: true
Attachment #772852 - Attachment is obsolete: true
Attachment #773592 - Flags: review?(gavin.sharp)
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.
Attachment #773592 - Flags: review?(gavin.sharp) → review+
(In reply to :Gavin Sharp (use gavin@gavinsharp.com 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.
Attached patch fix, tests, review changes v2 (obsolete) — Splinter Review
Attachment #773592 - Attachment is obsolete: true
Attachment #773654 - Flags: review?(gavin.sharp)
Attachment #773654 - Attachment is obsolete: true
Attachment #773654 - Flags: review?(gavin.sharp)
Attachment #773660 - Flags: review?(gavin.sharp)
Attachment #773660 - Flags: review?(gavin.sharp) → review+
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.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3d9e6b1005d2
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Thanks for the help everyone!
Depends on: 1268550
You need to log in before you can comment on or make changes to this bug.