Closed Bug 1340450 Opened 7 years ago Closed 7 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox57 --- fix-optional
firefox58 --- fixed

People

(Reporter: baku, Assigned: baku)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [userContextId][domsecurity-active])

Attachments

(1 file, 2 obsolete files)

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
Assignee: nobody → amarchesini
Attached patch storage.patch (obsolete) — Splinter Review
Attachment #8838608 - Flags: review?(mdeboer)
Attachment #8838608 - Flags: review?(gijskruitbosch+bugs)
OS: Unspecified → All
Hardware: Unspecified → All
Whiteboard: [userContextId]
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.
Attachment #8838608 - Flags: review?(gijskruitbosch+bugs)
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.
Attachment #8838608 - Flags: review?(mdeboer) → review+
Attached patch storage2.patch (obsolete) — Splinter Review
Attachment #8838631 - Flags: review?(mdeboer)
Andrea, can you attach the final patch that I can apply to run the test in patch 2?
Flags: needinfo?(amarchesini)
Attached patch patchSplinter Review
Patch + test merged.
Attachment #8838608 - Attachment is obsolete: true
Attachment #8838631 - Attachment is obsolete: true
Attachment #8838631 - Flags: review?(mdeboer)
Flags: needinfo?(amarchesini)
Attachment #8838867 - Flags: review?(mdeboer)
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'
Attachment #8838867 - Flags: review?(mdeboer) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b22d70e15f5
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer
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
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f7bb57e5440
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer
Flags: needinfo?(amarchesini)
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
Flags: needinfo?(amarchesini)
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.
Flags: needinfo?(amarchesini) → needinfo?(mdeboer)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ec5aae7453f
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer
Ok, I found the issue: TabStateFlusher.flushWindow() must be called.
Flags: needinfo?(mdeboer)
backed out for test failures in https://treeherder.mozilla.org/logviewer.html#?job_id=79383511&repo=mozilla-inbound
Flags: needinfo?(amarchesini)
Backout by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b156516eff
Backed out changeset 0ec5aae7453f for test failures in own test
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d59ad4198154
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer
Flags: needinfo?(amarchesini)
Priority: -- → P1
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
Flags: needinfo?(amarchesini)
Component: Security → DOM: Security
Whiteboard: [userContextId] → [userContextId][domsecurity-active]
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/49341ba517f9
When containers are disabled, it should not be possible to reopen container tabs, r=mdeboer
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f6b1224f1f42
Adding an extra space in order to make eslint happy, r=me
Depends on: 1549204
You need to log in before you can comment on or make changes to this bug.