Closed Bug 1362034 Opened 7 years ago Closed 6 years ago

Have addTab() provide the correct triggeringPrincipal

Categories

(Core :: DOM: Security, enhancement, P2)

Product:
Core
Component:
DOM: Security
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla63
Tracking Flags:
Tracking Status
firefox63 --- fixed

People

(Reporter: ckerschb, Assigned: jkt)

References

(Depends on 1 open bug, Blocks 2 open bugs)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 files, 5 obsolete files)

Bug 1362034 - Have addTab() provide the correct triggering principal.
46 bytes, text/x-phabricator-request
ckerschb
: review+
Gijs
: review+
Details | Review
Bug 1362034 - Tests for addTab() to provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!
46 bytes, text/x-phabricator-request
ckerschb
: review+
Gijs
: review+
Details | Review 
As discussed with Gijs within Bug 1333030 we should make sure that addTab always has a triggeringPrincipal available to pass on. 

[0] https://bugzilla.mozilla.org/show_bug.cgi?id=1333030#c6
[1] https://dxr.mozilla.org/mozilla-central/source/browser/base/content/tabbrowser.xml#2220
Assignee: nobody → ckerschb
Blocks: require-triggering-principal
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
Depends on: 1363975
Depends on: 1363977
Depends on: 1364392
Just adding that note so we can remember later; within the two xpi files:
* toolkit/components/addoncompat/tests/browser/compat-addon.xpi
* toolkit/components/addoncompat/tests/browser/addon.xpi

I had to update bootstrap.js in the following way:
> let tab = gBrowser.addTab("about:blank", {
>   triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal(),
> });
Assignee: ckerschb → jkt
Depends on: 1461707
Hey :kmag,

Could you please review: https://phabricator.services.mozilla.com/D1935. I couldn't find you on phabricator.

Thanks
Flags: needinfo?(kmaglione+bmo)
Attachment #8975474 - Attachment is obsolete: true
Attachment #8989695 - Attachment is obsolete: true
With the changes I see a new error that needs fixing: browser/components/search/test/browser_contextSearchTabPosition.js
Both of the diffs seem to contain all the changes (both test and non-test) now...
Attachment #8990444 - Attachment is obsolete: true
Attachment #8990439 - Attachment is obsolete: true
Attachment #8990941 - Attachment is obsolete: true
Blocks: 1474619
Comment on attachment 8990980 [details]
Bug 1362034 - Tests for addTab() to provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D2047
Attachment #8990980 - Flags: review+
Comment on attachment 8990978 [details]
Bug 1362034 - Have addTab() provide the correct triggering principal.

Christoph Kerschbaumer [:ckerschb] has approved the revision.

https://phabricator.services.mozilla.com/D2046
Attachment #8990978 - Flags: review+
Comment on attachment 8990978 [details]
Bug 1362034 - Have addTab() provide the correct triggering principal.

:Gijs (he/him) has approved the revision.

https://phabricator.services.mozilla.com/D2046
Attachment #8990978 - Flags: review+
Comment on attachment 8990980 [details]
Bug 1362034 - Tests for addTab() to provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!

:Gijs (he/him) has approved the revision.

https://phabricator.services.mozilla.com/D2047
Attachment #8990980 - Flags: review+
Note that your try push 

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b61d1b4ef77be52f82787d1bc3ec4526e7f4ec7c&filter-tier=1&filter-tier=2&filter-tier=3

resulted in a number of Assertion failure: false (Should not get system principal here), at /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp:961 failures on physical android devices.

You'll need to enable tier 3 to see them.

Thanks.
I'm currently working on that code, it's additional checks on top of what we are fixing here to hopefully tighten up our code. It's very broken at the moment.

However the code here I think has a clean or almost clean try run.
Hey :kmag, I just want to verify one thing which is:

In making this change web extensions will be able to see other extension paths: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6812f8ebc462dcb75c82cb22be10cbd0873ee67d&selectedJob=190859963 (This test failure highlights the change best).

Essentially the extension now makes the request to a file using it's own principal rather than system. This can be observed as being from the extensions origin in a different extension.

Is this ok? The current reason this is working is due to the elevated principal, after this change extensions still won't be able to monitor system principal changes however there won't be a way to test this either (I should probably file a follow up to explore how to test that we never get any requests we expect here).

Thanks
This is now actually status quo since https://bugzilla.mozilla.org/show_bug.cgi?id=1378647 has landed anyway so I don't think there are really any extension changes to worry about here now :).

I just worked through a nasty rebase so will retest but I think we are ok to land now.
Flags: needinfo?(kmaglione+bmo)
I have been fighting a test failure caused by the addTab patch I am working on for ages. Do you have any idea why hovering a tab in a mochitest would fix a "TabSwitchDone" event not firing causing a timeout in a test?

The test is browser/base/content/test/tabs/browser_positional_attributes.js seen in this try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3f144db5b0626670e8bca45e0842370b20116313&selectedJob=191146313

It looks like an AsyncTabSwitcher is Constructed as soon as I hover the tab (even though the selectedTab has already been set).

I can fix this test by placing the following at the end of setup():
EventUtils.synthesizeMouse(tabs[1], 1, 1, {type: "mouseorigin"});

Because of the amount of rebasing this patch requires, I'm probably going to try and land with this and file a follow up unless there is something obvious causing this in the test.

It looks like this test already has some intermittent failures but this patch somehow makes it constantly fail in debug builds.
Flags: needinfo?(dao+bmo)
Keywords: checkin-needed
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff6b05c96094
Have addTab() provide the correct triggering principal. r=ckerschb,Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c8925b75aa2
Tests for addTab() to provide the correct triggering principal. r=ckerschb,Gijs
Keywords: checkin-needed
(In reply to Jonathan Kingston [:jkt] from comment #34)
> I have been fighting a test failure caused by the addTab patch I am working
> on for ages. Do you have any idea why hovering a tab in a mochitest would
> fix a "TabSwitchDone" event not firing causing a timeout in a test?

Are you sure that's what's happening? I would expect more things to break if the TabSwitchDone event was this unreliable. Maybe it's the test gets confused if the mouse is at a certain position? Though the only async parts of the test seem to be await switchTab(...) calls, so this does seem suspicious. What does BrowserTestUtils.switchTab do when called with a tab that is already selected?

> The test is browser/base/content/test/tabs/browser_positional_attributes.js
> seen in this try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=3f144db5b0626670e8bca45e0842370b20116313&selectedJob=1
> 91146313
> 
> It looks like an AsyncTabSwitcher is Constructed as soon as I hover the tab
> (even though the selectedTab has already been set).

This would be because of the warmupTab call:

https://searchfox.org/mozilla-central/rev/51268dcbdff0f6f4a5cff7986df0f616efc5bcfd/browser/base/content/tabbrowser.xml#1863
Flags: needinfo?(dao+bmo)
Btw, why are you calling addTrustedTab from SessionStore and addWebTab from aboutSessionRestore.js? The use cases are roughly the same as far as I can tell. I'm asking not because this may mean we have more work to do make these APIs user friendly.
::dao

The one in aboutSessionRestore.js swaps out the docshell and behaves quite differently (I use addWebTab when system isn't needed not because it's from a place than could use system). We use trustedTab for session restore to restore tabs like about: urls.

> What does BrowserTestUtils.switchTab do when called with a tab that is already selected

The whole function just sets tabbrowser.selectedTab = tab and awaits on a "TabSwitchDone" so probably doesn't do anything right? Maybe there is a case in this test where this is possible. It only seems repro with a debug build.

:apavel

Just testing the fix right now, there were a few cases in talos tests I missed sorry.
Flags: needinfo?(jkt)
Keywords: checkin-needed
We cannot apply the patches because they were not modified since they were landed 28 days ago.
Could you please attach the patches again and request check in?
Flags: needinfo?(jkt)
The update was to the Tests yesterday I think bugzilla marks phabricator attachments from the original attachment. The patch was backed out yesterday (https://bugzilla.mozilla.org/show_bug.cgi?id=1362034#c36) and I updated that on phabricator.

I can see the change I made on "diff 8" under history in: https://phabricator.services.mozilla.com/D2047 However both patches are needed to land.

Is there anything else I need to do here, I haven't dealt with a phabricator backout before.
Flags: needinfo?(jkt) → needinfo?(aiakab)
Pushed by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f68b1b76af36
Have addTab() provide the correct triggering principal. r=ckerschb,Gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c3329958b8a
Tests for addTab() to provide the correct triggering principal. r=ckerschb,Gijs
Keywords: checkin-needed
Backed out 2 changesets (Bug 1362034) for failure at browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e643a04b3684d71d7a7e68ba82216665149b193e

Push link:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=1c3329958b8a7603f04149ad4ab8e128ab5421e1&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-classifiedState=unclassified

Failure log:
https://treeherder.mozilla.org/logviewer.html#?job_id=192843387&repo=mozilla-inbound&lineNumber=3913

18:41:09     INFO - TEST-INFO | screenshot: exit 0
18:41:09     INFO - Buffered messages logged at 18:38:41
18:41:09     INFO - Entering test bound setup
18:41:09     INFO - Buffered messages finished
18:41:09     INFO - TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js | Unexpected exception in [ tabsInTitlebar, fourPinned, maximized, onlyNavBar, noLWT, compactDensity ]: Error: Required argument triggeringPrincipal missing within addTab - 
18:41:09     INFO - Stack trace:
18:41:09     INFO - chrome://mozscreenshots/content/TestRunner.jsm:_performCombo:359
18:41:09     INFO - chrome://mozscreenshots/content/TestRunner.jsm:start:132
18:41:09     INFO - chrome://mochitests/content/browser/browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js:capture:15
18:41:09     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest/<:1106
18:41:09     INFO - chrome://mochikit/content/browser-test.js:Tester_execTest:1097
18:41:09     INFO - chrome://mochikit/content/browser-test.js:nextTest/<:999
18:41:09     INFO - chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<:795
18:41:09     INFO - 	Error: Required argument triggeringPrincipal missing within addTab
18:41:09     INFO - 	addTab@chrome://browser/content/tabbrowser.js:2204:13
18:41:09     INFO - applyConfig@chrome://mozscreenshots/content/configurations/Tabs.jsm:43:19
18:41:09     INFO - async*changeConfig@chrome://mozscreenshots/content/TestRunner.jsm:307:42
18:41:09     INFO - _performCombo@chrome://mozscreenshots/content/TestRunner.jsm:328:32
18:41:09     INFO - async*start@chrome://mozscreenshots/content/TestRunner.jsm:132:13
18:41:09     INFO - async*capture@chrome://mochitests/content/browser/browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js:15:9
18:41:09     INFO - Async*Tester_execTest/<@chrome://mochikit/content/browser-test.js:1106:34
18:41:09     INFO - async*Tester_execTest@chrome://mochikit/content/browser-test.js:1097:16
18:41:09     INFO - nextTest/<@chrome://mochikit/content/browser-test.js:999:9
18:41:09     INFO - SimpleTest.waitForFocus/waitForFocusInner/focusedOrLoaded/<@chrome://mochikit/content/tests/SimpleTest/SimpleTest.js:795:59
18:41:09     INFO - 
18:41:09     INFO - Combination 092/540: tabsInTitlebar_fourPinned_maximized_onlyNavBar_noLWT_normalDensity
18:41:09     INFO - promising fourPinned
18:41:09     INFO - calling fourPinned
18:41:09     INFO - called fourPinned
18:41:09     INFO - Not taking screenshot here: see the one that was previously logged
18:41:09     INFO - TEST-UNEXPECTED-FAIL | browser/tools/mozscreenshots/primaryUI/browser_primaryUI.js | Unexpected exception in [ tabsInTitlebar, fourPinned, maximized, onlyNavBar, noLWT, normalDensity ]: Error: Required argument triggeringPrincipal missing within addTab -
Flags: needinfo?(jkt)
--- a/browser/base/content/test/tabs/browser_positional_attributes.js
+++ b/browser/base/content/test/tabs/browser_positional_attributes.js
@@ -1,41 +1,43 @@
 /* Any copyright is dedicated to the Public Domain.
  * http://creativecommons.org/publicdomain/zero/1.0/
  */
 
 var tabs = [];
 
-function addTab(aURL) {
-  tabs.push(gBrowser.addTab(aURL, {skipAnimation: true}));
+function addTab(aURL, done) {
+  tabs.push(BrowserTestUtils.addTab(gBrowser, aURL, {
+    skipAnimation: true,
+  }, done));
 }
 
 function switchTab(index) {
   return BrowserTestUtils.switchTab(gBrowser, gBrowser.tabs[index]);
 }
 
 function testAttrib(tabIndex, attrib, expected) {
   is(gBrowser.tabs[tabIndex].hasAttribute(attrib), expected,
      `tab #${tabIndex} should${expected ? "" : "n't"} have the ${attrib} attribute`);
 }
 
 add_task(async function setup() {
   is(gBrowser.tabs.length, 1, "one tab is open initially");
 
-  addTab("http://mochi.test:8888/#0");
-  addTab("http://mochi.test:8888/#1");
-  addTab("http://mochi.test:8888/#2");
-  addTab("http://mochi.test:8888/#3");
+  await new Promise(r => addTab("http://mochi.test:8888/#0", r));
+  await new Promise(r => addTab("http://mochi.test:8888/#1", r));
+  await new Promise(r => addTab("http://mochi.test:8888/#2", r));
+  await new Promise(r => addTab("http://mochi.test:8888/#3", r));


This doesn't seem to make sense. You're waiting for the TabOpen event which is dispatched synchronously anyway.

(I wish I could comment in phabricator and the comment would show up here. *sigh*)
> I wish I could comment in phabricator and the comment would show up here

I think it was because the patch was closed as it got backed out. I removed this code thanks, it was an attempt to fix the previous issue I was having. Thanks!
Flags: needinfo?(jkt)
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f194a48a48bb
Tests for addTab() to provide the correct triggering principal. r=ckerschb r?=gijs
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f3907a1da34
Have addTab() provide the correct triggering principal. r=ckerschb r=Gijs
Pushed by jkingston@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9db45f8500a9
Fixup for tests for addTab() to provide the correct triggering principal.
https://hg.mozilla.org/mozilla-central/rev/f194a48a48bb
https://hg.mozilla.org/mozilla-central/rev/6f3907a1da34
https://hg.mozilla.org/mozilla-central/rev/9db45f8500a9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
Depends on: 1482681
Depends on: 1483025
Depends on: 1485307
Depends on: 1485778
Depends on: 1486119
Depends on: 1488914
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: