In ext-tabs.js, the allowInheritPrincipal and triggeringPrincipal logic has grown into something complicated. Initially, ext-tabs.js unconditionally used options.disallowInheritPrincipal=true. Then it was made conditional, and the motivation for doing so was: > // Only set disallowInheritPrincipal on non-discardable urls as it > // will override creating a lazy browser. https://searchfox.org/mozilla-central/diff/aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/parent/ext-tabs.js#572-573 The comment seems to imply that the disallowInheritPrincipal flag should not be touched unless necessary. In bug 1466801, disallowInheritPrincipal was flipped to allowInheritPrincipal: https://searchfox.org/mozilla-central/diff/a91f8e81081b54280c841d480331e19f6ba562f4/browser/components/extensions/parent/ext-tabs.js#576 Now the logic is: > // Only set allowInheritPrincipal on discardable urls as it > // will override creating a lazy browser. Setting triggeringPrincipal > // will ensure other cases are handled, but setting it may prevent > // creating about and data urls. > let discardable = url && !url.startsWith("about:"); > if (!discardable) { > options.allowInheritPrincipal = false; > ... > } else { > options.allowInheritPrincipal = true; > ... https://searchfox.org/mozilla-central/rev/c8483bebfa09173b1db5590b3c19f42270d819c9/browser/components/extensions/parent/ext-tabs.js#571-588 The "allowInheritPrincipal = true;" line should be removed, and the surrounding code can be simplified now that "allowInheritPrincipal" defaults to false. There won't be behavioral changes, because extensions cannot open data: or javascript:-URLs anyway via browser.tabs.create.

Shane, why was it necessary to limit "disallowInheritPrincipal = true;" to about:-URLs only? What were the consequences of unconditionally setting (dis)allowInheritPrincipal? https://searchfox.org/mozilla-central/diff/aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/parent/ext-tabs.js#572-583

Thanks for filing this, I meant to make a follow up for this too. Also the triggeringPrincipal is calculated twice in this function because of merges. The code is correct but could be equally cleaned up in this bug.

Making moco confidential after initial investigation into the functions based on Comment 2.

There is more code in https://hg.mozilla.org/mozilla-central/rev/fb52c3f74edc684c1fa99b7f909af2a2a9c86a6f such as https://searchfox.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3806 where the resulting logic has become redundant due to the change from bug 1466801 (where principals became non-inheritable by default). PS. None of the issues reported here are sensitive, so re-opening the bug should be fine.

I'm not sure if it is redundant mobile/android/components/extensions/ext-tabs.js#284 is still using that code however it should be flipped to be allowInheritPrincipal as all the other call sites are. I think we are good for security after having a closer look. https://treeherder.mozilla.org/#/jobs?repo=try&revision=904af94208842016e6f41a4e328df523cda36399

Pretty sure we can't land this as is. The patch creates these test failures and I think this is because of the inheritPrincipal removal here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c1ae2ec2375498773e5495b177481ae78d99b5f

(already answered in Phabricator/IRC)

(In reply to Rob Wu [:robwu] from comment #1) > Shane, why was it necessary to limit "disallowInheritPrincipal = true;" to > about:-URLs only? > What were the consequences of unconditionally setting > (dis)allowInheritPrincipal? > > https://searchfox.org/mozilla-central/diff/ > aad6d3674f408b409b27b8c294515800c28aad1f/browser/components/extensions/ > parent/ext-tabs.js#572-583 using the new name, allowInheritPrincipal... If allowInheritPrincipal is false, then b.loadURI is called. This causes the lazy browser (just created) to become a real browser, preventing the ability to create a discarded tab. https://searchfox.org/mozilla-central/rev/c3fef66a5b211ea8038c1c132706d02db408093a/browser/base/content/tabbrowser.js#2493

The (dis)allowInheritPrincipal was introduced for in bug 1310331 - https://searchfox.org/mozilla-central/diff/2c891cc35b174ee93e592e66a9c0e7e677ec1e49/browser/base/content/tabbrowser.xml#2246 Before that patch, there was only a check for about:blankness (blank frames did not result in an additional loadURI call). This flag is not obviously related to discard/lazy browsers, so using this flag for its side effect seems wrong to me. I think that the right fix is to modify tihis condition and add "&& !createLazyBrowser": https://searchfox.org/mozilla-central/rev/5a18fb5aeeec99f1ca1c36a697082c221189a3b9/browser/base/content/tabbrowser.js#2495

What is the status on finishing this?