1340450 - When containers are disabled, it is still possible to reopen container tabs.

Status: RESOLVED FIXED

(Core :: DOM: Security, defect, P1)

Description

[Andrea Marchesini [:baku]]

STR:
 * Enable containers in about:config
 * Create a tab, go to a page other than new tab
 * Close tab
 * Disable containers
 * Use history menu to reopen tab

Comment 2

[Andrea Marchesini [:baku]]

Attachment: storage.patch

Comment 3

[:Gijs (he/him)]

Comment on attachment 8838608 [details] [diff] [review]
storage.patch

Mike's review should be enough here. I can help with questions if/when those arise.

Comment 4

[Mike de Boer [:mikedeboer]]

Comment on attachment 8838608 [details] [diff] [review]
storage.patch

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

Looks good! Impressive turn-around time ;)

r=me with comments addressed.

::: browser/components/sessionstore/SessionStore.jsm
@@ +751,5 @@
> +      case "clear-origin-attributes-data":
> +        let userContextId = 0;
> +        try {
> +          userContextId = JSON.parse(aData).userContextId;
> +        } catch(e) {

nit: Can you keep the braces on the same line?

@@ +753,5 @@
> +        try {
> +          userContextId = JSON.parse(aData).userContextId;
> +        } catch(e) {
> +        }
> +        if (userContextId) {

nit: no need for braces for single-statement control structure blocks in the sessionstore component.

@@ +2590,5 @@
>      // Neither a tab nor a window was found, return undefined and let the caller decide what to do about it.
>      return undefined;
>    },
>  
> +  _forgetTabsWithUserContextId(userContextId) {

Can you add a docblock comment here that explains what this function is doing?

@@ +2599,5 @@
> +      if (windowState) {
> +        // In order to remove the tabs in the correct order, we store the ids
> +        // into an array, then we revert the array and remove closed data from
> +        // the last one going backward.
> +        let ids = [];

nit: Technically, these are not 'IDs', but 'indices'. Can you change that?

@@ +2606,5 @@
> +            ids.push(index);
> +          }
> +        });
> +
> +        ids.reverse().forEach(index => {

```js
for (let index of indices.reverse())
  this.removeClosedTabData(windowState._closedTabs, index);
```

@@ +2613,5 @@
> +      }
> +    }
> +
> +    // Notify of changes to closed objects.
> +    this._notifyOfClosedObjectsChange();

```js
if (indices.length)
  // Notify.
```

::: toolkit/components/contextualidentity/ContextualIdentityService.jsm
@@ +298,5 @@
>      }, userContextId);
>    },
>  
> +  disableContainers() {
> +    this._identities.forEach(identity => {

Please use for...of here too.

Comment 5

[Andrea Marchesini [:baku]]

Attachment: storage2.patch

Comment 6

[Mike de Boer [:mikedeboer]]

Andrea, can you attach the final patch that I can apply to run the test in patch 2?

Comment 7

[Andrea Marchesini [:baku]]

Attachment: patch

Patch + test merged.

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8838867 [details] [diff] [review]
patch

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

LGTM! Thanks!

::: browser/components/sessionstore/test/browser_disable_containers.js
@@ +3,5 @@
> +/**
> + * This test is to see if tabs can be reopened when containers are disabled.
> + */
> +
> +Cu.import("resource:///modules/sessionstore/SessionStore.jsm");

I think head.js has you covered here; you don't need to import anything.

@@ +26,5 @@
> +  await promiseBrowserLoaded(win.gBrowser.selectedBrowser, true, url);
> +  return win;
> +}
> +
> +add_task(function* test_undoCloseById() {

Oh, add_task doesn't support async functions yet?

@@ +41,5 @@
> +  yield openAndCloseTab(win, 0, "about:mozilla");
> +  is(SessionStore.lastClosedObjectType, "tab", "The last closed object is a tab");
> +
> +  // Open and close a container tab.
> +  yield openAndCloseTab(win, 3, "about:about");

Can you make this a constant to be used throughout this test?

@@ +48,5 @@
> +  // Restore the last closed tab.
> +  let tab = SessionStore.undoCloseTab(win, 0);
> +  yield promiseBrowserLoaded(tab.linkedBrowser);
> +  is(tab.linkedBrowser.currentURI.spec, "about:about", "The expected tab was re-opened");
> +  is(tab.getAttribute("usercontextid"), 3, "No userContextId for this tab");

ITYM 'Expected the tab userContextId to match'

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b22d70e15f5
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequent failure of own test browser_disable_containers.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/76db6e75cd47d81b7cfe075dcba16d136bfc9d5d

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=6b22d70e15f5d0a3955e2535573dd8ae18555b87&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable&filter-searchStr=browser-chrome
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=78838573&repo=mozilla-inbound

08:07:00     INFO - TEST-START | browser/components/sessionstore/test/browser_disable_containers.js
08:07:02     INFO - TEST-INFO | started process screenshot
08:07:02     INFO - TEST-INFO | screenshot: exit 0
08:07:02     INFO - Buffered messages logged at 08:07:00
08:07:02     INFO - Entering test bound test_undoCloseById
08:07:02     INFO - Buffered messages logged at 08:07:01
08:07:02     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | The last closed object is a tab - 
08:07:02     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | The last closed object is a tab - 
08:07:02     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | The expected tab was re-opened - 
08:07:02     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | No userContextId for this tab - 
08:07:02     INFO - Buffered messages logged at 08:07:02
08:07:02     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | No userContextId for this tab - 
08:07:02     INFO - Buffered messages finished
08:07:02     INFO - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_disable_containers.js | The expected tab was re-opened - Got about:about, expected about:mozilla
08:07:02     INFO - Stack trace:
08:07:02     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
08:07:02     INFO -     chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_disable_containers.js:test_undoCloseById:72
08:07:02     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
08:07:02     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
08:07:02     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7bb57e5440
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for frequently failing own test browser_disable_containers.js:

https://hg.mozilla.org/integration/mozilla-inbound/rev/775d6573482d7d0af7431ae3f54d1b9662fefb26

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=3f7bb57e5440dae6d6b21c2a9db8275af9ba54b2&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=79079283&repo=mozilla-inbound

07:36:33     INFO - TEST-START | browser/components/sessionstore/test/browser_disable_containers.js
07:36:35     INFO - WebGL(0x12c607000)::ForceLoseContext
07:36:35     INFO - WebGL(0x12c60d000)::ForceLoseContext
07:36:35     INFO - JavaScript error: chrome://global/content/aboutSupport.js, line 602: TypeError: data.syscallLog is undefined
07:36:35     INFO - TEST-INFO | started process screencapture
07:36:36     INFO - TEST-INFO | screencapture: exit 0
07:36:36     INFO - Buffered messages logged at 07:36:33
07:36:36     INFO - Entering test bound test_undoCloseById
07:36:36     INFO - Console message: OpenGL compositor Initialized Succesfully.
07:36:36     INFO - Version: 2.1 INTEL-10.6.33
07:36:36     INFO - Vendor: Intel Inc.
07:36:36     INFO - Renderer: Intel Iris OpenGL Engine
07:36:36     INFO - FBO Texture Target: TEXTURE_2D
07:36:36     INFO - Buffered messages logged at 07:36:34
07:36:36     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | The last closed object is a tab - 
07:36:36     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | The last closed object is a tab - 
07:36:36     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | The expected tab was re-opened - 
07:36:36     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | Expected the tab userContextId to match - 
07:36:36     INFO - Buffered messages logged at 07:36:35
07:36:36     INFO - Console message: [JavaScript Error: "TypeError: data.syscallLog is undefined" {file: "chrome://global/content/aboutSupport.js" line: 602}]
07:36:36     INFO - TEST-PASS | browser/components/sessionstore/test/browser_disable_containers.js | No userContextId for this tab - 
07:36:36     INFO - Buffered messages finished
07:36:36     INFO - TEST-UNEXPECTED-FAIL | browser/components/sessionstore/test/browser_disable_containers.js | The expected tab was re-opened - Got about:about, expected about:mozilla
07:36:36     INFO - Stack trace:
07:36:36     INFO -     chrome://mochikit/content/browser-test.js:test_is:911
07:36:36     INFO -     chrome://mochitests/content/browser/browser/components/sessionstore/test/browser_disable_containers.js:test_undoCloseById:70
07:36:36     INFO -     Tester_execTest@chrome://mochikit/content/browser-test.js:735:9
07:36:36     INFO -     Tester.prototype.nextTest</<@chrome://mochikit/content/browser-test.js:655:7
07:36:36     INFO -     SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:791:59

Comment 13

[Andrea Marchesini [:baku]]

Mike, it seems that the test fails intermittently. Is there any particular thing I have to wait for in order to be 100% that the tabs are restored/closed? I'll debug this issue locally a bit more, but, of course, I'm not able to reproduce it yet.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec5aae7453f
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer

Comment 15

[Andrea Marchesini [:baku]]

Ok, I found the issue: TabStateFlusher.flushWindow() must be called.

Comment 16

[Carsten Book [:Tomcat]]

backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=79383511&repo=mozilla-inbound

Comment 17

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b156516eff
Backed out changeset 0ec5aae7453f for test failures in own test

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59ad4198154
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer

Comment 19

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/68ada0ef9842 for 30-40% failure rate like https://treeherder.mozilla.org/logviewer.html#?job_id=79668281&repo=mozilla-inbound on Mac and Windows.

Comment 21

[Kamil Jozwiak [:kjozwiak]]

Looks like this is still an issue on all current versions of fx [1]. Baku, as you've already worked on this and attached a patch, do you want to give this another crack? Or should we just wait for the results from the shield study?

[1] Builds being used:

* fx55.0a1, buildid: 20170529030204, changeset: cce4d83d2b99
* fx54.0b12, buildid: 20170529025116, changeset: 715d88d0bd82
* fx53.0.3, buildid: 20170518000419, changeset: 1fe643b3421a

Comment 22

[Firefox Bug Husbandry Bot]

Bulk change per https://bugzilla.mozilla.org/show_bug.cgi?id=1401020

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49341ba517f9
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b1224f1f42
Adding an extra space in order to make eslint happy, r=me

Comment 25

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/49341ba517f9
https://hg.mozilla.org/mozilla-central/rev/f6b1224f1f42