Fixes PROXYFS error check #18716
Fixes PROXYFS error check #18716
Conversation
iliakan
force-pushed
the
proxy-fix
branch
4 times, most recently
from
ac71797
to
2b09299
Compare
| @@ -1061,7 +1061,7 @@ function wrapSyscallFunction(x, library, isWasi) { | |||
| pre += 'try {\n'; | |||
| handler += | |||
| "} catch (e) {\n" + | |||
| " if (typeof FS == 'undefined' || !(e instanceof FS.ErrnoError)) throw e;\n"; | |||
| " if (typeof FS == 'undefined' || !(e.name === 'ErrnoError')) throw e;\n"; | |||
There was a problem hiding this comment.
I think we maybe need to think a little more carefully about this. I think maybe the arguments made in #16994 (comment) may apply here.
If we do want to make this code work between 2 different emscripten instances I think we should have some testing of that mode, and also make sure that doesn't effect the code size for folks who don't need/want that.
There was a problem hiding this comment.
This kind of test helps to detect errors passed between iframes too. Not sure if it's needed here, though ;)
That's a JS trick like 20 years old. Or more, didn't count. Since the moment instanceof appeared, I'd guess =)
A theoretical downside is that inheritance tests won't work, unlike instanceof, but we don't inherit from FS.ErrnoError. So that's fine.
Also, no one meddles with FS errors by assigning them a custom name, so guess won't be any compat issues.
PROXYFS without the proper test (as I suggest or give your idea) is rather buggy, it causes failures in all wrapped syscalls. It almost feels like I'm beta-testing this feature =)
There was a problem hiding this comment.
There was a problem hiding this comment.
I'm actually not sure about this patch at all. If you are trying to import/use the FS object from another module why wouldn't you also import its FS.ErrnoError? In that mode wouldn't you want to completely avoid declaring FS (or FS.ErrnoError) in the consumer module?
There was a problem hiding this comment.
i.e. if you want module to somehow share and FS then only one of them should declare the FS, right?
There was a problem hiding this comment.
I added/pushed the test that demonstrates the bug on the current HEAD, and it's fixed in my branch.
In short, the problem happens in access syscall to an absent path in the mounted FS, it calls lookupPath and expects it to return an FS.ErrnoError, and it is such error, but from another FS, so the failing instanceof check causes a swift death.
P.S. So this happens in the "normal" scenario of PROXYFS usage, no magic.
P.P.S. Thank you for fast consideration of the PR. I almost forgot what the issue was exactly, but easily managed to remember and write it out.
|
@kripken I think you need to accept these changes before we can land. |
Make the check work reliably cross-FS.
Fixes #18715