1630092 - Verify wasm library sandboxing being included in the plain-opt mozconfig follows build guidelines

Status: RESOLVED FIXED

(Firefox Build System :: General, defect, P3)

Description

[Shravan Narayan]

This continues a discussion started here https://phabricator.services.mozilla.com/D70652

From dmajor

I have some concerns about having wasm-sandboxing stuff in the plain-opt mozconfig file at all. 
The plain configs are intended to be as close as possible to an empty/missing mozconfig file, to 
have CI assurance that we don't accidentally break people who build that way locally, especially
newcomers who are the most likely to have an empty mozconfig and the most likely to be scared
away by build errors.

The reason this file isn't _completely_ empty is to compensate for the nonstandard setup on our
automation builders -- they don't have a ~/.mozbuild so we can't find the mandatory toolchains
in the regular way. I don't think the wasm-sandboxing stuff is quite in the same category. I looked
up the review of this landing but didn't find any specific discussion about it.

Comment 1

[Shravan Narayan]

So the plan here was that wasm library sandboxing is the default build setting for firefox builds starting firefox 74 on linux and firefox 75 on mac. So I believe this is by design (although I am not a 100% sure). From the beginner's standpoint, mach bootstrap pulls in all toolchain dependencies so the default build with no config works as before. The thing that changes is the set of dependencies that need to be installed if you don't use mach bootstrap. Please correct me if I am wrong, but I'm guessing that does not count as a beginner scenario?

Comment 2

[(Away)]

If I do a build on Linux that is completely vanilla -- my tree has no file named mozconfig nor .mozconfig and I don't set a MOZCONFIG environment variable -- then what value of WASM_SANDBOXED_LIBRARIES does the build system see? Empty, right? In which case, why does the plain-opt mozconfig set WASM_SANDBOXED_LIBRARIES?

Comment 3

[Shravan Narayan]

So, I'm trying to distinguish between two scenarios here

1. Enabling wasm library sandboxing by default on builds
2. The mechanism (w.r.t how to set up configs) to enable wasm library sandboxing by default on builds

I think I am a bit confused whether we are talking about point 1 or point 2 above. I think my earlier response was more towards addressing point 1 rather than point 2. Could you please clarify?

Comment 4

[(Away)]

#1 and #2 sound very similar to me, so I don't want to answer the wrong thing and lead to further confusion.

How about I try to rephrase my concern in a different way: How do we prevent accidentally landing code that fails to build when MOZ_WASM_SANDBOXING_GRAPHITE is undefined?

Comment 5

[Shravan Narayan]

Ah, I see! So part of the answer here is we have made deliberate efforts to minimize the actual code changes that rely on the setting MOZ_WASM_SANDBOXING_GRAPHITE. Most of the code is setup to be intentionally agnostic to this value. Right now, there are three pieces of code that depend on this config that can change

1. LibrarySandboxPreload - To ensure platform specific sandboxing policies don't block use of library sandboxing. This is required only if MOZ_WASM_SANDBOXING_GRAPHITE is actually used.

2. Setting sandbox type - This basically looks at MOZ_WASM_SANDBOXING_GRAPHITE and sets the sandbox type (which could either be wasm if the setting is enabled or none if it is not) for the code that uses graphite.

3. Creating the sandbox - This looks at MOZ_WASM_SANDBOXING_GRAPHITE to correctly set parameters to instantiate the chosen sandbox type.

These three pieces are fairly standard and the hope is that this shouldn't be modified too often. However, new libraries begin wasm sandboxed would have to also add the same 3 pieces as above (i.e. similar to the code review linked in the original post).

The other part of the answer to what happens if someone incorrectly modifies these 3 pieces OR adds new code that depends on MOZ_WASM_SANDBOXING_GRAPHITE is that, right now, wasm based library sandboxing is being used only on desktop x64 linux and mac (windows coming soon). This effectively means try builds for linux and mac 64 bit have MOZ_WASM_SANDBOXING_GRAPHITE set while other platforms do not. Thus any code submission at this points effectively gets compiled for both configurations.

Not sure how satisfying this answer is, but for the moment this will ensure no one accidentally breaks either configuration. Maybe, longer term we may need to have an explicit build with MOZ_WASM_SANDBOXING_* set and one without MOZ_WASM_SANDBOXING_* set?

I'm also adding Eric, who can perhaps shed more light on this / add more folks who can.

Comment 6

[(Away)]

Maybe, longer term we may need to have an explicit build with MOZ_WASM_SANDBOXING_* set and one without MOZ_WASM_SANDBOXING_* set?

The plain builds are supposed to fulfill the latter, that's their reason for existing. That's why I raised this concern.

Comment 7

[Eric Rahm [:erahm]]

I'm going with the assumption that we added it to plain for a good reason. I think we can hold off on answering this question until Nathan is back.

Comment 8

[Shravan Narayan]

@Nathan - could you help with this?

Comment 9

[Nathan Froyd [:froydnj]]

I don't recall why I added it to plain; I think it might have just made life easier? But I think dmajor is correct; we should not be including this stuff in plain builds, because it (should) default to off, and that is what the plain build is testing. I will have a go at removing it from the plain builds.

Comment 10

[(Away)]

Attachment: Don't pull in wasm sandboxing in plain or base-toolchain builds

The plain builds are intended to make sure that building with an empty mozconfig doesn't break. Since an empty mozconfig build wouldn't have wasm sandboxing, neither should the plain builds.

While here, also remove wasi/lucet dependencies from the base-toolchain tasks, since they don't enable sandboxing either.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d83c7bb5e91c
Don't pull in wasm sandboxing in plain or base-toolchain builds r=froydnj

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/d83c7bb5e91c