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.

Sign up for GitHub

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

Jump to bottom

Crash under ~TimerBase() from LibWebRTCAudioModule #2010

Merged
merged 1 commit into from Jul 2, 2022
Merged

Crash under ~TimerBase() from LibWebRTCAudioModule #2010

merged 1 commit into from Jul 2, 2022

Conversation

cdumez
Copy link
Contributor

@cdumez cdumez commented Jul 1, 2022 •
edited by webkit-commit-queue

1fc2703

Crash under ~TimerBase() from LibWebRTCAudioModule
https://bugs.webkit.org/show_bug.cgi?id=242262
<rdar://81438938>

Reviewed by Eric Carlson.

LibWebRTCAudioModule has a Timer data member which gets constructed on the main thread,
gets started / stopped on the main thread and fires on the main thread. However, the
LibWebRTCAudioModule can get destroyed on a non-main thread, which would also destroy
the Timer on that thread. This wasn't safe and would result in crashes.

To address the issue, I now do a callOnMainThreadAndWait() in the LibWebRTCAudioModule
destructor and make sure to destroy the timer on the main thread. The "AndWait" part
is necessary since the timer may fire on the main thread during the execution on the
LibWebRTCAudioModule destructor on the background thread.

* Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:
(WebCore::LibWebRTCAudioModule::LibWebRTCAudioModule):
(WebCore::LibWebRTCAudioModule::~LibWebRTCAudioModule):
(WebCore::LibWebRTCAudioModule::StartPlayout):
(WebCore::LibWebRTCAudioModule::StopPlayout):
* Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:

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

@cdumez cdumez self-assigned this Jul 1, 2022
@cdumez cdumez added Media Bugs related to the HTML 5 Media elements. WebKit Nightly Build labels Jul 1, 2022
eric-carlson
eric-carlson approved these changes Jul 1, 2022
View reviewed changes
Copy link
Contributor

@eric-carlson eric-carlson left a comment

Choose a reason for hiding this comment

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

r=me

@webkit-early-warning-system webkit-early-warning-system added the merging-blocked Applied to prevent a change from being merged label Jul 1, 2022
@cdumez cdumez added merge-queue Applied to send a pull request to merge-queue and removed merging-blocked Applied to prevent a change from being merged labels Jul 2, 2022
@cdumez
Crash under ~TimerBase() from LibWebRTCAudioModule
1fc2703 
https://bugs.webkit.org/show_bug.cgi?id=242262
<rdar://81438938>

Reviewed by Eric Carlson.

LibWebRTCAudioModule has a Timer data member which gets constructed on the main thread,
gets started / stopped on the main thread and fires on the main thread. However, the
LibWebRTCAudioModule can get destroyed on a non-main thread, which would also destroy
the Timer on that thread. This wasn't safe and would result in crashes.

To address the issue, I now do a callOnMainThreadAndWait() in the LibWebRTCAudioModule
destructor and make sure to destroy the timer on the main thread. The "AndWait" part
is necessary since the timer may fire on the main thread during the execution on the
LibWebRTCAudioModule destructor on the background thread.

* Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.cpp:
(WebCore::LibWebRTCAudioModule::LibWebRTCAudioModule):
(WebCore::LibWebRTCAudioModule::~LibWebRTCAudioModule):
(WebCore::LibWebRTCAudioModule::StartPlayout):
(WebCore::LibWebRTCAudioModule::StopPlayout):
* Source/WebCore/platform/mediastream/libwebrtc/LibWebRTCAudioModule.h:

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

Committed 252085@main (1fc2703): https://commits.webkit.org/252085@main

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

@webkit-early-warning-system webkit-early-warning-system merged commit 1fc2703 into WebKit:main Jul 2, 2022
@webkit-commit-queue webkit-commit-queue removed the merge-queue Applied to send a pull request to merge-queue label Jul 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@eric-carlson eric-carlson eric-carlson approved these changes

@youennf youennf Awaiting requested review from youennf

@geoffreygaren geoffreygaren Awaiting requested review from geoffreygaren

Assignees

@cdumez cdumez

Labels
Media Bugs related to the HTML 5 Media elements.
Projects
None yet
Milestone
No milestone
4 participants
@cdumez @webkit-commit-queue @eric-carlson @webkit-early-warning-system