As discussed within Bug 1343279, in particular comment 27, 28 and 29 [1] we should consider to _not_ rely on pointer comparison [2] of NullPrincipals because principals might have been serialized. [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1343279#c27 [2] https://dxr.mozilla.org/mozilla-central/source/caps/nsNullPrincipal.cpp#144

Let's see what eggs break: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a936d550559ea7344e5373e71b027b3e2fa5f305

The only test I see failing is: https://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js Log: https://treeherder.mozilla.org/logviewer.html#?job_id=210815020&repo=try&lineNumber=14391

What's the perf impact?

The patch in question just changes the comparison, afaict. It does not change the prng used.

> The patch in question just changes the comparison, afaict. It does, however I was told the e10s changes already did this to make these thread safe. Perhaps I'm misunderstanding that though.

The single test failure appears to be a race condition caused by calling isInstallAllowed() in resource://gre/modules/addons/XPIInstall.jsm twice in the updated case.

The relevant code is nsUUIDGenerator::GenerateUUIDInPlace and does the following: Windows: CoCreateGuid. I don't believe this is cryptographically secure in any way, nor is it really using a prng, afaict. Mac: CFUUIDCreate. I can't quickly find information on this. If HAVE_ARC4RANDOM is defined (which is when? I don't think it is on Linux...): use arc4random Else: use random() So unless I am missing something, we are not using a csprng for this stuff.

My understanding is the uuid crate in rust uses only a csprng so I moved the code to using that. It will need some work to neaten it up I suspect however I think this might be a viable solution so long as talos is happy. Try run here: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e0a376baca47f211517f7ec8f338d9980f03ce44

Perfherder here: https://treeherder.mozilla.org/perf.html#/compare?originalProject=try&originalRevision=ebb8f453dd61397e62b5f8a034436e4aaef9953f&newProject=try&newRevision=62d28e3e274c6a3ec28d15fef68c0c0380b60419&framework=1

Comment on attachment 9025726 [details] Bug 1346759 - Use URI comparison for null principals instead of pointer comparison. :ckerschb would you be able to provide any initial feedback on this patch?

Comment on attachment 9025726 [details] Bug 1346759 - Use URI comparison for null principals instead of pointer comparison. Yeah, that looks pretty good. Thanks for working on this, wanted to have that landed a long time ago!

Comment on attachment 9025726 [details] Bug 1346759 - Use URI comparison for null principals instead of pointer comparison. :heycam other than the feedback :ckerschb gave is there anything I need to think about further from an oxidation standpoint when landing this? I know the Rust is super simple but I haven't done ffi before or anything in Firefox.

Commented in Phabricator.

Hey :kmag, This code changes how we compare principals from pointer comparison to string comparison. In toolkit/mozapps/extensions/AddonManager.jsm we have the following code: > else if (!aBrowser.contentPrincipal || !aInstallingPrincipal.subsumes(aBrowser.contentPrincipal)) { My current assumption is that null principals are being serialized across the process boundary and not passing this subsumes( code where as in my test run it is returning true. To make the failing test (toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js) pass I changed to: > } else if (aInstallingPrincipal.isNullPrincipal || !aBrowser.contentPrincipal || !aInstallingPrincipal.subsumes(aBrowser.contentPrincipal)) { --- So I could instead add "Harness.installBlockedCallback = install_blocked;" to toolkit/mozapps/extensions/test/xpinstall/browser_datauri.js to make it pass. This is because it is still failing the following check: > if (!this.isInstallAllowed(aMimetype, aInstallingPrincipal)) { Are there other concerning changes you can think of in the addon code to making this change?

Pushed by jkingston@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/ef4325327e46 Use URI comparison for null principals instead of pointer comparison. r=ckerschb,bholley