Unhandled rejection

[bas-d]

Abort throws an error that isn't catched, resulting in an unhandled Promise rejection. This should fix #6255.

[kripken]

Thanks @bas-d!

Doesn't that catch stop the error from being thrown? So no error is reported?

We need to figure out that test failure tests/runner.py other.test_only_force_stdlibs - could be it is that it expects an error there.

[bas-d]

Looks like it does. I've been thinking, libsodium uses onAbort being called to fall back to asm.js, but perhaps that just isn't right, because he documentation for onAbort says:

After calling this function, program termination occurs (i.e., you can’t use this to try to do something else instead of stopping; there is no possibility of recovering here).

The problem is that if wasm can't load because it is not allowed (as is the case with loading it into a chrome extension with a CSP without unsafe-eval), abort is being called automatically. So perhaps a better way than just catching the error would be to add an option to Module, e.g. Module[recover], that allows to recover before aborting. So add something like to postamble.js in function abort(what):

  if (Module['recover']) {
    Module['recover'](what);
    return
  }
  if (Module['onAbort']) {
    Module['onAbort'](what);
  }

If the recover function is not assigned, it would exit normally, otherwise it allows to fall back to asm.js or do something else.
What do you think?

[kripken]

Hmm, maybe the best solution is to just test for WebAssembly directly in your code? Something like, fall back to asm.js if typeof WebAssembly === 'undefined'?

[bas-d]

Ok it turned out that the fallback in essence worked, but things went wrong because uglifyjs was used with the drop_console=true function. Look like the way emscripten allows to rebinds the Module.print and Module.printErr functions causes drop_console to mess things up. I was able to fix it by explicitly defining Module.print and Module.printErr in libsodium, but perhaps this is something that should be fixed in emscripten as well, or at least warned for in the documentation?

That leaves the uncaught rejection of abort in the console, which is still kind of ugly. I was able to get rid of it by overriding Module.instantiateWasm. But that requires to copy a lot of code from emscripten with the only difference that the error is catched. An easy fix for this would be to make same adjustments to this part of preamble.js:

emscripten/src/preamble.js

Lines 1044 to 1082 in d449897

	function receiveInstantiatedSource(output) {
	// 'output' is a WebAssemblyInstantiatedSource object which has both the module and instance.
	// receiveInstance() will swap in the exports (to Module.asm) so they can be called
	#if ASSERTIONS
	assert(Module === trueModule, 'the Module object should not be replaced during async compilation - perhaps the order of HTML elements is wrong?');
	trueModule = null;
	#endif
	#if USE_PTHREADS
	receiveInstance(output['instance'], output['module']);
	#else
	// TODO: Due to Closure regression https://github.com/google/closure-compiler/issues/3193, the above line no longer optimizes out down to the following line.
	// When the regression is fixed, can restore the above USE_PTHREADS-enabled path.
	receiveInstance(output['instance']);
	#endif
	}
	function instantiateArrayBuffer(receiver) {
	getBinaryPromise().then(function(binary) {
	return WebAssembly.instantiate(binary, info);
	}).then(receiver, function(reason) {
	err('failed to asynchronously prepare wasm: ' + reason);
	abort(reason);
	});
	}
	// Prefer streaming instantiation if available.
	if (!Module['wasmBinary'] &&
	typeof WebAssembly.instantiateStreaming === 'function' &&
	!isDataURI(wasmBinaryFile) &&
	typeof fetch === 'function') {
	WebAssembly.instantiateStreaming(fetch(wasmBinaryFile, { credentials: 'same-origin' }), info)
	.then(receiveInstantiatedSource, function(reason) {
	// We expect the most common failure cause to be a bad MIME type for the binary,
	// in which case falling back to ArrayBuffer instantiation should work.
	err('wasm streaming compile failed: ' + reason);
	err('falling back to ArrayBuffer instantiation');
	instantiateArrayBuffer(receiveInstantiatedSource);
	});
	} else {
	instantiateArrayBuffer(receiveInstantiatedSource);
	}

If we could refactor this into a function (e.g. instantiateAsync()), where all promises are properly returned, it would allow a user to catch the abort if necessary by overriding instantiateWasm with something like

Module.instantiateWasm = function(imports, successCallback) {
  instantiateAsync().catch(function(err) {
    // intitiate fallback
});
}

Preamble.js simply calls the function without catching the error. The advantage is that this wouldn't make any functional changes in the code, so should be able to pass all tests then :).

If this sounds OK I can adjust this PR into the refactor or make a new one if you don't want the previous commits in it.

[kripken]

drop_console=true

I don't think we run uglify compress - we just use it to minify whitespace. Maybe I misunderstood what you meant?

Refactoring the code to be properly async sounds good in general. And it's fine to use this PR for that (we'll squash the commits anyhow). However, we'd need to be sure code size does not regress in that refactoring.

[bas-d]

No I meant that if others apply drop_console on code that is generated by emscripten. That's what happend with libsodium anyway. And I think that even if we would disable it in libsodium and someone who uses libsodium in their application would enable it, it would result in the same error.

[kripken]

Thanks, looks good with minor comments.

I didn't follow the issue with drop_console - what happens if uglify is run with that option on emscripten output? Does it just remove console.logs entirely? I don't see how that would end up with anything broken though..

[bas-d]

I'm not sure what happens exactly either, I think has to do something with this: mishoo/UglifyJS#1399, mishoo/UglifyJS#2557.

For libsodium, either disabling drop_console or overriding Module.print and Module.printErr fixed some errors of functions not being defined, that's all I know..
But I'll dig into it a little deeper, and open a new PR / issue if I have some more substantial evidence / a fix!

[sbc100]

Looks nicer. Is there a reason not to wrap the instantiate[A]sync dunctions in #ifdef? I guess the JS optimizer will eliminate them anyway?

[kripken]

@sbc100 good point, yeah, we should ifdef those on WASM_ASYNC_COMPILATION. (although closure would be able to remove them, and possibly the js optimizer too)

[bas-d]

On WASM_ASYNC_COMPILATION or BINARYEN_ASYNC_COMPILATION (which is where function calls and original definitions were checked on)?

[sbc100]

Ah, yes, BINARYEN_ASYNC_COMPILATION was renamed to WASM_ASYNC_COMPILATION yesterday. Please use the new name only: WASM_ASYNC_COMPILATION.

[kripken]

Thanks @bas-d! Code looks good.

I think those test failures are because of an unrelated problem on earlier incoming. It's been fixed, so if you merge in latest incoming we should see them pass and can merge this.

[jennatuckerdeveloper]

Is this merge still hung up on one remaining mystery test failure?

[kripken]

I think that's a test that randomly fails sometimes, so not a blocker. Merging, thanks!