loadURI in browser.js doesn't throw if we don't have a valid triggeringPrincipal.

Depends on D4550

Depends on D4551

Depends on D4552

Depends on D4555

Depends on D4556

Depends on D4557

Depends on D4558

Depends on D4560

Depends on D4561

Comment on attachment 9004920 [details] Bug 1485305 - misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs :Gijs (he/him) has approved the revision.

Comment on attachment 9004907 [details] Bug 1485305 - accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov alexander :surkov (:asurkov) has approved the revision.

Comment on attachment 9004915 [details] Bug 1485305 - devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins Brian Grinstead [:bgrins] has approved the revision.

Comment on attachment 9004909 [details] Bug 1485305 - browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop Dave Townsend [:mossop] has approved the revision.

Comment on attachment 9004923 [details] Bug 1485305 - toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop Dave Townsend [:mossop] has approved the revision.

Comment on attachment 9004917 [details] Bug 1485305 - dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika :Nika Layzell has approved the revision.

Comment on attachment 9004908 [details] Bug 1485305 - browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop Dave Townsend [:mossop] (he/him) has approved the revision.

Comment on attachment 9004921 [details] Bug 1485305 - toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop Kris Maglione [:kmag] has approved the revision.

Chris please could you take a look over this now that you are back :) Thanks!

(In reply to Jonathan Kingston [:jkt] from comment #19) > Chris please could you take a look over this now that you are back :) Overall this looks pretty good. I haven't looked at any of the tests because it seems they mostly rely on BrowserTestUtils anyway and we have covered that in other bugs/patches. Anyway, leaving a few notes here: * Patch: "browser/ tests Ensure loadURI always passes a triggeringPrincipal()" Within browser/ you sometimes use SystemPrincipal and sometimes you use a NullPrincipal, I couldn't figure out when you used which one. Why not always use NullPrincipal? It seems it should work? * Patch: devtools/ Ensure loadURI always passes a triggeringPrincipal() Within devtools/client/responsive.html/browser/swap.js you use systemPrincipal for the ContainerURL which seems scary, can't we use NullPrincipal in that case? In addition I think it needs the right userContextId. Additonal note: Within web-navigation.js there is a function loadURIWithOptions where we fall back to using a NullPrincipal in case the triggeringPrincipal is not passed in. Is it worth making sure that this function always receives a non-null triggeringPrincipal or is the fallback fine? *Patch: dom/ Ensure loadURI always passes a triggeringPrincipal() Why are we falling back to using SystemPrincipal within toolkit/actors/WebNavigationChild.jsm? That doesn't seem right to me. Also toolkit/components/extensions/ExtensionXPCShellUtils.jsm uses SystemPrincipal within loadURI, that seems scary to me as well.

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #20) > > * Patch: "browser/ tests Ensure loadURI always passes a > triggeringPrincipal()" > Within browser/ you sometimes use SystemPrincipal and sometimes you use a > NullPrincipal, I couldn't figure out when you used which one. > Why not always use NullPrincipal? It seems it should work? - For data urls to simlify checking I used System rather than creating a codebase etc. - I see many tests loading pages like about:preferences or other system pages which wouldn't be openable via the web. - Mostly I have used System in tests to reduce the amount of checking for these cases. - When loading about:blank or other explicit web URLs, I explicitly used NullPrincipal as it's clear it will always work. (The intent is to safely land these patches with minimal rework, overall even the test SystemPrincipals we can hopefully remove where possible). > * Patch: devtools/ Ensure loadURI always passes a triggeringPrincipal() > Within devtools/client/responsive.html/browser/swap.js you use > systemPrincipal for the ContainerURL which seems scary, can't we use > NullPrincipal in that case? In addition I think it needs the right > userContextId. The current use to swapToInnerBrowser is currently hard coded uses to links like: "chrome://devtools/content/responsive.html/index.xhtml" > > Additonal note: Within web-navigation.js there is a function > loadURIWithOptions where we fall back to using a NullPrincipal in case the > triggeringPrincipal is not passed in. Is it worth making sure that this > function always receives a non-null triggeringPrincipal or is the fallback > fine? I think we should do this as a follow up also. > *Patch: dom/ Ensure loadURI always passes a triggeringPrincipal() > Why are we falling back to using SystemPrincipal within > toolkit/actors/WebNavigationChild.jsm? That doesn't seem right to me. This is due to things like SessionRestore where the triggering principal can't be unserialized. I will file a follow up to this as I agree but it broke a fair number of things. > Also toolkit/components/extensions/ExtensionXPCShellUtils.jsm uses > SystemPrincipal within loadURI, that seems scary to me as well. I can't remember the exact failures here but again I'll file a bug and add it to the comment.

Hey :smaug / :bz, Not sure if you got a notification for https://phabricator.services.mozilla.com/D4556 are you able to review or give to someone else? Thanks! > https://phabricator.services.mozilla.com/D4560 adds the default of systemPrincipal for BrowserTestUtils. > https://phabricator.services.mozilla.com/D4551 adds the requirement in browser.js that a triggeringPrincipal is explicitly passed. > The motivation is to pass an explicit triggeringPrincipal to all loads going into docshell so we can stop using an implied systemPrincipal as a default in the docshell code. Hey :heycam / :dbaron are you able to review https://phabricator.services.mozilla.com/D4558 also, I put in the mentioned fixes. Sorry for pinging you all, just these patches bitrot quite fast and I would like to get them in early into the cycle to fix any fallout it causes. Thanks in advance!

This is not in my phabricator review queue.

I got notifications, but I've been on a bunch of unexpected PTO over the last few weeks so I'm way behind. Looking now.

Comment on attachment 9004916 [details] Bug 1485305 - docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins Boris Zbarsky [:bzbarsky, bz on IRC] has approved the revision.

Comment on attachment 9004918 [details] Bug 1485305 - layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs David Baron :dbaron: 🏴󠁵󠁳󠁣󠁡󠁿 ⌚UTC-7 has approved the revision.

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/6263695b3cb8 accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/2fee48378f73 browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/03632075ac2c browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/f40900bf8621 devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/35bd32f99f60 docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/e02038177b6b dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika https://hg.mozilla.org/integration/mozilla-inbound/rev/e9da73786c5f layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/721871bb64f1 misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/bf0f2adb765e toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/63c50fd60ae4 toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Backed out 10 changesets for failures at browser/content/browser.js Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/64c64fc5e163e3299803d71606582a058e2cf43b Push with the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=199925981&revision=63c50fd60ae4d343533a4b5541489d706409a230 Failure link: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&selectedJob=199926336&revision=63c50fd60ae4d343533a4b5541489d706409a230 Relevant log file: https://treeherder.mozilla.org/logviewer.html#?job_id=199926336&repo=mozilla-inbound&lineNumber=9972

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4eb33bf25d6f accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/bca973d90acc browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/61aa1cfb0b01 browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/3a111e9e5c9c devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaccb374a59 docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/8085d1eefd7c dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika https://hg.mozilla.org/integration/mozilla-inbound/rev/2f8a5a03ccb5 layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/cd063d8afe4e misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/d9f04aeeeef7 toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/138b8596a9cd toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Backed out 10 changesets (bug 1485305)for failing browser chrome tests on browser_loadDisallowInherit.js Backout revision https://hg.mozilla.org/integration/mozilla-inbound/rev/95a55456863eb47670c06465b331188a403b474d Failed push https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=testfailed%2Cbusted%2Cexception&classifiedState=unclassified&selectedJob=199982956&revision=138b8596a9cd09c3614279a8a3d24561b07c3121 And more exactly the bc3 failures started from revision https://hg.mozilla.org/integration/mozilla-inbound/rev/3aaccb374a5922905f56f1e76dd5e676b22dda01 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=199982956&repo=mozilla-inbound for bc3. This failure migrated on bc1,bc6,bc7 :jkt could you please take a look?

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/238d92348159 accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/4b5c9d5929fc browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/1b74152cabc1 browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/323773a395cc devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/acce14683c13 docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/e698f2fc1c1a dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika https://hg.mozilla.org/integration/mozilla-inbound/rev/75220b2f6669 layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c99b97b4348b misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/a05e40ef7215 toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/50439ec01661 toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

Backed out 10 changesets (Bug 1485305) for browser-chrome failures on docshell/test/browser/browser_loadURI.js. Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/8e4824d6f1357859bc4a374433ab282d7741c28b Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&resultStatus=success,testfailed,busted,exception&revision=50439ec0166116f4d6e80e1af2616be1cf22e95e&selectedJob=200195358 Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=200195358&repo=mozilla-inbound&lineNumber=3374

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/d9840fe72bb5 accessible/ Ensure loadURI always passes a triggeringPrincipal() r=surkov https://hg.mozilla.org/integration/mozilla-inbound/rev/a3ee5b2a7bf2 browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/9b125055e5e0 browser/ tests Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/e1bac78c8df5 devtools/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/55115a3dfb82 docshell/ Ensure loadURI always passes a triggeringPrincipal() r=bgrins https://hg.mozilla.org/integration/mozilla-inbound/rev/542c2d7a2c1d dom/ Ensure loadURI always passes a triggeringPrincipal() r=Nika https://hg.mozilla.org/integration/mozilla-inbound/rev/a253b24f69b2 layout/ Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/c1da667c41b3 misc Ensure loadURI always passes a triggeringPrincipal() r=Gijs https://hg.mozilla.org/integration/mozilla-inbound/rev/cc21f29f364b toolkit/mozapps/extensions/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop https://hg.mozilla.org/integration/mozilla-inbound/rev/c249c91dc8ca toolkit/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop

https://hg.mozilla.org/mozilla-central/rev/d9840fe72bb5 https://hg.mozilla.org/mozilla-central/rev/a3ee5b2a7bf2 https://hg.mozilla.org/mozilla-central/rev/9b125055e5e0 https://hg.mozilla.org/mozilla-central/rev/e1bac78c8df5 https://hg.mozilla.org/mozilla-central/rev/55115a3dfb82 https://hg.mozilla.org/mozilla-central/rev/542c2d7a2c1d https://hg.mozilla.org/mozilla-central/rev/a253b24f69b2 https://hg.mozilla.org/mozilla-central/rev/c1da667c41b3 https://hg.mozilla.org/mozilla-central/rev/cc21f29f364b https://hg.mozilla.org/mozilla-central/rev/c249c91dc8ca

Commit pushed to master at https://github.com/mozilla/activity-stream https://github.com/mozilla/activity-stream/commit/fc1be991a4d06316edb0e588ae98f7b7577fa1b9 Port Bug 1485305 - browser/ Ensure loadURI always passes a triggeringPrincipal() r=Mossop (#4445)