At a minimum, we need to add some support to this giant #if: https://searchfox.org/mozilla-central/source/security/sandbox/chromium/build/build_config.h#106-179 I'm sure there are other fun things to start fixing after that.

This patch includes the changes that Microsoft landed for the sandbox along with other changes to the supporting base files that they depend upon.

Look like I need to hone my try fuzzy skills: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1055b7c175edf4db88d1c7d13ed0670b49f33c5c

Comment on attachment 9030265 [details] [diff] [review] part 2: Enable aarch64 Windows chromium sandbox code Review of attachment 9030265 [details] [diff] [review]: ----------------------------------------------------------------- I think you actually want to remove that block in old-configure.in...if you actually need the #define below, we should talk. ::: old-configure.in @@ +2961,5 @@ > fi > > case "$OS_TARGET:$CPU_ARCH" in > WINNT:aarch64) > + ARCH_CPU_64_BITS=1 I don't understand why this is necessary, given that the relevant #define is set in your first patch set. Could you explain?

(In reply to Nathan Froyd [:froydnj] from comment #4) ... > ::: old-configure.in > @@ +2961,5 @@ > > fi > > > > case "$OS_TARGET:$CPU_ARCH" in > > WINNT:aarch64) > > + ARCH_CPU_64_BITS=1 > > I don't understand why this is necessary, given that the relevant #define is > set in your first patch set. Could you explain? Perhaps I can get rid of that, I'll try. I added it when I was tracking down that MemoryBarrier problem, because it was showing up in the small amount of code that is used in xul and when I added it, it did change the number of errors, so I thought it was having some effect. However, since that I realised that I was missing a change to pull in a different atomicops header and the reason I added that define was in the other file anyway.

Comment on attachment 9030273 [details] [diff] [review] part 2: Enable aarch64 Windows chromium sandbox code r=froydnj from comment 4. aarch64 try push with the #define removed: https://treeherder.mozilla.org/#/jobs?repo=try&revision=aef23c275a90a2f998572bd50c555cfdb1935615

Comment on attachment 9030263 [details] [diff] [review] part 1: Add aarch64 Windows support to the chromium sandbox code Review of attachment 9030263 [details] [diff] [review]: ----------------------------------------------------------------- To satisfy my paranoia, I looked up what I could here and everything seems good. Some of it, like the parameters to ReadProcessMemory, are not yet documented aarch64 Windows internals (I guess). ::: security/sandbox/chromium-shim/patches/with_update/add_aarch64_windows_support.patch @@ +16,5 @@ > + Atomic64 NoBarrier_Load(volatile const Atomic64* ptr); > + Atomic64 Acquire_Load(volatile const Atomic64* ptr); > + Atomic64 Release_Load(volatile const Atomic64* ptr); > + #endif // ARCH_CPU_64_BITS > + It looks like there are a few line ending issues with this file but not in the files it applies to. So maybe it applies ok with these lines. But, a warning in case this causes lint issues in the future.

(In reply to David Parks (dparks) [:handyman] from comment #8) > Comment on attachment 9030263 [details] [diff] [review] > part 1: Add aarch64 Windows support to the chromium sandbox code > > Review of attachment 9030263 [details] [diff] [review]: > ----------------------------------------------------------------- > > To satisfy my paranoia, I looked up what I could here and everything seems > good. Some of it, like the parameters to ReadProcessMemory, are not yet > documented aarch64 Windows internals (I guess). That's great, thanks for doing that. > > + #endif // ARCH_CPU_64_BITS > > + > > It looks like there are a few line ending issues with this file but not in > the files it applies to. So maybe it applies ok with these lines. But, a > warning in case this causes lint issues in the future. Yeah, that's because the control character for an unchanged line is a space, so when you have an empty unchanged line it looks like a trailing whitespace issue.

Pushed by bobowencode@gmail.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4c0f7afe6712 part 1: Add aarch64 Windows support to the chromium sandbox code. r=handyman https://hg.mozilla.org/integration/mozilla-inbound/rev/9f01fb6fbbfd part 2: Enable aarch64 Windows chromium sandbox code. r=froydnj

https://hg.mozilla.org/mozilla-central/rev/4c0f7afe6712 https://hg.mozilla.org/mozilla-central/rev/9f01fb6fbbfd

This made the aarch64 builds unusable: nothing loads in any tab. I had to disable the sandbox to get a working browser, although I only tried level 0, not any value between 0 and 5.

(In reply to Mike Hommey [:glandium] from comment #12) > This made the aarch64 builds unusable: nothing loads in any tab. I had to > disable the sandbox to get a working browser, although I only tried level 0, > not any value between 0 and 5. glandium, I could reproduce this over the weekend so I landed a patch in bug 1514583 to disable it (it's just hit m-c). However, I've just tried to reproduce again to start to diagnose the issue and frustratingly it is now working. Would you re-test your older aarch64 m-c build with the sandbox enabled please.

My old aarch64 m-c build was d86d184dc7d6 and with sandbox enabled, it doesn't work. I also tried a7eb50b9dc42, which is presently the last aarch64 build downloadable on m-c, and with sandbox enabled manually, it doesn't work either. One thing that I noticed, though it hasn't happened recently, is that sometimes Firefox doesn't actually exit when closing windows, and opening what one might think is a new Firefox, actually brings up the one that was still running. Maybe you've hit that? (I now double check every time I close Firefox that there's no process left before I start another one)

(In reply to Mike Hommey [:glandium] from comment #14) ... > One thing that I noticed, though it hasn't happened recently, is that > sometimes Firefox doesn't actually exit when closing windows, and opening > what one might think is a new Firefox, actually brings up the one that was > still running. Maybe you've hit that? (I now double check every time I close > Firefox that there's no process left before I start another one) Thanks, yeah, I'd seen that too. I'd copied firefox into |Program Files (Arm)|, which appears to make it work for some reason. If I try and run it from, for example, Downloads then I get the problem again, permissions I imagine, so it looks like the sandbox rules might not be working after all. :-( Now I just need to work out what's happening.

I can confirm it works if I copy the firefox directory into c:\Program Files (Arm)!

The problem here appears to be that it does some part of the DLL loading on a new thread. The chromium sandbox sets an impersonation token on the main thread of the child process with more rights, so that it can do general start-up things like load DLLs. This is reverted early in the process when we call TargetServicesBase::LowerToken. I can see the child process spinning up a new thread and calling NtQueryAttributesFile on mozglue.dll, which fails because that thread has the more restricted token. The brokering won't work either because we haven't called TargetServicesBase::Init and it's possibly too early in the process start-up anyway. I'll file a new bug and try and work out what we can do about this.