Instead of rewriting all the tests to use gBrowser.addTab(uri, {triggeringPrincipal}) we should add BrowsertestUtils.addTab() and provide a triggeringPrincipal in all the cases tests don't provide a triggeringPrincipal explicitly.

Florian, I wrote a script that replaces all the occurrences of gBrowser.addTab(...) to call BrowserTestUtils.addTab(...). I hope this is what you had in mind as well.

Comment on attachment 8866314 [details] [diff] [review] bug_1362993_add_browsertestutils_addtab.patch >+ * Opens a tab with a given uri and params object. If the params object is not set >+ * or the params parameter does not include a triggeringPricnipal then this function >+ * provides a params object using the systemPrincipal as the default triggeringPrincipal. >+ */ >+ addTab(browser, uri, params) { >+ if (!params) { >+ params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() } >+ } >+ else if (!params.triggeringPrincipal) { >+ params.triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); >+ } Why doesn't gBrowser.addTab do this by default?

(In reply to Dão Gottwald [::dao] from comment #3) > Comment on attachment 8866314 [details] [diff] [review] > bug_1362993_add_browsertestutils_addtab.patch > > >+ * Opens a tab with a given uri and params object. If the params object is not set > >+ * or the params parameter does not include a triggeringPricnipal then this function > >+ * provides a params object using the systemPrincipal as the default triggeringPrincipal. > >+ */ > >+ addTab(browser, uri, params) { > >+ if (!params) { > >+ params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() } > >+ } > >+ else if (!params.triggeringPrincipal) { > >+ params.triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); > >+ } > > Why doesn't gBrowser.addTab do this by default? Because it's an insecure default - see the discussion in bug 1362329 comment #3 and further. If we add the system principal as the default in gBrowser.addTab(), any cases where we fail to pass the right content/unprivileged principal end up being security vulnerabilities.

Comment on attachment 8866314 [details] [diff] [review] bug_1362993_add_browsertestutils_addtab.patch Review of attachment 8866314 [details] [diff] [review]: ----------------------------------------------------------------- ::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm @@ +1369,5 @@ > + * Opens a tab with a given uri and params object. If the params object is not set > + * or the params parameter does not include a triggeringPricnipal then this function > + * provides a params object using the systemPrincipal as the default triggeringPrincipal. > + */ > + addTab(browser, uri, params) { You can give {} as the default value for 'params', so that you don't need the first 'if (!params) check.

Comment on attachment 8866315 [details] [diff] [review] bug_1362993_rewrite_tests_to_use_browsertestutils_addtab.patch Review of attachment 8866315 [details] [diff] [review]: ----------------------------------------------------------------- I assume your script was a simple search & replace. What I had in mind is a script using the JS parser to rewrite some of the code (see bug 1362882 for an example). From grepping your patch ($ egrep -C 2 '^\+[^\+]+[^;]$' attachment.cgi\?id\=8866315) I see that this broke the indent in only 3 places so it'll be easier to just fix them by hand. ::: browser/base/content/test/general/browser_bug537013.js @@ +19,1 @@ > aText + "</h1>"); please fix indent here. ::: devtools/client/webconsole/test/browser_console_private_browsing.js @@ +28,1 @@ > "<p>hello world! I am not private!"); here ::: toolkit/components/passwordmgr/test/browser/browser_hasInsecureLoginForms_streamConverter.js @@ +86,1 @@ > "passwordmgr/test/browser/streamConverter_content.sjs", and here.

(In reply to Florian Quèze [:florian] [:flo] from comment #5) > You can give {} as the default value for 'params', so that you don't need > the first 'if (!params) check. Yeah, makes sense, thanks!

(In reply to Florian Quèze [:florian] [:flo] from comment #6) > I assume your script was a simple search & replace. What I had in mind is a > script using the JS parser to rewrite some of the code (see bug 1362882 for > an example). Indeed, very simple string replacement script - next time we can hack up something more sophisticated. > From grepping your patch ($ egrep -C 2 '^\+[^\+]+[^;]$' > attachment.cgi\?id\=8866315) I see that this broke the indent in only 3 > places so it'll be easier to just fix them by hand. Done - thanks for pointing that out.

I only test so many tests locally. I suppose BrowserTestUtils is available everywhere, let's make sure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5022599851c713b72be1f72c61737123af722199

(In reply to :Gijs from comment #4) > (In reply to Dão Gottwald [::dao] from comment #3) > > Comment on attachment 8866314 [details] [diff] [review] > > bug_1362993_add_browsertestutils_addtab.patch > > > > >+ * Opens a tab with a given uri and params object. If the params object is not set > > >+ * or the params parameter does not include a triggeringPricnipal then this function > > >+ * provides a params object using the systemPrincipal as the default triggeringPrincipal. > > >+ */ > > >+ addTab(browser, uri, params) { > > >+ if (!params) { > > >+ params = { triggeringPrincipal: Services.scriptSecurityManager.getSystemPrincipal() } > > >+ } > > >+ else if (!params.triggeringPrincipal) { > > >+ params.triggeringPrincipal = Services.scriptSecurityManager.getSystemPrincipal(); > > >+ } > > > > Why doesn't gBrowser.addTab do this by default? > > Because it's an insecure default - see the discussion in bug 1362329 comment > #3 and further. If we add the system principal as the default in > gBrowser.addTab(), any cases where we fail to pass the right > content/unprivileged principal end up being security vulnerabilities. Can we have a pref to set the default only for tests? Outlawing simple addTab calls in tests is counterintuitive and seems like a footgun.

(In reply to Dão Gottwald [::dao] from comment #10) > Can we have a pref to set the default only for tests? Outlawing simple > addTab calls in tests is counterintuitive and seems like a footgun. I am not sure if that's easily possible, but also I don't fully understand what the footgun is here? BrowserTestUtils.addTab() just forwards the call to gBrowser.addTab() setting the systemPrincipal as the triggeringPrincipal for all browser mochitests. All of our Gecko code will be updated to pass a valid triggeringPrincipal (see Bug 1333030 and dependencies) so we can reason where the load is coming from. Obviously Addons can also use whatever front end API we provide not providing that principal, but at least within gecko we can reason about 'where the load is coming from' and what security mechanisms we need to enforce.

(In reply to Dão Gottwald [::dao] from comment #10) > (In reply to :Gijs from comment #4) > > (In reply to Dão Gottwald [::dao] from comment #3) > > > Why doesn't gBrowser.addTab do this by default? > > > > Because it's an insecure default - see the discussion in bug 1362329 comment > > #3 and further. If we add the system principal as the default in > > gBrowser.addTab(), any cases where we fail to pass the right > > content/unprivileged principal end up being security vulnerabilities. > > Can we have a pref to set the default only for tests? That will mask issues with 'real' new non-test consumers calling addTab() without a principal - they will pass in tests but fail in the 'real world', which also doesn't seem desirable. Do you see another way around this problem?

Comment on attachment 8867092 [details] [diff] [review] bug_1362993_add_browsertestutils_addtab.patch Review of attachment 8867092 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thanks! Please have a look at your try results. I would retrigger the 'c1' failures a couple times to ensure they are actually intermittent. And you have a bunch of eslint errors to fix.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/08e52e63564d Add BrowserTestUtils.addTab() which provides triggeringPrincipal for params object. r=florian https://hg.mozilla.org/integration/mozilla-inbound/rev/a2b7eaff1268 Rewrite gBrowser.addTab() to use BrowserTestUtils.addTab(). r=florian

I'm not sure why this hasn't broken talos tests on inbound (I suspect it's because inbound magically uses signed/packaged versions of the talos add-ons), but on try and in local runs this breaks the talos tabpaint tests, because BrowserTestUtils is not defined in its bootstrap.js scope.

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #15) > I'm not sure why this hasn't broken talos tests on inbound (I suspect it's > because inbound magically uses signed/packaged versions of the talos > add-ons), but on try and in local runs this breaks the talos tabpaint tests, > because BrowserTestUtils is not defined in its bootstrap.js scope. oh oh - Kris, what is the best way to fix this?

Most likely just backout the change to this line: http://searchfox.org/mozilla-central/rev/ae24a3c83d22e0e35aecfd9049c2b463ca7e045b/testing/talos/talos/tests/tabpaint/bootstrap.js#159 But it would probably be a good idea to make sure nothing else accidentally got broken the same way. I only noticed this because it broke my try runs.

Kris, thanks for pointing that out. I suppose we can just push that as a follow up, passing an explicit params object. What do you think? Should work, because Services is used elsewhere in that file.

Comment on attachment 8868016 [details] [diff] [review] bug_1362993_tabpaint_followup.patch Review of attachment 8868016 [details] [diff] [review]: ----------------------------------------------------------------- r=me Someone will need to repackage and sign this so the new version is used for non-try builds, though.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d479f76dc5e8 Followup: Use gBrowser.addTab with explicit triggeringPricnipal instead of BrowserTestUtils. r=kmaglione

(In reply to Kris Maglione [:kmag] (busy; behind on reviews) from comment #19) > Someone will need to repackage and sign this so the new version is used for > non-try builds, though. Kris, I pushed to that inbound so that all changes can land together on central. If you tell me how to package then I'll file a follow up and get that ready to land.

It needs to be done by someone with privileges to sign those add-ons.

https://hg.mozilla.org/mozilla-central/rev/08e52e63564d https://hg.mozilla.org/mozilla-central/rev/a2b7eaff1268 https://hg.mozilla.org/mozilla-central/rev/d479f76dc5e8

let me sign the tabpaint addon

I will sign the tabpaint addon in bug 1365289