Skip to content

Fix webidl_binder so it can't handle missing assert function #21171

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix webidl_binder so it can't handle missing assert function #21171

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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: 2 additions & 1 deletion test/test_core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7637,6 +7637,7 @@ def test_embind_no_rtti_followed_by_rtti(self):
def test_webidl(self, mode, allow_memory_growth):
self.uses_es6 = True
self.set_setting('WASM_ASYNC_COMPILATION', 0)
self.set_setting('STRICT')
if self.maybe_closure():
# avoid closure minified names competing with our test code in the global name space
self.set_setting('MODULARIZE')
Expand Down Expand Up @@ -7670,7 +7671,7 @@ def test_webidl(self, mode, allow_memory_growth):

# Export things on "TheModule". This matches the typical use pattern of the bound library
# being used as Box2D.* or Ammo.*, and we cannot rely on "Module" being always present (closure may remove it).
self.emcc_args += ['--post-js=glue.js', '--extern-post-js=extern-post.js']
self.emcc_args += ['--no-entry', '--post-js=glue.js', '--extern-post-js=extern-post.js']
if mode == 'ALL':
self.emcc_args += ['-sASSERTIONS']
if allow_memory_growth:
Expand Down
2 changes: 1 addition & 1 deletion test/webidl/post.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,7 +239,7 @@ try {
} catch (e) {}

try {
s = new TheModule.StringUser('abc', 1);
var s = new TheModule.StringUser('abc', 1);
s.Print(123, null); // Expects a string or a wrapped pointer
} catch (e) {}

Expand Down
9 changes: 9 additions & 0 deletions tools/webidl_binder.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,15 @@ def build_constructor(name):
mid_js += build_constructor('WrapperObject')

mid_js += ['''
/*
* For now, the webidl-generated code unconditionally depends on the `assert` function,
* but there are certain build modes where emscripten does not define this.
* TODO(sbc): Make the usage of assert conditional.
*/
if (typeof assert == "undefined") {
assert = (cond) => {}
}
Comment on lines +141 to +143
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (typeof assert == "undefined") {
assert = (cond) => {}
}
if (typeof assert === "undefined") {
var assert = () => {};
}

Copy link
Member

Choose a reason for hiding this comment

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

This may not work, if the generated code here is in a different scope than where assert is defined,

function assert() { .. }
function other() {
  if (typeof assert === "undefined") {
   // assert is always overridden here
    var assert = () => {};
  }
}

I think the WebIDL binder output here is usually added in a pre-js, so maybe that would be fine. However, it seems a little risky. Might be simplest to just define a new wassert and use that in the WebIDL binder code, perhaps?

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 think the above example would work just fine wouldn't it typeof assert === "undefined" will return false if there is an assert in the output/global scope.

webidl code certainly must be the same scope as the emscripten module code and --post-js is the canonical way to use it.

In the long term I hope to add --debug flag to webidl_binder.py but for now I think this solution should work fine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because JS does function hoisting the check will always work regardless of whether this code comes after of before the assert declartion.

For example the following code always prints "good" regardless of whether it is run at the global scope or in a function:

  if (typeof assert == 'undefined') {
    console.error('oops');
  } else {
    console.error('good');
  }

  function assert() {
  }

Copy link
Member

@kripken kripken Jan 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I think the above example would work just fine wouldn't it typeof assert === "undefined" will return false if there is an assert in the output/global scope.

No, there is a problem with scoping. Here is an extension of my example that is runnable:

function assert() {
  throw "true assert";
}

function other() {
  if (typeof assert === "undefined") {
    // assert is always overridden here
    var assert = () => {};
  }
  assert(1 == 0);
}

other();

Running that does not throw true assert, showing the assert was always overridden.

var is scoped to the function, so entering other means that the assert in the outer scope does not matter. Just the presence of the var means that assert is equal to undefined in that scope (as it is undefined until it actually reaches the var).


/** @suppress {duplicate} (TODO: avoid emitting this multiple times, it is redundant)
@param {*=} __class__ */
function getCache(__class__) {
Expand Down