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

[api-minor] Replace the PromiseCapability with Promise.withResolvers() #17854

Merged
merged 1 commit into from
Apr 2, 2024

Conversation

Snuffleupagus
Copy link
Collaborator

This replaces our custom PromiseCapability-class with the new native Promise.withResolvers() functionality, which does almost the same thing[1]; please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers

The only difference is that PromiseCapability also had a settled-getter, which was however not widely used and the call-sites can either be removed or re-factored to avoid it. In particular:

  • In src/display/api.js we can tweak the PDFObjects-class to use a "special" initial data-value and just compare against that, in order to replace the settled-state.
  • In web/app.js we change the only case to manually track the settled-state, which should hopefully be OK given how this is being used.
  • In web/pdf_outline_viewer.js we can remove the settled-checks, since the code should work just fine without it. The only thing that could potentially happen is that we try to resolve a Promise multiple times, which is however not a problem since the value of a Promise cannot be changed once fulfilled or rejected.
  • In web/pdf_viewer.js we can remove the settled-checks, since the code should work fine without them:
    • For the _onePageRenderedCapability case the settled-check is used in a EventBus-listener which is removed on its first (valid) invocation.
    • For the _pagesCapability case the settled-check is used in a print-related helper that works just fine with "only" the other checks.
  • In test/unit/api_spec.js we can change the few relevant cases to manually track the settled-state, since this is both simple and test-only code.

[1] In browsers/environments that lack native support, note the compatibility data, it'll be polyfilled via the core-js library (but only in legacy builds).

@Snuffleupagus
Copy link
Collaborator Author

/botio-linux preview

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_preview from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/678d40cc0bcb042/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/678d40cc0bcb042/output.txt

Total script time: 1.24 mins

Published

@Snuffleupagus Snuffleupagus force-pushed the rm-PromiseCapability branch 2 times, most recently from c5692b6 to d7f45fe Compare March 29, 2024 11:00
@Snuffleupagus
Copy link
Collaborator Author

/botio test

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/2c352b25ce5b0e2/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_test from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/7e51f14bda71aca/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Failed

Full output at http://54.241.84.105:8877/2c352b25ce5b0e2/output.txt

Total script time: 26.24 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 12

Image differences available at: http://54.241.84.105:8877/2c352b25ce5b0e2/reftest-analyzer.html#web=eq.log

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Failed

Full output at http://54.193.163.58:8877/7e51f14bda71aca/output.txt

Total script time: 44.68 mins

  • Unit tests: Passed
  • Integration Tests: Passed
  • Regression tests: FAILED
  different ref/snapshot: 5

Image differences available at: http://54.193.163.58:8877/7e51f14bda71aca/reftest-analyzer.html#web=eq.log

…rs()`

This replaces our custom `PromiseCapability`-class with the new native `Promise.withResolvers()` functionality, which does *almost* the same thing[1]; please see https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers

The only difference is that `PromiseCapability` also had a `settled`-getter, which was however not widely used and the call-sites can either be removed or re-factored to avoid it. In particular:
 - In `src/display/api.js` we can tweak the `PDFObjects`-class to use a "special" initial data-value and just compare against that, in order to replace the `settled`-state.
 - In `web/app.js` we change the only case to manually track the `settled`-state, which should hopefully be OK given how this is being used.
 - In `web/pdf_outline_viewer.js` we can remove the `settled`-checks, since the code should work just fine without it. The only thing that could potentially happen is that we try to `resolve` a Promise multiple times, which is however *not* a problem since the value of a Promise cannot be changed once fulfilled or rejected.
 - In `web/pdf_viewer.js` we can remove the `settled`-checks, since the code should work fine without them:
     - For the `_onePageRenderedCapability` case the `settled`-check is used in a `EventBus`-listener which is *removed* on its first (valid) invocation.
     - For the `_pagesCapability` case the `settled`-check is used in a print-related helper that works just fine with "only" the other checks.
 - In `test/unit/api_spec.js` we can change the few relevant cases to manually track the `settled`-state, since this is both simple and *test-only* code.

---
[1] In browsers/environments that lack native support, note [the compatibility data](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Promise/withResolvers#browser_compatibility), it'll be polyfilled via the `core-js` library (but only in `legacy` builds).
@Snuffleupagus
Copy link
Collaborator Author

/botio unittest

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.241.84.105:8877/b50e1ee01b54dfb/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Received

Command cmd_unittest from @Snuffleupagus received. Current queue size: 0

Live output at: http://54.193.163.58:8877/c45446bc80fde70/output.txt

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Linux m4)


Success

Full output at http://54.241.84.105:8877/b50e1ee01b54dfb/output.txt

Total script time: 2.38 mins

  • Unit Tests: Passed

@moz-tools-bot
Copy link
Collaborator

From: Bot.io (Windows)


Success

Full output at http://54.193.163.58:8877/c45446bc80fde70/output.txt

Total script time: 11.64 mins

  • Unit Tests: Passed

@timvandermeij timvandermeij merged commit 2e52829 into mozilla:master Apr 2, 2024
9 checks passed
@timvandermeij
Copy link
Contributor

It's really nice that this custom helper can now be replaced with a supported JS language feature. Thanks!

@cjboy76
Copy link

cjboy76 commented Apr 20, 2024

For those people processing PDF document in server side receiving message Promise.withResolvers is not a function, currently Promise.withResolvers is not supported in Node.js, see here.

@LZQCN
Copy link

LZQCN commented May 24, 2024

This PR caused a butterfly effect, many users need to fix compatibility errors.

@Snuffleupagus
Copy link
Collaborator Author

This PR caused a butterfly effect, many users need to fix compatibility errors.

There won't be any issues if you, as already mentioned in #17854 (comment), use a legacy build of the PDF.js library; please also see https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

@mzedeler
Copy link

mzedeler commented Jun 4, 2024

This PR caused a butterfly effect, many users need to fix compatibility errors.

There won't be any issues if you, as already mentioned in #17854 (comment), use a legacy build of the PDF.js library; please also see https://github.com/mozilla/pdf.js/wiki/Frequently-Asked-Questions#faq-support

Can you point to an older version that doesn't rely on withResolvers while still not having this high severity vulnerability?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants