Support node env override in Electron renderer process

[kwonoj]

It might not be legit approach to solve issues and possibly I am lack of way to correctly setting it - feel freely close PR if so.

This PR try to resolve usecases in Electron (https://electron.atom.io/) 's renderer process. Electron's renderer process is basically web browser instance but have node.js integration, so it is possible global context to have window.fetch and node.js both. In those process, setting up ENVIRONMENT to NODE is not sufficient to load wasm binary without fetch, as current detection logic falls back to use fetch as soon as it is available.

Instead, adds one additional condition to check - if given process is Electron (where window have node.js require and fetch both) but explicitly overridden to use NODE, skips to fetch. Otherwise it'll work as same as browser context.

[nazar-pc]

I'm wondering if this issue is only happening to wasm file and not to mem file or other potential paths. Did you investigate it further?
Because in #5368 I've found that there are a few file types that use similar code paths and might be affected by the same issue.
Otherwise seems reasonable to me.

[kwonoj]

@nazar-pc Current stable electron supports native wasm, so I didn't verify asm.js or other paths at the moment I wrote this PR.

[nazar-pc]

Great! I think it should be fine then, wasm is the main focus for future development anyway.

[kwonoj]

I just noticed some running context on Electron might need falls back to asm.js (like <webView> or browserView and hosted page have CSP enabled, blocking WebAssembly.compile.). But think that can be dealt separately, mostly I haven't tested asm.js correctly with those environment yet.

[nazar-pc]

Well, if that is a real issue, I'd deal with it in the same PR just because it is essentially the same issue.
You can just grep across Emscripten for all fetch() usages and check whether it is related.

Also I'm wondering why WebAssembly.compile is banned with CSP, sounds like a bug in Electron to me.

[kwonoj]

WebAssembly.compile is banned with CSP, sounds like a bug in Electron to me

: it is not a bug (of Electron), there are discussions around CSP interaction in design WebAssembly/design#1092 , at this moment it simply falls back to not to allow compile wasm binary based on CSP . (i.e : https://bugs.chromium.org/p/chromium/issues/detail?id=686369). I haven't verified by myself, but just using <webView> on plain chromimum with loading page have CSP would have same result.

[kwonoj]

I'd deal with it in the same PR just because it is essentially the same issue

Actually I'm more hoping #5296 could land instead of this PR, provides options does't require to deal with special running context checking.

[kripken]

Instead of detecting electron, could we see if the fetch fails, and if it does, try the other approach (that is taken if fetch is not present)? That seems like it would make things more robust in general, and also fix this, unless I have misunderstood.

[kwonoj]

@kripken I would say detection is more general use case for Electron's process. In Electron's renderer process, you can have both context (node.js / fetch) and you may want to opt-in one approach instead of falling it back from fetch. Let's assume user writes code doesn't supply endpoiint for fetch by assuming to use node.js FS always in renderer, fall back starting from fetch will always attempt fetch first, fail it, then falling back to other approaches. In this PR's detection user can opt into fetch (by not specifying ENVIRONMENT).

[nazar-pc]

I would say detection is more general use case for Electron's process

This is a use case for Electron only. If/when other environment like this exists, the issue will need to be fixed again, introducing even more workarounds.

fall back starting from fetch will always attempt fetch first, fail it, then falling back to other approaches

I don't see any problem here.

Look like no one expected fetch() to be present, but failing, when corresponding code was written initially, but turns out handling its failure is a much better option in long term and is platform-independent.

[kwonoj]

This is a use case for Electron only. If/when other environment like this exists, the issue will need to be fixed again, introducing even more workarounds

While I said this is case for Electron, this is for any environment for where node.js has global fetch. Do you see edge cases of current logic could possibly fail on those environment? Current detection is generic enough that user can opt-in by specifying environment, otherwise it'll work idetical as browser environment.

I don't see any problem here.

Following your comment, this means any further user environment have global fetch but would like to use node.js fallback should fire redundant request failure regardless of user wants or not. I don't mind how to opt-in this behavior, but I believe user should able to opt in and don't think that's good ergonomics for certain environment.

[kwonoj]

In short, if this PR's not being accepted I agree with those decisions, but I'd argue fetch first fallback is not a way to go.

[nazar-pc]

I thought about this issue a bit more and end up with #5596.
Should increase final build size by just (a||b)&&, which is 8 bytes in total.

[dschuff]

I just uploaded #5606 which may have some bearing on this too. I don't really understand from this thread what it is that prevents fetch from being used in electron, that wouldn't just affect the web in the same way? In any case, I suppose if it has fetch, Electron may end up with WebAssembly.compileStreaming too.

edit: I guess the real reason is that you are using a local FS through node.js, and while fetch may work, you'd just rather use the local FS instead?

[kwonoj]

@dschuff crux of this issue is Electron's renderer process have no way to avoid to use fetch, while they have node.js environment can access files directly. Seems #5596 is merged and this PR itself is no longer needed.

[dschuff]

right; i guess my question is, do we need to do the same for #5606?

[kwonoj]

I think so. If any branch's just execute based on existense of feature, it'll fall back same cases.

[dschuff]

Electron seems like an odd environment because it's not really purely ENVIRONMENT_IS_WEB or ENVIRONMENT_IS_NODE, right? Or really, it's both. You could conceivably want to pick and choose APIs from both. I'm wondering if it makes more sense in that case to just write your own instantiation code and register it with the existing Module['wasm-instantiate'] mechanism?

[nazar-pc]

Looks like #5606 will be the second place using fetch() in integrateWasmJS.
Maybe we should add local fetch variable and define it with global fetch() or left with undefined depending on conditions specified in #5596.

Or really, it's both

It is Node.js and Browser at the same time. Which means you can load WebAssembly from the local filesystem and using Web APIs like fetch().

However, when you're loading a library from local filesystem and then trying to load WebAssembly for its consumption using fetch() it falls apart because fetch() is a different context. You should either load both library and WebAssembly from local filesystem or both using Web APIs. At least in most cases.

[kwonoj]

Or really, it's both.

Yes, that's reason #5606 accepts checker, cause ENVIRONMENT_IS_* is overridable via Module['ENVIRONMENT'] variable by injecting module object when preamble init. I wouldn't consider it to more make sense to wire own init logic instead of choosing environment to run.

[nazar-pc]

Actually there are a few more places where fetch() is used without additional checks, so I'm wondering whether we need even more generic approach.

[dschuff]

We could have fetch be something overridable on the module too. (Either a function, or some value that indicates that fetch is available and desired). Are there other cases besides loading files where Electron has capabilities both of web and node? Should they be handled all with one big switch ("I always prefer node capabilities") or independently ("I want node FS instead of fetch, but everything else to be web")?

[nazar-pc]

Potentially there might be other cases, but I'd not overengineer it too early.

Also I want node FS instead of fetch, but everything else to be web doesn't make much sense to me. At this point you might consider forking Emscripten.

[dschuff]

So maybe some module-level fetch function or variable. Also I just discovered that Module['read-async'] exists which is an async XHR on the web. @kripken does that just predate fetch? Should it be combined somehow with whatever we do for fetch?

[kripken]

Yeah, I think Module.readAsync predates fetch. But given the issues here, maybe we should just avoid fetch as much as possible - we can use readAsync in all places, I think - does fetch just improve syntax, or also add some new capabilities that we use?

If it adds new capabilities, we could detect whether we can use it and use it in readAsync, so it's all in one place.

[nazar-pc]

@kripken, that is a very good point! fetch() does have some new capabilities comparing to good old XMLHttpRequest, but Emscripten doesn't use any of that, so it is effectively a code duplication. If we support older browsers without fetch() we should only use XMLHttpRequest, otherwise we should only use fetch(), there is no reason to support both at the same time.

But I'd like to clarify that XMLHttpRequest under Electron will have the same odd behavior as fetch().

So my suggestion would be to switch over to using Module.readAsync in every place where fetch() is currently used (and probably the same for XMLHttpRequest, need to take a closer look) and make Module.readAsync aware that under such environments like Electron it shouldn't use XMLHttpRequest, but rather local filesystem instead.

I can work on such PR if it sounds appropriate.

[dschuff]

I think moving all the logic into Module.readAsync and using it everywhere seems fine. Maybe we should also make it return a promise now, instead of just invoking a callback. We'd get the new error handling behavior, and most APIs are moving in that direction anyway.

[kripken]

Agreed, let's move code into readAsync, and make that a promise. (I don't think we need to worry about changing an internal API like that.)