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

Error on piped ReadableStream leads to Unhandled Promise Rejection #11864

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Mar 23, 2023

3a75b5d

Error on piped ReadableStream leads to Unhandled Promise Rejection
https://bugs.webkit.org/show_bug.cgi?id=251463
rdar://problem/105149496

Reviewed by Alex Christensen.

The closed promise can be rejected in which case we do not need to do anything, except make sure the promise is handled.
Replacing undefined by an empty function to make it handled.

Ditto for pendingWritePromise since we want to react on promise settlement, no matter whether resolved or rejected.
This only impacts unhandled promise rejection code since eithwe we ignore the promise altogether, or the current code is using the same handler for fulfillment and rejection.

Removing some GLIB specifc test expectations as they should be the same as the base ones.

* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/close-propagation-backward.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/backpressure.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any.worker-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/abort.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/close-propagation-backward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/close-propagation-forward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/flow-control.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/general.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/pipe-through.any-expected.txt: Removed.
* LayoutTests/streams/pipeTo-unhandled-promise-expected.txt: Added.
* LayoutTests/streams/pipeTo-unhandled-promise.html: Added.
* Source/WebCore/Modules/streams/ReadableStreamInternals.js:
(pipeToClosingMustBePropagatedForward):
(pipeToClosingMustBePropagatedBackward): Deleted.

Canonical link: https://commits.webkit.org/262137@main

0e79d9e

Misc iOS, tvOS & watchOS macOS Linux Windows
βœ… πŸ§ͺ style βœ… πŸ›  ios βœ… πŸ›  mac βœ… πŸ›  wpe βœ… πŸ›  wincairo
βœ… πŸ§ͺ bindings βœ… πŸ›  ios-sim βœ… πŸ›  mac-AS-debug ❌ πŸ§ͺ wpe-wk2
βœ… πŸ§ͺ webkitperl βœ… πŸ§ͺ ios-wk2 βœ… πŸ§ͺ api-mac βœ… πŸ›  gtk
❌ πŸ§ͺ api-ios βœ… πŸ§ͺ mac-wk1 ❌ πŸ§ͺ gtk-wk2
βœ… πŸ›  tv βœ… πŸ§ͺ mac-wk2 ⏳ πŸ§ͺ api-gtk
βœ… πŸ›  tv-sim βœ… πŸ§ͺ mac-AS-debug-wk2
βœ… πŸ›  watch βœ… πŸ§ͺ mac-wk2-stress
βœ… πŸ›  πŸ§ͺ merge βœ… πŸ›  watch-sim

@youennf youennf self-assigned this Mar 23, 2023
@youennf youennf added the JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues. label Mar 23, 2023
@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Mar 23, 2023
@youennf youennf force-pushed the eng/Error-on-piped-ReadableStream-leads-to-Unhandled-Promise-Rejection branch from 5465e96 to a982ed8 Compare March 23, 2023 16:48
@youennf youennf force-pushed the eng/Error-on-piped-ReadableStream-leads-to-Unhandled-Promise-Rejection branch from a982ed8 to 1a6cbd8 Compare March 24, 2023 09:38
@youennf youennf force-pushed the eng/Error-on-piped-ReadableStream-leads-to-Unhandled-Promise-Rejection branch from 1a6cbd8 to 0e79d9e Compare March 24, 2023 12:05
@youennf youennf removed the merging-blocked Applied to prevent a change from being merged label Mar 24, 2023
@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label Mar 27, 2023
https://bugs.webkit.org/show_bug.cgi?id=251463
rdar://problem/105149496

Reviewed by Alex Christensen.

The closed promise can be rejected in which case we do not need to do anything, except make sure the promise is handled.
Replacing undefined by an empty function to make it handled.

Ditto for pendingWritePromise since we want to react on promise settlement, no matter whether resolved or rejected.
This only impacts unhandled promise rejection code since eithwe we ignore the promise altogether, or the current code is using the same handler for fulfillment and rejection.

Removing some GLIB specifc test expectations as they should be the same as the base ones.

* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/abort.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/close-propagation-backward.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/flow-control.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any.worker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/backpressure.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any.serviceworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any.sharedworker-expected.txt:
* LayoutTests/imported/w3c/web-platform-tests/streams/transform-streams/terminate.any.worker-expected.txt:
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/abort.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/close-propagation-backward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/close-propagation-forward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/error-propagation-backward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/error-propagation-forward.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/flow-control.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/general.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/multiple-propagation.any-expected.txt: Removed.
* LayoutTests/platform/glib/imported/w3c/web-platform-tests/streams/piping/pipe-through.any-expected.txt: Removed.
* LayoutTests/streams/pipeTo-unhandled-promise-expected.txt: Added.
* LayoutTests/streams/pipeTo-unhandled-promise.html: Added.
* Source/WebCore/Modules/streams/ReadableStreamInternals.js:
(pipeToClosingMustBePropagatedForward):
(pipeToClosingMustBePropagatedBackward): Deleted.

Canonical link: https://commits.webkit.org/262137@main
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Error-on-piped-ReadableStream-leads-to-Unhandled-Promise-Rejection branch from 0e79d9e to 3a75b5d Compare March 27, 2023 07:25
@webkit-commit-queue
Copy link
Collaborator

Committed 262137@main (3a75b5d): https://commits.webkit.org/262137@main

Reviewed commits have been landed. Closing PR #11864 and removing active labels.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
JavaScriptCore For bugs in JavaScriptCore, the JS engine used by WebKit, other than kxmlcore issues.
Projects
None yet
5 participants