Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

In MODULARIZE mode avoid modifying the incoming moduleArg. NFC #21775

Merged
merged 1 commit into from
Apr 17, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/closure-externs/closure-externs.js
Original file line number Diff line number Diff line change
Expand Up @@ -245,9 +245,8 @@ var moduleArg;
* Used in MODULARIZE mode.
* We need to access this after the code we pass to closure so from closure's
* POV this is "extern".
* @suppress {duplicate}
*/
var readyPromise;
var moduleRtn;

/**
* This was removed from upstream closure compiler in
Expand Down
4 changes: 4 additions & 0 deletions src/jsifier.mjs
Original file line number Diff line number Diff line change
Expand Up @@ -744,6 +744,10 @@ var proxiedFunctionTable = [
includeFile(fileName, shouldPreprocess(fileName));
}

if (MODULARIZE) {
includeFile('postamble_modularize.js');
}

print(
'//FORWARDED_DATA:' +
JSON.stringify({
Expand Down
41 changes: 41 additions & 0 deletions src/postamble_modularize.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
// In MODULARIZE mode we wrap the generated code in a factory function
// and return either the Module itself, or a promise of the module.
//
// We assign to the `moduleRtn` global here and configure closure to see
// this as and extern so it won't get minified.

#if WASM_ASYNC_COMPILATION

#if USE_READY_PROMISE
moduleRtn = readyPromise;
#else
moduleRtn = {};
#endif

#else // WASM_ASYNC_COMPILATION

moduleRtn = Module;

#endif // WASM_ASYNC_COMPILATION

#if ASSERTIONS
// Assertion for attempting to access module properties on the incoming
// moduleArg. In the past we used this object as the prototype of the module
// and assigned properties to it, but now we return a distinct object. This
// keeps the instance private until it is ready (i.e the promise has been
// resolved).
for (const prop of Object.keys(Module)) {
if (!(prop in moduleArg)) {
Object.defineProperty(moduleArg, prop, {
configurable: true,
get() {
#if WASM_ASYNC_COMPILATION
abort(`Access to module property ('${prop}') is no longer possible via the incoming module contructor argument; Instead, use the result of the module promise.`)
#else
abort(`Access to module property ('${prop}') is no longer possible via the module input argument; Instead, use the module constructor return value.`)
#endif
}
});
}
}
#endif
4 changes: 2 additions & 2 deletions src/shell.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
// The Module object: Our interface to the outside world. We import
// and export values on it. There are various ways Module can be used:
// 1. Not defined. We create it here
// 2. A function parameter, function(Module) { ..generated code.. }
// 2. A function parameter, function(moduleArg) => Promise<Module>
// 3. pre-run appended it, var Module = {}; ..generated code..
// 4. External script tag defines var Module.
// We need to check if Module already exists (e.g. case 3 above).
Expand All @@ -21,7 +21,7 @@
// before the code. Then that object will be used in the code, and you
// can continue to use Module afterwards as well.
#if MODULARIZE
var Module = moduleArg;
var Module = Object.assign({}, moduleArg);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think we could take this further and fully separate the arguments and return value at some point too (i.e. not put all the module arg inputs on the module return value)? It would be a bigger breaking change, but IIRC it doesn't seem like many people rely on that behavior when using modularize?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see what you mean, yes. I think it might be possible yes.

#elif USE_CLOSURE_COMPILER
// if (!Module)` is crucial for Closure Compiler here as it will otherwise replace every `Module` occurrence with a string
var /** @type {{
Expand Down
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size.js.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
55580
55579
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_no_asserts.js.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
31465
31464
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_strict.js.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
54468
54467
25 changes: 19 additions & 6 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -6416,11 +6416,11 @@ def test_modularize_sync_compilation(self):
console.log('before');
var result = Module();
// It should be an object.
console.log(typeof result);
console.log('typeof result: ' + typeof result);
// And it should have the exports that Module has, showing it is Module in fact.
console.log(typeof result._main);
console.log('typeof _main: ' + typeof result._main);
// And it should not be a Promise.
console.log(typeof result.then);
console.log('typeof result.then: ' + typeof result.then);
console.log('after');
''')
self.run_process([EMCC, test_file('hello_world.c'),
Expand All @@ -6430,12 +6430,25 @@ def test_modularize_sync_compilation(self):
self.assertContained('''\
before
hello, world!
object
function
undefined
typeof result: object
typeof _main: function
typeof result.then: undefined
after
''', self.run_js('a.out.js'))

def test_modularize_argument_misuse(self):
create_file('test.c', '''
#include <emscripten.h>
EMSCRIPTEN_KEEPALIVE int foo() { return 42; }''')

create_file('post.js', r'''
var arg = { bar: 1 };
var promise = Module(arg);
arg._foo();''')

expected = "Aborted(Access to module property ('_foo') is no longer possible via the incoming module contructor argument; Instead, use the result of the module promise"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

contructor => constructor

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I read this comment a few times and I'm not actually sure what the user is expected to do to fix it? This is post-js code so it is inside the emitted JS, how can the module Promise be accessed?

Also, this seems like a breaking change that should be mentioned in the changelog?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is --extern-post-js code.. where we run the Module() constructor, but we ignore the return value and instead try to use a method on the argument we passed into the constructor.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, thanks, I missed the -extern-.

self.do_runf('test.c', expected, assert_returncode=NON_ZERO, emcc_args=['--no-entry', '-sMODULARIZE', '--extern-post-js=post.js'])

def test_export_all_3142(self):
create_file('src.cpp', r'''
typedef unsigned int Bit32u;
Expand Down
14 changes: 2 additions & 12 deletions tools/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -2350,31 +2350,21 @@ def modularize(options):
shared.target_environment_may_be('web'):
async_emit = 'async '

# Return the incoming `moduleArg`. This is is equivalent to the `Module` var within the
# generated code but its not run through closure minification so we can reference it in
# the return statement.
return_value = 'moduleArg'
if settings.WASM_ASYNC_COMPILATION:
if settings.USE_READY_PROMISE:
return_value = 'readyPromise'
else:
return_value = '{}'

# TODO: Remove when https://bugs.webkit.org/show_bug.cgi?id=223533 is resolved.
if async_emit != '' and settings.EXPORT_NAME == 'config':
diagnostics.warning('emcc', 'EXPORT_NAME should not be named "config" when targeting Safari')

src = '''
%(maybe_async)sfunction(moduleArg = {}) {
var moduleRtn;

%(src)s

return %(return_value)s
return moduleRtn;
}
''' % {
'maybe_async': async_emit,
'src': src,
'return_value': return_value,
}

if settings.MINIMAL_RUNTIME and not settings.PTHREADS:
Expand Down
Loading