Changeset 252497 in webkit

Timestamp:

Nov 15, 2019 12:08:08 PM (4 years ago)

Author:

Chris Dumez

Message:

Regression: http/tests/navigation/page-cache-getUserMedia-pending-promise.html is crashing in Debug
​https://bugs.webkit.org/show_bug.cgi?id=204232

Reviewed by Eric Carlson.

No new tests, covered by http/tests/navigation/page-cache-getUserMedia-pending-promise.html.

• Modules/mediastream/MediaStreamTrack.cpp:

(WebCore::MediaStreamTrack::create):
(WebCore::MediaStreamTrack::MediaStreamTrack):
Call suspendIfNeeded() in the factory and not in the constructor. It is never safe to call
suspendIfNeeded() from inside the constructor because it may call the suspend() method, which
may ref |this|.

• Modules/mediastream/UserMediaRequest.cpp:

(WebCore::UserMediaRequest::allow):
Queue a task on the HTML event loop when the user media request is approved. This way, if the
page is suspended when this happens, we delay constructing the media stream (among other things)
until the page actually resumes.

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/mediastream/MediaStreamTrack.cpp (modified) (2 diffs)
• Modules/mediastream/UserMediaRequest.cpp (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-11-15  Chris Dumez  <cdumez@apple.com>
+
+        Regression: http/tests/navigation/page-cache-getUserMedia-pending-promise.html is crashing in Debug
+        https://bugs.webkit.org/show_bug.cgi?id=204232
+
+        Reviewed by Eric Carlson.
+
+        No new tests, covered by http/tests/navigation/page-cache-getUserMedia-pending-promise.html.
+
+        * Modules/mediastream/MediaStreamTrack.cpp:
+        (WebCore::MediaStreamTrack::create):
+        (WebCore::MediaStreamTrack::MediaStreamTrack):
+        Call suspendIfNeeded() in the factory and not in the constructor. It is never safe to call
+        suspendIfNeeded() from inside the constructor because it may call the suspend() method, which
+        may ref |this|.
+
+        * Modules/mediastream/UserMediaRequest.cpp:
+        (WebCore::UserMediaRequest::allow):
+        Queue a task on the HTML event loop when the user media request is approved. This way, if the
+        page is suspended when this happens, we delay constructing the media stream (among other things)
+        until the page actually resumes.
+
 2019-11-15  Zalan Bujtas  <zalan@apple.com>
 

trunk/Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp

@@ -63 +63 @@
 Ref<MediaStreamTrack> MediaStreamTrack::create(ScriptExecutionContext& context, Ref<MediaStreamTrackPrivate>&& privateTrack)
 {
-    return adoptRef(*new MediaStreamTrack(context, WTFMove(privateTrack)));
+    auto track = adoptRef(*new MediaStreamTrack(context, WTFMove(privateTrack)));
+    track->suspendIfNeeded();
+    return track;
 }
 
@@ -74 +76 @@
 {
     ALWAYS_LOG(LOGIDENTIFIER);
-    suspendIfNeeded();
 
     m_private->addObserver(*this);

trunk/Source/WebCore/Modules/mediastream/UserMediaRequest.cpp

@@ -232 +232 @@
     RELEASE_LOG(MediaStream, "UserMediaRequest::allow %s %s", audioDevice ? audioDevice.persistentId().utf8().data() : "", videoDevice ? videoDevice.persistentId().utf8().data() : "");
 
-    auto callback = [this, protector = makePendingActivity(*this), completionHandler = WTFMove(completionHandler)](RefPtr<MediaStreamPrivate>&& privateStream) mutable {
-        auto scopeExit = makeScopeExit([completionHandler = WTFMove(completionHandler)]() mutable {
-            completionHandler();
-        });
-        if (isContextStopped())
+    queueTaskKeepingObjectAlive(*this, TaskSource::UserInteraction, [this, audioDevice = WTFMove(audioDevice), videoDevice = WTFMove(videoDevice), deviceIdentifierHashSalt = WTFMove(deviceIdentifierHashSalt), completionHandler = WTFMove(completionHandler)]() mutable {
+        auto callback = [this, protector = makePendingActivity(*this), completionHandler = WTFMove(completionHandler)](RefPtr<MediaStreamPrivate>&& privateStream) mutable {
+            auto scopeExit = makeScopeExit([completionHandler = WTFMove(completionHandler)]() mutable {
+                completionHandler();
+            });
+            if (isContextStopped())
+                return;
+
+            if (!privateStream) {
+                RELEASE_LOG(MediaStream, "UserMediaRequest::allow failed to create media stream!");
+                deny(MediaAccessDenialReason::HardwareError);
+                return;
+            }
+
+            auto& document = downcast<Document>(*m_scriptExecutionContext);
+            privateStream->monitorOrientation(document.orientationNotifier());
+
+            auto stream = MediaStream::create(document, privateStream.releaseNonNull());
+            stream->startProducingData();
+
+            if (!isMediaStreamCorrectlyStarted(stream)) {
+                deny(MediaAccessDenialReason::HardwareError);
+                return;
+            }
+
+            ASSERT(document.isCapturing());
+            stream->document()->setHasCaptureMediaStreamTrack();
+            m_promise->resolve(WTFMove(stream));
+        };
+
+        auto& document = downcast<Document>(*scriptExecutionContext());
+        document.setDeviceIDHashSalt(deviceIdentifierHashSalt);
+
+        RealtimeMediaSourceCenter::singleton().createMediaStream(document.logger(), WTFMove(callback), WTFMove(deviceIdentifierHashSalt), WTFMove(audioDevice), WTFMove(videoDevice), m_request);
+
+        if (!m_scriptExecutionContext)
             return;
 
-        if (!privateStream) {
-            RELEASE_LOG(MediaStream, "UserMediaRequest::allow failed to create media stream!");
-            deny(MediaAccessDenialReason::HardwareError);
-            return;
-        }
-
-        auto& document = downcast<Document>(*m_scriptExecutionContext);
-        privateStream->monitorOrientation(document.orientationNotifier());
-
-        auto stream = MediaStream::create(document, privateStream.releaseNonNull());
-        stream->startProducingData();
-
-        if (!isMediaStreamCorrectlyStarted(stream)) {
-            deny(MediaAccessDenialReason::HardwareError);
-            return;
-        }
-
-        ASSERT(document.isCapturing());
-        stream->document()->setHasCaptureMediaStreamTrack();
-        m_promise->resolve(WTFMove(stream));
-    };
-
-    auto& document = downcast<Document>(*scriptExecutionContext());
-    document.setDeviceIDHashSalt(deviceIdentifierHashSalt);
-
-    RealtimeMediaSourceCenter::singleton().createMediaStream(document.logger(), WTFMove(callback), WTFMove(deviceIdentifierHashSalt), WTFMove(audioDevice), WTFMove(videoDevice), m_request);
-
-    if (!m_scriptExecutionContext)
-        return;
-
 #if ENABLE(WEB_RTC)
-    if (auto* page = document.page())
-        page->rtcController().disableICECandidateFilteringForDocument(document);
+        if (auto* page = document.page())
+            page->rtcController().disableICECandidateFilteringForDocument(document);
 #endif
+    });
 }