Skip to content

Commit

Permalink
Assert if MODULARIZE factory function is used with new.
Browse files Browse the repository at this point in the history
Once we start using `async` for for this function then `new` stops
working: emscripten-core#23157.

Using `new` in this was was always an antipattern but there was nothing
stopping users from doing this.
  • Loading branch information
sbc100 committed Dec 18, 2024
1 parent dea1bf9 commit 02a1a96
Show file tree
Hide file tree
Showing 8 changed files with 81 additions and 58 deletions.
5 changes: 5 additions & 0 deletions ChangeLog.md
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,11 @@ See docs/process.md for more on how version tagging works.

3.1.75 (in development)
-----------------------
- When using `-sMODULARIZE` we now assert if the factory function is called with
the JS `new` keyword. e.g. `var a = new Module()` rather than `var b =
Module()`. This paves the way for marking the function as `async` which does
not allow `new` to be used. This usage of `new` here was never documented and
is considered and antipattern. (#)
- `PATH.basename()` no longer calls `PATH.normalize()`, so that
`PATH.basename("a/.")` returns `"."` instead of `"a"` and
`PATH.basename("a/b/..")` returns `".."` instead of `"a"`. This is in line with
Expand Down
8 changes: 4 additions & 4 deletions test/code_size/hello_webgl2_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 454,
"a.html.gz": 328,
"a.js": 4532,
"a.js.gz": 2315,
"a.js": 4527,
"a.js.gz": 2311,
"a.wasm": 10402,
"a.wasm.gz": 6703,
"total": 15388,
"total_gz": 9346
"total": 15383,
"total_gz": 9342
}
8 changes: 4 additions & 4 deletions test/code_size/hello_webgl2_wasm2js.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"a.html": 346,
"a.html.gz": 262,
"a.js": 22200,
"a.js.gz": 11583,
"total": 22546,
"total_gz": 11845
"a.js": 22195,
"a.js.gz": 11581,
"total": 22541,
"total_gz": 11843
}
8 changes: 4 additions & 4 deletions test/code_size/hello_webgl_wasm.json
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"a.html": 454,
"a.html.gz": 328,
"a.js": 4070,
"a.js.gz": 2158,
"a.js": 4065,
"a.js.gz": 2155,
"a.wasm": 10402,
"a.wasm.gz": 6703,
"total": 14926,
"total_gz": 9189
"total": 14921,
"total_gz": 9186
}
8 changes: 4 additions & 4 deletions test/code_size/hello_webgl_wasm2js.json
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
{
"a.html": 346,
"a.html.gz": 262,
"a.js": 21726,
"a.js.gz": 11413,
"total": 22072,
"total_gz": 11675
"a.js": 21721,
"a.js.gz": 11409,
"total": 22067,
"total_gz": 11671
}
35 changes: 13 additions & 22 deletions test/test_browser.py
Original file line number Diff line number Diff line change
Expand Up @@ -4795,28 +4795,19 @@ def test_browser_run_from_different_directory(self):

# Similar to `test_browser_run_from_different_directory`, but asynchronous because of `-sMODULARIZE`
def test_browser_run_from_different_directory_async(self):
for args, creations in [
(['-sMODULARIZE'], [
'Module();', # documented way for using modularize
'new Module();' # not documented as working, but we support it
]),
]:
print(args)
# compile the code with the modularize feature and the preload-file option enabled
self.compile_btest('browser_test_hello_world.c', ['-o', 'test.js', '-O3'] + args)
ensure_dir('subdir')
shutil.move('test.js', Path('subdir/test.js'))
shutil.move('test.wasm', Path('subdir/test.wasm'))
for creation in creations:
print(creation)
# Make sure JS is loaded from subdirectory
create_file('test-subdir.html', '''
<script src="subdir/test.js"></script>
<script>
%s
</script>
''' % creation)
self.run_browser('test-subdir.html', '/report_result?0')
# compile the code with the modularize feature and the preload-file option enabled
self.compile_btest('browser_test_hello_world.c', ['-o', 'test.js', '-O3', '-sMODULARIZE'])
ensure_dir('subdir')
shutil.move('test.js', Path('subdir/test.js'))
shutil.move('test.wasm', Path('subdir/test.wasm'))
# Make sure JS is loaded from subdirectory
create_file('test-subdir.html', '''
<script src="subdir/test.js"></script>
<script>
Module();
</script>
''')
self.run_browser('test-subdir.html', '/report_result?0')

# Similar to `test_browser_run_from_different_directory`, but
# also also we eval the initial code, so currentScript is not present. That prevents us
Expand Down
6 changes: 6 additions & 0 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -6774,6 +6774,12 @@ def test_modularize_strict(self):
output = self.run_js('run.js')
self.assertEqual(output, 'hello, world!\n')

def test_modularize_new_misuse(self):
self.run_process([EMCC, test_file('hello_world.c'), '-sMODULARIZE', '-sEXPORT_NAME=Foo'])
create_file('run.js', 'var m = require("./a.out.js"); new m();')
err = self.run_js('run.js', assert_returncode=NON_ZERO)
self.assertContained('Foo() must not be called with new\n', err)

@parameterized({
'': ([],),
'export_name': (['-sEXPORT_NAME=Foo'],),
Expand Down
61 changes: 41 additions & 20 deletions tools/link.py
Original file line number Diff line number Diff line change
Expand Up @@ -2385,6 +2385,16 @@ def modularize():
if settings.EXPORT_ES6 and settings.ENVIRONMENT_MAY_BE_NODE:
async_emit = 'async '

# TODO(sbc): Do we really need _scriptName in all cases. Look into reducing this
# condition.
add_script_name_wrapper = not settings.MINIMAL_RUNTIME or settings.SHARED_MEMORY
add_assertion_wrapper = settings.ASSERTIONS and not settings.MODULARIZE == 'instance'

if add_script_name_wrapper or add_assertion_wrapper:
wrapper_name = settings.EXPORT_NAME + '_inner'
else:
wrapper_name = settings.EXPORT_NAME

if settings.MODULARIZE == 'instance':
wrapper_function = '''
export default async function init(moduleArg = {}) {
Expand All @@ -2398,8 +2408,8 @@ def modularize():
'generated_js': generated_js
}
else:
wrapper_function = '''
%(maybe_async)sfunction(moduleArg = {}) {
wrapper_function = '''\
%(maybe_async)sfunction %(wrapper_name)s(moduleArg = {}) {
var moduleRtn;
%(generated_js)s
Expand All @@ -2408,26 +2418,18 @@ def modularize():
}
''' % {
'maybe_async': async_emit,
'generated_js': generated_js
'generated_js': generated_js,
'wrapper_name': wrapper_name
}

if settings.MINIMAL_RUNTIME and not settings.PTHREADS:
# Single threaded MINIMAL_RUNTIME programs do not need access to
# document.currentScript, so a simple export declaration is enough.
src = f'/** @nocollapse */ var {settings.EXPORT_NAME} = {wrapper_function};'
else:
if add_script_name_wrapper:
script_url_node = ''
# When MODULARIZE this JS may be executed later,
# after document.currentScript is gone, so we save it.
# In EXPORT_ES6 + PTHREADS the 'thread' is actually an ES6 module
# webworker running in strict mode, so doesn't have access to 'document'.
# In this case use 'import.meta' instead.
if settings.EXPORT_ES6 and settings.USE_ES6_IMPORT_META:
script_url = 'import.meta.url'
else:
script_url = "typeof document != 'undefined' ? document.currentScript?.src : undefined"
if settings.ENVIRONMENT_MAY_BE_NODE:
script_url_node = "if (typeof __filename != 'undefined') _scriptName = _scriptName || __filename;"
# When MODULARIZE this JS may be executed later, after
# document.currentScript is gone, so we save it. This is only
# needed if the program actually requires `_scriptName`
script_url = "typeof document != 'undefined' ? document.currentScript?.src : undefined"
if settings.ENVIRONMENT_MAY_BE_NODE:
script_url_node = "if (typeof __filename != 'undefined') _scriptName = _scriptName || __filename;"
if settings.MODULARIZE == 'instance':
src = '''\
var _scriptName = %(script_url)s;
Expand All @@ -2443,14 +2445,33 @@ def modularize():
var %(EXPORT_NAME)s = (() => {
var _scriptName = %(script_url)s;
%(script_url_node)s
return (%(wrapper_function)s);
return %(wrapper_function)s
})();
''' % {
'EXPORT_NAME': settings.EXPORT_NAME,
'script_url': script_url,
'script_url_node': script_url_node,
'wrapper_function': wrapper_function,
}
elif add_assertion_wrapper:
src = '''\
var %(EXPORT_NAME)s = (() => {
%(wrapper_function)s
// Return a small, never-async wrapper around %(wrapper_name)s which
// checks for callers incorrectly using it with `new`.
return function(arg) {
assert(new.target, "%(EXPORT_NAME)s() should not be called with `new %(EXPORT_NAME)s()`");
return %(wrapper_name)s(arg);
}
})();
''' % {
'EXPORT_NAME': settings.EXPORT_NAME,
'wrapper_function': wrapper_function,
'wrapper_name': wrapper_name,
}
else:
# No wrapper required. A simple export declaration is enough.
src = f'/** @nocollapse */ {wrapper_function};'

# Given the async nature of how the Module function and Module object
# come into existence in AudioWorkletGlobalScope, store the Module
Expand Down

0 comments on commit 02a1a96

Please sign in to comment.