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

Something about xsnap not compat with SES 0.12.1 #570

Closed
erights opened this issue Feb 3, 2021 · 8 comments · Fixed by #571
Closed

Something about xsnap not compat with SES 0.12.1 #570

erights opened this issue Feb 3, 2021 · 8 comments · Fixed by #571
Assignees

Comments

@erights
Copy link
Contributor

erights commented Feb 3, 2021

See CI failure at https://github.com/Agoric/agoric-sdk/pull/2323/checks?check_run_id=1819852110 . A relevant part seems to be

  Error {
    message: 'Uncaught exception in v1: SyntaxError: ^((?:.*[( ])?)[:/\\w-_] invalid range',
  }

  › runToIdle (packages/xsnap/src/xsnap.js:126:15)
@dckc
Copy link
Contributor

dckc commented Feb 3, 2021

also relevant: the failure was in the workers › worker › xs vat manager test

possibly related:

@michaelfig
Copy link
Member

michaelfig commented Feb 3, 2021

It's the following regex in SES:

const FILENAME_FILTER = /^((?:.*[( ])?)[:/\w-_]*\/(packages\/.+)$/;

According to https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Regular_Expressions/Groups_and_Ranges

A character class. Matches any one of the enclosed characters. You can specify a range of characters by using a hyphen, but if the hyphen appears as the first or last character enclosed in the square brackets it is taken as a literal hyphen to be included in the character class as a normal character.

So, I suspect that [-:/\w_] or [:/\w_-] would work just fine.

@dckc
Copy link
Contributor

dckc commented Feb 3, 2021

Thanks, @michaelfig .

I'm inclined to transfer this to the SES-shim repo and refine the title, in that case. I'll stand by for a bit for feedback.

@erights
Copy link
Contributor Author

erights commented Feb 3, 2021

Thanks @michaelfig !

@dckc yes, please move it. Thanks.

@dckc dckc transferred this issue from Agoric/agoric-sdk Feb 3, 2021
erights added a commit that referenced this issue Feb 4, 2021
@erights
Copy link
Contributor Author

erights commented Feb 4, 2021

Since #571 claims to fix a bug that doesn't happen for me locally, and only happens to Agoric/agoric-sdk#2323 under CI, how can we test #571 without making a new SES release and switching Agoric/agoric-sdk#2323 to depend on that newer release instead? Or is this indeed the lowest friction way to test #571 ?

@dckc
Copy link
Contributor

dckc commented Feb 4, 2021

I built a test to verify that xsnap loses on the former regex. I stashed it but didn't check it in.

I can make a test for the fixed regex...

erights added a commit that referenced this issue Feb 4, 2021
@dckc
Copy link
Contributor

dckc commented Feb 4, 2021

The tests below verify the fixed regex.

You can also do it with the repl:

~/projects/agoric/agoric-sdk/packages/xsnap$ ./src/xsrepl
xs> const FILENAME_FILTER = /^((?:.*[( ])?)[:/\w_-]*\/(packages\/.+)$/;

adding these to the branch for Agoric/agoric-sdk#2266 is a bit lazy, but...

reject non-std range
Agoric/agoric-sdk@9f2ac65

accept std range
Agoric/agoric-sdk@8967b0e

@dckc
Copy link
Contributor

dckc commented Feb 4, 2021

This episode suggests bumping up the priority of #508 .

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 a pull request may close this issue.

4 participants