As agreed with Gijs over IRC (see also [1]) we should not reuse originPrincipal as the triggeringPrincipal within utilitOverlay.js. We introduced that behavior within [2]. I don't think it's critical at the moment, but we should fix that, probably by passing an explicit triggeringPrincipal argument. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1343279#c4 [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1331686

Gijs, I mostly based this patch on the patch that introduced OriginPrincipal [1]. Initial testing seems promising to me. Could you take a first look if you agree? In addition to running automated tests its crucial that we not regress view-image on canvas [2] which still works with this patch applied. Here is a try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=268433065945f83ed94da3b4da2b3b4ac5d11e6d Anything else I am missing? [1] https://bugzilla.mozilla.org/attachment.cgi?id=8793491&action=diff [2] https://bugzilla.mozilla.org/show_bug.cgi?id=1343279#c0

In fact we should fix this bug before moving on to fixing Bug 1333030, otherwise it gets too complicated.

Comment on attachment 8852886 [details] [diff] [review] bug_1344706_do_not_reuse_originprincipal.patch Review of attachment 8852886 [details] [diff] [review]: ----------------------------------------------------------------- This looks OK at a glance for what this bug is about... though it's raised some other questions with me. I'll set needinfo and hopefully I'll have time to email you and a few other folks later today, otherwise tomorrow. ::: browser/base/content/utilityOverlay.js @@ +260,5 @@ > // opening a link in a new private window, or in a new container tab. > // Please note we do not have to do that for SystemPrincipals and we > // can not do it for NullPrincipals since NullPrincipals are only > // identical if they actually are the same object (See Bug: 1346759) > + if (aTriggeringPrincipal && aTriggeringPrincipal.isCodebasePrincipal) { Do we not need to keep doing this for aPrincipal?

(In reply to :Gijs from comment #3) > ::: browser/base/content/utilityOverlay.js > @@ +260,5 @@ > > // opening a link in a new private window, or in a new container tab. > > // Please note we do not have to do that for SystemPrincipals and we > > // can not do it for NullPrincipals since NullPrincipals are only > > // identical if they actually are the same object (See Bug: 1346759) > > + if (aTriggeringPrincipal && aTriggeringPrincipal.isCodebasePrincipal) { > > Do we not need to keep doing this for aPrincipal? I think you are right and we need to keep doing this for aPrincipal as well.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/fdb11fbb53cf Do not reuse originPrincipal as triggeringPrincipal within utilityOverlay.js. r=gijs

backed out for eslint failure like TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/base/content/utilityOverlay.js:197:1 | Function 'openLinkIn' has a complexity of 44. (complexity)

Backout by cbook@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/48747e70be14 Backed out changeset fdb11fbb53cf for eslint failure

https://pastebin.mozilla.org/8984047 Should be enough to shut up the warning, and simplify the principal-changing code a tiny bit.

(In reply to :Gijs from comment #9) > https://pastebin.mozilla.org/8984047 > > Should be enough to shut up the warning, and simplify the principal-changing > code a tiny bit. Thanks - that shuts up the warning.

Pushed by mozilla@christophkerschbaumer.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/76b5194257a9 Do not reuse originPrincipal as triggeringPrincipal within utilityOverlay.js. r=gijs