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

Fix early crash on abort if the argument is circular #8657

Merged
merged 2 commits into from
Jun 11, 2019

Conversation

i404788
Copy link
Contributor

@i404788 i404788 commented May 23, 2019

Previously passing an circular object to abort would cause an early crash without showing any relevant information.

This patch uses a modified version of the MDN Circular Replacer Example

Example:

TypeError: Converting circular structure to JSON
    at JSON.stringify (<anonymous>)
    at process.abort (zstd-codec/js/lib/zstd-codec-binding-wasm.js:8:1237371)
    at process.emit (events.js:198:15)
    at processPromiseRejections (internal/process/promises.js:139:20)
    at processTicksAndRejections (internal/process/task_queues.js:87:32)

src/postamble.js Outdated
}
return value;
};
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

maybe declare this inside of abort since its only used in there?

What kind of situation would trigger this exactly? can we create a test for it somehow?

Copy link
Contributor Author

@i404788 i404788 May 23, 2019

Choose a reason for hiding this comment

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

Putting it inside abort would be good.

It's triggered by process["on"]("unhandledRejection",abort) in nodejs (browser seems to work fine as it doesn't have the event). You can probably test it by calling process.emit("unhandledRejection", [CircularObject]) manually, after which abort will throw an exception as shown in the original example.

As for writing a test, maybe we can call abort([CircularObject]) catch the exception in abort and make sure that it's either caused by https://github.com/emscripten-core/emscripten/blob/incoming/src/postamble.js#L422 or by https://github.com/emscripten-core/emscripten/blob/incoming/src/postamble.js#L431

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest maybe we change that line to process["on"]("unhandledRejection",onUnhandledRejection) which can do whatever it wants and then call abort with a string value.

Also we can/should probably just simplify abort to call ".toString()" on its argument. I'm not sure it makes sense for abort to accept complex objects that need JSON formatting.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also we can/should probably just simplify abort to call ".toString()" on its argument.

That might also make the cause less obvious, often when calling toString() javascript will return [object Object].

If I remember correctly process["on"]("unhandledRejection",onUnhandledRejection) will stop nodejs from showing that there was an unhandledRejection, which means that the only feedback you get (by default) would be from the module.

Copy link
Collaborator

Choose a reason for hiding this comment

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

People shouldn't be calling abort with arbitrary JS objects. I think its reasonable to expect it to take a string or an integer. Anyway, I guess I'm ok with this change as is now.

Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure, but I think we caught unhandledRejection because otherwise node didn't show a proper error during async wasm compilation. It's possible it's no longer necessary.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's interesting, well to resolve the issue I see a few options:
1.put the unhandledRejection catcher itself behind a flag
2. fix abort to be compatible with an external unhandledRejection (this PR)
3. remove the unhandledRejection (might break something if it is actually used somewhere)

The last one would be the best option in my opinion, but only if we can confirm that it is safe to remove

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kripken I found an old issue referring to the reason of catching unhandledRejection,
#7855

It seems that there is/was a test case for it, so I might check if it's still valid

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, yes, let's see if we still need to have unhandledRejection code. If not, then it would be nice to not have to add code to support circularity etc. as discussed earlier.

Copy link
Contributor Author

@i404788 i404788 Jun 5, 2019

Choose a reason for hiding this comment

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

I've looked at the test and it still fails if you comment unhandledRejection. I've changed abort to use exit (and console.error) instead of throw, and now it does work without unhandledRejection. I'll be running the full test suite to make sure it doesn't break something else. If everything seems reasonable I'll create another PR.

@i404788
Copy link
Contributor Author

i404788 commented May 23, 2019

This language feature is only supported for ECMASCRIPT6 mode or better: block-scoped function declaration.

Moving the function inside abort seems to break the closure compiler build, will probably revert back
Looks like I need to move it to the outermost scope function

@kripken
Copy link
Member

kripken commented May 28, 2019

Thanks @i404788 , I think this idea is good! I'd prefer not to increase code size by default, though - how about only doing this when ASSERTIONS are on?

@i404788
Copy link
Contributor Author

i404788 commented May 28, 2019

@kripken That sounds like a good idea, I think it would be good to replace the JSON.stringify altogether in non-ASSERTIONS builds with just toString() (as per @sbc100 suggestion).
That should resolve early crashing if an unhandledRejection is thrown, while still giving sufficient feedback to developers.

@kripken
Copy link
Member

kripken commented May 30, 2019

Thanks, I think this looks good, but need to merge in latest incoming to fix the CI errors.

@kripken
Copy link
Member

kripken commented Jun 7, 2019

Where do we stand on this PR? Separate from the unhandled rejection issue, I think it's best to not handle the circular case, that is, not do JSON.stringify. That seems simplest.

@i404788
Copy link
Contributor Author

i404788 commented Jun 8, 2019

I would agree, for this PR/issue removing JSON.stringify would suffice.
I've updated it to just remove that line, PTAL

@kripken
Copy link
Member

kripken commented Jun 10, 2019

This looks good, but those errors on sanity look weird. I re-rean the test suite and they still happen. Do you see them locally? python tests/runner.py sanity.test_emcc_caching, but note that it will modify your .emscripten file, the sanity tests are weird. Or, maybe just merge in latest incoming to here, which we need to do anyhow for the conflicts, perhaps that will fix it if it was a problem on older incoming.

@i404788
Copy link
Contributor Author

i404788 commented Jun 10, 2019

I saw the test failure and looked into it for a bit. The CI seems to suggests there is inactivity for a long time which is the same when I run it locally, however I have no idea where it comes from.
I'll try running the test at incoming just before my changes

@i404788
Copy link
Contributor Author

i404788 commented Jun 10, 2019

Results (time python tests/runner.py sanity.test_emcc_caching):

real	7m15.562s
user    6m16.250s
sys	3m11.969s

To be fair the tests run a lot faster on my machine compared to the CI so I wouldn't be surprised if there was a 10m timeout on the CI. I'll try merging to latest incoming
EDIT: rebased to latest it's still just as slow, so I'm not sure if that's just 'normal' behavior

@kripken
Copy link
Member

kripken commented Jun 11, 2019

Thanks! Looks good now on the tests.

@kripken kripken merged commit 6569dd8 into emscripten-core:incoming Jun 11, 2019
belraquib pushed a commit to belraquib/emscripten that referenced this pull request Dec 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants