Have addTab() provide the correct triggeringPrincipal

RESOLVED FIXED in Firefox 63

Status

()

P2
normal
RESOLVED FIXED
a year ago
4 days ago

People

(Reporter: ckerschb, Assigned: jkt, NeedInfo)

Tracking

(Depends on: 3 bugs, Blocks: 2 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

(Whiteboard: [domsecurity-active])

Attachments

(2 attachments, 5 obsolete attachments)

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
(Reporter)

Updated

a year ago
Assignee: nobody → ckerschb
Blocks: 1333030
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [domsecurity-active]
(Reporter)

Updated

a year ago
No longer blocks: 1333030
(Reporter)

Updated

a year ago
Blocks: 1333030
(Reporter)

Updated

a year ago
Depends on: 1363975
(Reporter)

Updated

a year ago
Depends on: 1363977
(Reporter)

Updated

a year ago
Depends on: 1364392
(Reporter)

Comment 4

a year ago
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(),
> });
(Reporter)

Updated

6 months ago
Assignee: ckerschb → jkt
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

4 months ago
Depends on: 1461707
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 17

3 months ago
Created attachment 8989695 [details]
Bug 1362034 - Have addTab() provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!
(Assignee)

Comment 18

3 months ago
Hey :kmag,

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

Thanks
Flags: needinfo?(kmaglione+bmo)
(Assignee)

Updated

3 months ago
Attachment #8975474 - Attachment is obsolete: true
(Assignee)

Comment 19

3 months ago
Created attachment 8990439 [details]
Bug 1362034 - Have addTab() provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!

Updated

3 months ago
Attachment #8989695 - Attachment is obsolete: true
(Assignee)

Comment 20

3 months ago
Created attachment 8990444 [details]
Bug 1362034 - Tests for addTab() to provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!
(Assignee)

Comment 21

3 months ago
With the changes I see a new error that needs fixing: browser/components/search/test/browser_contextSearchTabPosition.js

Comment 22

2 months ago
Both of the diffs seem to contain all the changes (both test and non-test) now...

Updated

2 months ago
Attachment #8990444 - Attachment is obsolete: true

Updated

2 months ago
Attachment #8990439 - Attachment is obsolete: true
(Assignee)

Comment 23

2 months ago
Created attachment 8990941 [details]
Bug 1362034 - Have addTab() provide the correct triggering principal.

Updated

2 months ago
Attachment #8990941 - Attachment is obsolete: true
(Assignee)

Comment 24

2 months ago
Created attachment 8990978 [details]
Bug 1362034 - Have addTab() provide the correct triggering principal.
(Assignee)

Comment 25

2 months ago
Created attachment 8990980 [details]
Bug 1362034 - Tests for addTab() to provide the correct triggering principal. r?ckerschb! r?gijs! r?kmag!

Depends on D2046
(Assignee)

Updated

2 months ago
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+

Comment 30

2 months ago
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.
(Assignee)

Comment 31

2 months ago
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.
(Assignee)

Comment 32

2 months ago
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
(Assignee)

Comment 33

2 months ago
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)
(Assignee)

Comment 34

2 months ago
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)
(Assignee)

Updated

a month ago
Keywords: checkin-needed

Comment 35

a month ago
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.
(Assignee)

Comment 39

a month ago
::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)
(Assignee)

Updated

a month ago
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)
(Assignee)

Comment 41

a month ago
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)

Comment 42

a month ago
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*)
(Assignee)

Comment 45

a month ago
> 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)

Comment 46

a month ago
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

Comment 47

a month ago
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.

Comment 48

a month ago
bugherder
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
Last Resolved: a month ago
status-firefox63: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63

Updated

a month ago
Depends on: 1482681

Updated

a month ago
Depends on: 1483025

Updated

28 days ago
Depends on: 1485307

Updated

27 days ago
Depends on: 1485778
(Assignee)

Updated

26 days ago
Depends on: 1486119

Updated

4 days ago
Depends on: 1488914
You need to log in before you can comment on or make changes to this bug.