Always fixup custom __proxy attributes in library_html5_webgl.js

[sbc100]

Fixes: #20134

[sbc100]

OK, I came up with a better solution.

Part 1: Make jsifier stricter so it always checks for invalid __proxy attributes, even if its not going to use them. This mean that the error is reproducible by any test that uses -sINCLUDE_FULL_LIBRARY
Part 2: Fix the failures by always processing the custom __proxy attributes locally.

[kripken]

lgtm (i see auto-merge is on, so not clicking approve due to comment)

[juj]

This is awkward, no? Wouldn't all other libraries besides library_htm5_webgl.js want to run this functionality as well to ignore proxy attributes?

Ideally for Wasm Workers, the __proxy mechanism should be ignored if function is called from main thread, and in assertions builds, if a proxied JS function is called from a Wasm Worker, the code would raise an error, and in release builds simply truck through?

That is what this code

emscripten/src/jsifier.js

Lines 247 to 255 in 3a55fe1

	} else if (WASM_WORKERS && ASSERTIONS) {
	// In ASSERTIONS builds add runtime checks that proxied functions are not attempted to be called in Wasm Workers
	// (since there is no automatic proxying architecture available)
	snippet = modifyJSFunction(snippet, (args, body) => `
	function(${args}) {
	assert(!ENVIRONMENT_IS_WASM_WORKER, "Attempted to call proxied function '${mangled}' in a Wasm Worker, but in Wasm Worker enabled builds, proxied function architecture is not available!");
	${body}
	}\n`);
	}

attempts to do, so the above change should be redundant if the linked code works as intended?

[sbc100]

This code only runs if !SHARED_MEMORY so when building with wasm workers or pthreads this code doesn't run and the attributes are not ignored.

I agree this could probably use a comment. The issue is that library_htm5_webgl.js uses custom __proxy addribute sync_on_webgl_context_handle_thread which needs to get either re-written (above), or removed (here) before the library is processed, otherwise we get an Invalid proxyingMode jsifier.

[sbc100]

Oh, I did already include a comment as to why this done. Does it make sense now, given my explanation?

My initial version of this attempted to relax the __proxy checking but @kripken suggested we always remove / rewrite the custom values instead: #20135 (comment)

[juj]

This code only runs if !SHARED_MEMORY

I am confused. Taking a closer look, the code runs if

if (SHARED_MEMORY) {
  if (PTHREADS) {
    // ...
  } else if (WASM_WORKERS && ASSERTIONS) {
    // emit "assert(!ENVIRONMENT_IS_WASM_WORKER"
  }
}

but I think I wrote that when I still had the assumption that PTHREADS and WASM_WORKERS would always be mutually exclusive and you'd only build targeting one or the other - but turns out targeting both at the same time made sense.

So that above code should instead be

if (SHARED_MEMORY) {
  if (PTHREADS) {
    // ...
  }
  if (WASM_WORKERS && ASSERTIONS) {
    // emit "assert(!ENVIRONMENT_IS_WASM_WORKER"
  }
}

so that in PTHREADS + WASM_WORKERS builds the assertion is also added. Would that appropriately resolve the issue?

I still think no library_x.js file should need to mutate the __proxy fields depending on the build mode.

[sbc100]

I think I see what you are saying. We should probably not erase the __proxy attributes in the wasm workers build. I will create a followup to do that.

I agree the custom proxy stuff in library_html5_webgl.js is unusual but it looks like you were the one that created this mechanism back in #11356, and I think its OK as long as it get locally handled/erased.

[sbc100]

Sorry, I mis-remembered, I already did do that in this PR, that __proxy handling is enabled here for all SHARED_MEMORY builds.

I think updating jsifier.js in way you suggest sounds fine to me. That seems like a pre-existing issue that is somewhat unrelated to this PR and would effect all libraries, right?

Are you happy with this PR in the way it deals with library_html5_webgl.js?

[juj]

That seems like a pre-existing issue that is somewhat unrelated to this PR and would effect all libraries, right?

Yeah, I think it would be very preferable to have that affect all libraries, so that any developer who is calling a __proxy: sync function inside a Wasm Worker would get an assertion firing on them. That is because I don't think any function with that label would work properly when called in the context of a Wasm Worker.