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

Update mozjs to 89 #28449

Closed
wants to merge 4 commits into from
Closed

Update mozjs to 89 #28449

wants to merge 4 commits into from

Conversation

sagudev
Copy link
Member

@sagudev sagudev commented May 29, 2021

Trying to catch Firefox release.
Depends on servo/rust-mozjs#543

@highfive
Copy link

Heads up! This PR modifies the following files:

  • @asajeffrey: components/script/dom/bindings/codegen/CodegenRust.py, components/script/script_module.rs, components/script/Cargo.toml, components/script/dom/windowproxy.rs, components/script/dom/bindings/proxyhandler.rs
  • @wafflespeanut: python/servo/bootstrap.py
  • @KiChjang: components/script/dom/bindings/codegen/CodegenRust.py, components/script/script_module.rs, components/script/Cargo.toml, components/script/dom/windowproxy.rs, components/script/dom/bindings/proxyhandler.rs

@highfive
Copy link

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!

@highfive highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 29, 2021
@jdm
Copy link
Member

jdm commented May 29, 2021

@bors-servo try

@bors-servo
Copy link
Contributor

⌛ Trying commit a044383 with merge 2e38240...

bors-servo added a commit that referenced this pull request May 29, 2021
Update mozjs to 89

Trying to catch Firefox release.
Depends on servo/rust-mozjs#543
@sagudev
Copy link
Member Author

sagudev commented May 29, 2021

@jdm mac failed on packaging app.

@bors-servo
Copy link
Contributor

💔 Test failed - checks-github

@highfive highfive added the S-tests-failed The changes caused existing tests to fail. label May 29, 2021
@sagudev
Copy link
Member Author

sagudev commented May 29, 2021

After reviewing tests, it looks that most of them become PASS (mostly wasm stuff), but it looks like something is wrong with promises.

@sagudev
Copy link
Member Author

sagudev commented May 29, 2021

  ▶ Unexpected subtest result in /html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html:
  │ FAIL [expected PASS] b'import() must reject with the same error object for each import when there is a evaluation error'
  │   → assert_equals: The error objects must be equal expected object "Error: Unevaluated or errored module returned by module resolve hook" but got object "Error: Unevaluated or errored module returned by module resolve hook"

@jdm
Copy link
Member

jdm commented May 29, 2021

The module tests in particular look like there's a problem with those changes in this PR.

@sagudev

This comment has been minimized.

@sagudev
Copy link
Member Author

sagudev commented May 31, 2021

  ▶ Unexpected subtest result in /html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html:
  │ FAIL [expected PASS] b'import() must reject with the same error object for each import when there is a evaluation error'
  │   → assert_equals: The error objects must be equal expected object "Error: Unevaluated or errored module returned by module resolve hook" but got object "Error: Unevaluated or errored module returned by module resolve hook"

Expected error is Error.
These Errors come from https://searchfox.org/mozilla-central/search?q=Unevaluated+or+errored&path=
There is also Unhandled rejection error in this test and in some other test there are problems with promises.

@sagudev
Copy link
Member Author

sagudev commented Jun 5, 2021

On ./mach test-wpt html/semantics/scripting-1/the-script-element/module/dynamic-import/dynamic-imports-script-error.html
Expected:
slika
What its currently:
slika

@sagudev
Copy link
Member Author

sagudev commented Jun 15, 2021

Probably missing some error handling like:
https://hg.mozilla.org/mozilla-central/rev/be113df2bd10#l4.363

@highfive highfive removed the S-tests-failed The changes caused existing tests to fail. label Jun 18, 2021
@bors-servo
Copy link
Contributor

☔ The latest upstream changes (presumably #28538) made this pull request unmergeable. Please resolve the merge conflicts.

@highfive highfive added the S-needs-rebase There are merge conflict errors. label Jul 10, 2021
@Redfire75369
Copy link

According to a GJS developer, @ewlsh in #spidermonkey:mozilla.org matrix.

It looks like they just haven't updated their code for the TLA changes to the promises API
You can look at GJS' mozjs91 MR if you want to get a sense of how to adjust API usage + handle dynamics

I believe they are referring to this MR: https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/632
Even more specifically, this looks like the commit that implemented those changes: https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/632/diffs?commit_id=e4ed245f13a703968b7e2f5acadc36a54452e22a#158c25371cf55a8db28e9b6c41a3ee4840d2cf8d.

@sagudev
Copy link
Member Author

sagudev commented Feb 16, 2022

Due to not having time I'm closing this PR.

@sagudev sagudev closed this Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-awaiting-review There is new code that needs to be reviewed. S-needs-rebase There are merge conflict errors.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants