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

ReadableStream::pipeTo memory leak #6759

Merged

Conversation

youennf
Copy link
Contributor

@youennf youennf commented Nov 23, 2022

cb01f4c

ReadableStream::pipeTo memory leak
https://bugs.webkit.org/show_bug.cgi?id=247618
rdar://problem/102366716

Reviewed by Alex Christensen.

Before the patch, a reference to pipeState would be kept in the AbortSignal.
We are now removing the algorithm from thr AbortSignal when finalizing pipeTo to break the reference cycle.

* Source/WebCore/Modules/streams/ReadableStreamInternals.js:
(readableStreamPipeToWritableStream):
(pipeToFinalize):
* Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:
(WebCore::JSC_DEFINE_HOST_FUNCTION):
(WebCore::JSDOMGlobalObject::addBuiltinGlobals):
* Source/WebCore/bindings/js/WebCoreBuiltinNames.h:
* Source/WebCore/dom/AbortSignal.cpp:
(WebCore::AbortSignal::signalAbort):
(WebCore::AbortSignal::addAbortAlgorithmToSignal):
(WebCore::AbortSignal::removeAbortAlgorithmFromSignal):
(WebCore::AbortSignal::addAlgorithm):
(WebCore::AbortSignal::removeAlgorithm):
(WebCore::AbortSignal::whenSignalAborted): Deleted.
* Source/WebCore/dom/AbortSignal.h:
* Source/WebCore/dom/AbortSignal.idl:

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

0c85a4a

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

@youennf youennf self-assigned this Nov 23, 2022
@youennf youennf added the WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit). label Nov 23, 2022
@@ -84,7 +86,8 @@ class AbortSignal final : public RefCounted<AbortSignal>, public EventTarget, pr
void eventListenersDidChange() final;

bool m_aborted { false };
Vector<Algorithm> m_algorithms;
Vector<std::pair<uint32_t, Algorithm>> m_algorithms;
uint32_t m_algorithmIdentifier { 0 };
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be an ObjectIdentifier of some sort?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about this, the issue is that the algorithm identifier is stored by built-in JS code which only knows about integers, not ObjectIdentifier.

@youennf youennf added the merge-queue Applied to send a pull request to merge-queue label Nov 29, 2022
https://bugs.webkit.org/show_bug.cgi?id=247618
rdar://problem/102366716

Reviewed by Alex Christensen.

Before the patch, a reference to pipeState would be kept in the AbortSignal.
We are now removing the algorithm from thr AbortSignal when finalizing pipeTo to break the reference cycle.

* Source/WebCore/Modules/streams/ReadableStreamInternals.js:
(readableStreamPipeToWritableStream):
(pipeToFinalize):
* Source/WebCore/bindings/js/JSDOMGlobalObject.cpp:
(WebCore::JSC_DEFINE_HOST_FUNCTION):
(WebCore::JSDOMGlobalObject::addBuiltinGlobals):
* Source/WebCore/bindings/js/WebCoreBuiltinNames.h:
* Source/WebCore/dom/AbortSignal.cpp:
(WebCore::AbortSignal::signalAbort):
(WebCore::AbortSignal::addAbortAlgorithmToSignal):
(WebCore::AbortSignal::removeAbortAlgorithmFromSignal):
(WebCore::AbortSignal::addAlgorithm):
(WebCore::AbortSignal::removeAlgorithm):
(WebCore::AbortSignal::whenSignalAborted): Deleted.
* Source/WebCore/dom/AbortSignal.h:
* Source/WebCore/dom/AbortSignal.idl:

Canonical link: https://commits.webkit.org/257109@main
@webkit-commit-queue
Copy link
Collaborator

Committed 257109@main (cb01f4c): https://commits.webkit.org/257109@main

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

@webkit-commit-queue webkit-commit-queue merged commit cb01f4c into WebKit:main Nov 29, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
WebCore Misc. For miscellaneous bugs in the WebCore framework (and not JavaScriptCore or WebKit).
Projects
None yet
4 participants