commit 32f1e9f90254f50f37c2a1f3edebfa4705219d43
author cdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc> Sun Jun 18 20:25:19 2017 +0000
committer cdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc> Sun Jun 18 20:25:19 2017 +0000
tree b45540a6daf6227e41b0d8641d59e8376118953f
parent b44b208fd21dd5f34bb69e6f762f80e58d8bd94d

Crash when re-entering MediaDevicesEnumerationRequest::cancel()
https://bugs.webkit.org/show_bug.cgi?id=173522
<rdar://problem/31185739>

Reviewed by Darin Adler.

Source/WebCore:

When a MediaDevicesRequest is started, it creates a MediaDevicesEnumerationRequest
object and passes a completion handler to that MediaDevicesEnumerationRequest
object. The completion handler holds a reference to the MediaDevicesRequest object
so that its stays alive until the MediaDevicesEnumerationRequest either completes
or is canceled. MediaDevicesRequest also holds a reference to the
MediaDevicesEnumerationRequest object via its m_enumerationRequest data member.

When the document is destroyed, both MediaDevicesRequest::contextDestroyed() and
MediaDevicesEnumerationRequest::contextDestroyed() gets called and the other is not
pre-determined. If MediaDevicesEnumerationRequest::contextDestroyed() gets called
first then it calls MediaDevicesEnumerationRequest::cancel(). Calling cancel() ends
up destroying the completion handler. Destroying the completion handler ends up
dereferencing and destroying the MediaDevicesRequest object. The MediaDevicesRequest
destructor would call MediaDevicesEnumerationRequest::cancel() again, causing us to
re-enter it and assign nullptr to the completion callback again. Re-entering
std::function's operator=(nullptr_t) is not safe because of the way it is implemented
as we end up trying to destroy the lambda twice and crashing. Using a WTF::Function
instead fixes this particular issue because re-entering WTF::Function's operator=(nullptr_t)
is safe.

However, this fix is not sufficient. Calling the MediaDevicesRequest destructor also
dereferencing and destroys the MediaDevicesEnumerationRequest object. As a result,
when MediaDevicesEnumerationRequest::contextDestroyed() returns from its call to cancel
|this| is already dead when we call ContextDestructionObserver::contextDestroyed().
To address this issue, we now protect |this| in MediaDevicesEnumerationRequest::contextDestroyed().

Test: fast/mediastream/destroy-document-while-enumerating-devices.html

* Modules/mediastream/MediaDevicesEnumerationRequest.cpp:
(WebCore::MediaDevicesEnumerationRequest::contextDestroyed):
Protect |this| as the call to cancel() may destroy |this| before calling
ContextDestructionObserver::contextDestroyed() otherwise.

* Modules/mediastream/MediaDevicesEnumerationRequest.h:
Use WTF::Function instead of std::function for the completion handler as
it is safer (in terms of re-entrency) and avoids unnecessary copying.

* Modules/mediastream/MediaDevicesRequest.cpp:
(WebCore::MediaDevicesRequest::~MediaDevicesRequest):
Stop calling MediaDevicesEnumerationRequest::cancel(). When the destructor
is called, the MediaDevicesEnumerationRequest has either completed or been
canceled so there is no need to cancel again. I added an assertion to
make sure it is the case. This avoids re-entering
MediaDevicesEnumerationRequest::cancel() is some cases, which was risky.

(WebCore::MediaDevicesRequest::start):
Add comment for clarity and capture a Ref<> instead of a RefPtr<> now that
we can since we use WTF::Function.

Tools:

Add API test for re-entering Function's assignment operators.

* TestWebKitAPI/TestWebKitAPI.xcodeproj/project.pbxproj:
* TestWebKitAPI/Tests/WTF/Function.cpp: Added.
(TestWebKitAPI::TestObject::TestObject):
(TestWebKitAPI::TestObject::~TestObject):
(TestWebKitAPI::TestObject::operator()):
(TestWebKitAPI::TEST):

LayoutTests:

Add layout test coverage. This test would flakily crash before the fix because it
relies on the order in which contextDestroyed() is called for MediaDevicesRequest
and MediaDevicesEnumerationRequest.

* fast/mediastream/destroy-document-while-enumerating-devices-expected.txt: Added.
* fast/mediastream/destroy-document-while-enumerating-devices.html: Added.
* fast/mediastream/resources/enumerate-devices-frame.html: Added.

git-svn-id: http://svn.webkit.org/repository/webkit/trunk@218465 268f45cc-cd09-0410-ab3c-d52691b4dbfc