RESOLVED FIXED 170355
fast/mediastream/MediaStream-page-muted.html times out and asserts
https://bugs.webkit.org/show_bug.cgi?id=170355
Summary fast/mediastream/MediaStream-page-muted.html times out and asserts
Alexey Proskuryakov
Reported 2017-03-31 12:15:45 PDT
Fairly frequent timeouts and assertion failures on fast/mediastream/MediaStream-page-muted.html This assertion means that event target wrapper got garbage collected, so this is a general problem with MediaStreamTrack that would affect customers and many test cases. We need to properly protect the wrapper when it's needed. ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction /Volumes/Data/slave/elcapitan-debug/build/Source/WebCore/bindings/js/JSEventListener.h(127) : JSC::JSObject *WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext *) const 1 0x115194270 WTFCrash 2 0x108a46868 WebCore::JSEventListener::jsFunction(WebCore::ScriptExecutionContext*) const 3 0x108e48d24 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext*, WebCore::Event*) 4 0x1082fffdf WebCore::EventTarget::fireEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener>, 1ul, WTF::CrashOnOverflow, 16ul>) 5 0x1082ffb96 WebCore::EventTarget::fireEventListeners(WebCore::Event&) 6 0x1082ffa29 WebCore::EventTarget::dispatchEvent(WebCore::Event&) 7 0x10990c741 WebCore::MediaStreamTrack::trackMutedChanged(WebCore::MediaStreamTrackPrivate&) 8 0x10990c79f non-virtual thunk to WebCore::MediaStreamTrack::trackMutedChanged(WebCore::MediaStreamTrackPrivate&) 9 0x109913b78 WebCore::MediaStreamTrackPrivate::sourceMutedChanged() 10 0x109b81cc0 WebCore::RealtimeMediaSource::setMuted(bool) 11 0x1099a96ed WebCore::MockRealtimeMediaSource::startProducingData() 12 0x1099ae3af WebCore::MockRealtimeVideoSource::startProducingData() 13 0x109903d90 WebCore::MediaStreamTrackPrivate::startProducingData() 14 0x109900b20 WebCore::MediaStreamPrivate::startProducingData() 15 0x1098f4089 WebCore::MediaStream::startProducingData() 16 0x1098f41b0 WebCore::MediaStream::pageMutedStateDidChange() 17 0x1098f41dc non-virtual thunk to WebCore::MediaStream::pageMutedStateDidChange() 18 0x1081093dd WebCore::Document::pageMutedStateDidChange() 19 0x109a5e5ed WebCore::Page::setMuted(unsigned int) 20 0x11c94ee6b WebCore::Internals::setPageMuted(WTF::String const&)
Attachments
Patch (2.13 KB, patch)
2017-05-17 01:24 PDT, Per Arne Vollan
no flags
Archive of layout-test-results from ews122 for ios-simulator-wk2 (856.78 KB, application/zip)
2017-05-17 03:31 PDT, Build Bot
no flags
Archive of layout-test-results from ews123 for ios-simulator-wk2 (12.21 MB, application/zip)
2017-05-17 04:38 PDT, Build Bot
no flags
Patch (2.13 KB, patch)
2017-05-17 04:40 PDT, Per Arne Vollan
no flags
Archive of layout-test-results from ews103 for mac-elcapitan (1.07 MB, application/zip)
2017-05-17 07:28 PDT, Build Bot
no flags
Patch (2.36 KB, patch)
2017-05-17 12:23 PDT, Per Arne Vollan
no flags
Proposed patch. (13.78 KB, patch)
2017-06-08 04:50 PDT, Eric Carlson
no flags
Proposed patch. (13.91 KB, patch)
2017-06-08 06:38 PDT, Eric Carlson
cdumez: review-
Updated patch. (13.98 KB, patch)
2017-06-08 09:38 PDT, Eric Carlson
no flags
Another patch (15.17 KB, patch)
2017-06-08 14:44 PDT, Eric Carlson
no flags
One more patch (13.87 KB, patch)
2017-06-09 05:24 PDT, Eric Carlson
no flags
Alexey Proskuryakov
Comment 1 2017-03-31 12:18:58 PDT
Radar WebKit Bug Importer
Comment 2 2017-03-31 12:19:16 PDT
Per Arne Vollan
Comment 3 2017-05-17 01:24:26 PDT
Build Bot
Comment 4 2017-05-17 03:31:06 PDT
Comment on attachment 310357 [details] Patch Attachment 310357 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3760170 New failing tests: compositing/absolute-inside-out-of-view-fixed.html
Build Bot
Comment 5 2017-05-17 03:31:08 PDT
Created attachment 310374 [details] Archive of layout-test-results from ews122 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews122 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Build Bot
Comment 6 2017-05-17 04:38:04 PDT
Comment on attachment 310357 [details] Patch Attachment 310357 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: http://webkit-queues.webkit.org/results/3761806 New failing tests: http/tests/loading/resourceLoadStatistics/non-prevalent-resource-without-user-interaction.html
Build Bot
Comment 7 2017-05-17 04:38:06 PDT
Created attachment 310379 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.11.6
Per Arne Vollan
Comment 8 2017-05-17 04:40:20 PDT
Build Bot
Comment 9 2017-05-17 07:28:53 PDT
Comment on attachment 310380 [details] Patch Attachment 310380 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/3762796 New failing tests: http/tests/appcache/404-resource-with-slow-main-resource.php
Build Bot
Comment 10 2017-05-17 07:28:54 PDT
Created attachment 310387 [details] Archive of layout-test-results from ews103 for mac-elcapitan The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Alexey Proskuryakov
Comment 11 2017-05-17 09:56:09 PDT
Will this extend the lifetime forever, until the root object goes away?
Eric Carlson
Comment 12 2017-05-17 10:37:15 PDT
Comment on attachment 310380 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310380&action=review > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:342 > + if (hasEventListeners(eventNames().muteEvent) || hasEventListeners(eventNames().unmuteEvent)) > + return true; Actually, this is the wrong fix.
Eric Carlson
Comment 13 2017-05-17 10:43:19 PDT
The track won't dispatch events if activeDOMObjectsAreSuspended or activeDOMObjectsAreStopped. We should figure out how the wrapper gets garbage collected on a active activeDOMObject.
Per Arne Vollan
Comment 14 2017-05-17 12:23:40 PDT
Per Arne Vollan
Comment 15 2017-05-17 12:33:58 PDT
(In reply to Eric Carlson from comment #13) > The track won't dispatch events if activeDOMObjectsAreSuspended or > activeDOMObjectsAreStopped. We should figure out how the wrapper gets > garbage collected on a active activeDOMObject. It seems the return values of hasPendingActivity() and isFiringEventListeners() determines if the wrapper can be garbage collected: bool JSMediaStreamTrackOwner::isReachableFromOpaqueRoots(JSC::Handle<JSC::Unknown> handle, void*, SlotVisitor& visitor) { auto* jsMediaStreamTrack = jsCast<JSMediaStreamTrack*>(handle.slot()->asCell()); if (jsMediaStreamTrack->wrapped().hasPendingActivity()) return true; if (jsMediaStreamTrack->wrapped().isFiringEventListeners()) return true; UNUSED_PARAM(visitor); return false; }
Darin Adler
Comment 16 2017-06-06 13:44:30 PDT
Comment on attachment 310427 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=310427&action=review > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:343 > + if (!ended()) > + return true; > + return ActiveDOMObject::hasPendingActivity(); Looks OK like this. Could also just write this: return !ended() || ActiveDOMObject::hasPendingActivity(); I find that form quite readable for a simple case like this, maybe more than the early exit style. > Source/WebCore/Modules/mediastream/MediaStreamTrack.h:125 > + // ActiveDOMObject API. > + bool hasPendingActivity() const final; I suggest making this override private rather than public. I also don’t think we need the "ActiveDOMObject API" comment. For one thing, an interface like this inside WebCore is not "API".
Eric Carlson
Comment 17 2017-06-06 14:21:52 PDT
This needs a bit of refinement and a test, I will take it.
Eric Carlson
Comment 18 2017-06-08 04:50:32 PDT
Created attachment 312292 [details] Proposed patch.
Eric Carlson
Comment 19 2017-06-08 06:38:11 PDT
Created attachment 312300 [details] Proposed patch.
Chris Dumez
Comment 20 2017-06-08 08:31:56 PDT
Comment on attachment 312300 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312300&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:521 > + return true; If you return true here, shouldn't you override void ActiveDOMObject::suspend(ReasonForSuspension) ? Or does nothing need suspending? > Source/WebCore/Modules/mediastream/MediaStream.cpp:526 > + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); Can you explain why having a listener counts as having activity? This seems odd. Normally, it is about something being active / playing or having pending events.
Chris Dumez
Comment 21 2017-06-08 08:33:27 PDT
Comment on attachment 312300 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312300&action=review >> Source/WebCore/Modules/mediastream/MediaStream.cpp:526 >> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); > > Can you explain why having a listener counts as having activity? This seems odd. > > Normally, it is about something being active / playing or having pending events. I suspect this would cause leaks if there is any event listener registered.
Geoffrey Garen
Comment 22 2017-06-08 08:53:30 PDT
Comment on attachment 312300 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312300&action=review >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:526 >>> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); >> >> Can you explain why having a listener counts as having activity? This seems odd. >> >> Normally, it is about something being active / playing or having pending events. > > I suspect this would cause leaks if there is any event listener registered. I agree. I think this needs to check m_isActive or m_isProducingData -- whichever one indicates whether the stream will ever produce new data in the future.
youenn fablet
Comment 23 2017-06-08 08:59:08 PDT
Comment on attachment 312300 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312300&action=review >> Source/WebCore/Modules/mediastream/MediaStream.cpp:521 >> + return true; > > If you return true here, shouldn't you override void ActiveDOMObject::suspend(ReasonForSuspension) ? Or does nothing need suspending? Default implementation of returning false seems good to me. >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:526 >>> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); >> >> Can you explain why having a listener counts as having activity? This seems odd. >> >> Normally, it is about something being active / playing or having pending events. > > I suspect this would cause leaks if there is any event listener registered. If we have listeners, the JS stream wrapper might get collected while the stream is still active and ready to fire events. Using setPendingActivity/unsetPendingActivity seems like overkill here. hasPendingActivity() should probably return false if m_isActive is false.
Chris Dumez
Comment 24 2017-06-08 09:01:54 PDT
Comment on attachment 312300 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312300&action=review >>> Source/WebCore/Modules/mediastream/MediaStream.cpp:521 >>> + return true; >> >> If you return true here, shouldn't you override void ActiveDOMObject::suspend(ReasonForSuspension) ? Or does nothing need suspending? > > Default implementation of returning false seems good to me. Please stop blindly returning false in those methods :( You will make PageCache a lot worse. At the very least, we should be able to suspend if we're not active (e.g. !hasPendingActivity() or !m_isActive).
Eric Carlson
Comment 25 2017-06-08 09:16:07 PDT
Comment on attachment 312300 [details] Proposed patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312300&action=review >>>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:526 >>>>> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); >>>> >>>> Can you explain why having a listener counts as having activity? This seems odd. >>>> >>>> Normally, it is about something being active / playing or having pending events. >>> >>> I suspect this would cause leaks if there is any event listener registered. >> >> I agree. I think this needs to check m_isActive or m_isProducingData -- whichever one indicates whether the stream will ever produce new data in the future. > > If we have listeners, the JS stream wrapper might get collected while the stream is still active and ready to fire events. > Using setPendingActivity/unsetPendingActivity seems like overkill here. > > hasPendingActivity() should probably return false if m_isActive is false. Ack, I uploaded the wrong patch file! The correct version has an early return when m_isActive is false.
Eric Carlson
Comment 26 2017-06-08 09:38:10 PDT
Created attachment 312313 [details] Updated patch.
Chris Dumez
Comment 27 2017-06-08 09:41:47 PDT
Comment on attachment 312313 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312313&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:529 > + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); This looks fragile. What If someone introduces a new event fired on this object. What if those are not the only 2 events? Can't we simply return m!_isActive? > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:360 > + return hasEventListeners(eventNames().muteEvent) || hasEventListeners(eventNames().unmuteEvent); ditto.
Eric Carlson
Comment 28 2017-06-08 10:12:31 PDT
Comment on attachment 312313 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312313&action=review >> Source/WebCore/Modules/mediastream/MediaStream.cpp:529 >> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); > > This looks fragile. What If someone introduces a new event fired on this object. What if those are not the only 2 events? Can't we simply return m!_isActive? This is the approach Geoff suggested. It don't agree that it is fragile, but I can change it if you insist.
Chris Dumez
Comment 29 2017-06-08 10:15:35 PDT
(In reply to Eric Carlson from comment #28) > Comment on attachment 312313 [details] > Updated patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=312313&action=review > > >> Source/WebCore/Modules/mediastream/MediaStream.cpp:529 > >> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); > > > > This looks fragile. What If someone introduces a new event fired on this object. What if those are not the only 2 events? Can't we simply return m!_isActive? > > This is the approach Geoff suggested. I'll defer to Geoff then. He knows this area better. All I see from Geoff on this bug is: "I agree. I think this needs to check m_isActive or m_isProducingData -- whichever one indicates whether the stream will ever produce new data in the future." Which is not what is implemented in the patch. > It don't agree that it is fragile, but I can change it if you insist. So he sometimes updates to fire the implementation to fire a new event at this object but fails to update hasPendingActivity(), it won't crash? :)
Chris Dumez
Comment 30 2017-06-08 10:20:08 PDT
Comment on attachment 312313 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312313&action=review >>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:529 >>>> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); >>> >>> This looks fragile. What If someone introduces a new event fired on this object. What if those are not the only 2 events? Can't we simply return m!_isActive? >> >> This is the approach Geoff suggested. >> >> It don't agree that it is fragile, but I can change it if you insist. > > I'll defer to Geoff then. He knows this area better. All I see from Geoff on this bug is: > "I agree. I think this needs to check m_isActive or m_isProducingData -- whichever one indicates whether the stream will ever produce new data in the future." > > Which is not what is implemented in the patch. Again, I'll defer to Geoff here but maybe something like "return !m_isActive || hasEventListeners();" would be an acceptable compromise?
Chris Dumez
Comment 31 2017-06-08 10:22:11 PDT
Comment on attachment 312313 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312313&action=review >>>>> Source/WebCore/Modules/mediastream/MediaStream.cpp:529 >>>>> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); >>>> >>>> This looks fragile. What If someone introduces a new event fired on this object. What if those are not the only 2 events? Can't we simply return m!_isActive? >>> >>> This is the approach Geoff suggested. >>> >>> It don't agree that it is fragile, but I can change it if you insist. >> >> I'll defer to Geoff then. He knows this area better. All I see from Geoff on this bug is: >> "I agree. I think this needs to check m_isActive or m_isProducingData -- whichever one indicates whether the stream will ever produce new data in the future." >> >> Which is not what is implemented in the patch. > > Again, I'll defer to Geoff here but maybe something like "return !m_isActive || hasEventListeners();" would be an acceptable compromise? Also what about addtrackEvent / removetrackEvent ? Why don't we need to handle those here?
Chris Dumez
Comment 32 2017-06-08 10:26:31 PDT
Comment on attachment 312313 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312313&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:526 > + if (!m_isActive) So if a MediaStream is not active, we keep the wrapper alive? This seems backwards. I have just noticed that after setting m_isActive to false, we queue an event to be dispatched asynchronously. As a result, we definitely need to keep the wrapper alive a little bit longer after m_isActive becomes false. As a result, I would suggest something like: bool MediaStream::hasPendingActivity() const { return m_isActive || !m_scheduledActivityEvents.isEmpty(); }
Chris Dumez
Comment 33 2017-06-08 10:32:56 PDT
Comment on attachment 312313 [details] Updated patch. View in context: https://bugs.webkit.org/attachment.cgi?id=312313&action=review >> Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:360 >> + return hasEventListeners(eventNames().muteEvent) || hasEventListeners(eventNames().unmuteEvent); > > ditto. What if the page has a listener for endedEvent (but no listener for mute/unmute events) ? It seems to me we allow the JS wrapper to be collected before the track has ended. Then later on, we'll crash when we try to dispatch the endedEvent, no?
Eric Carlson
Comment 34 2017-06-08 14:34:47 PDT
(In reply to Eric Carlson from comment #28) > Comment on attachment 312313 [details] > Updated patch. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=312313&action=review > > >> Source/WebCore/Modules/mediastream/MediaStream.cpp:529 > >> + return hasEventListeners(eventNames().activeEvent) || hasEventListeners(eventNames().inactiveEvent); > > > > This looks fragile. What If someone introduces a new event fired on this object. What if those are not the only 2 events? Can't we simply return m!_isActive? > > This is the approach Geoff suggested. > > It don't agree that it is fragile, but I can change it if you insist. You make good points - I concede that this is the wrong approach.
Eric Carlson
Comment 35 2017-06-08 14:44:31 PDT
Created attachment 312347 [details] Another patch
Chris Dumez
Comment 36 2017-06-08 15:37:31 PDT
Comment on attachment 312347 [details] Another patch View in context: https://bugs.webkit.org/attachment.cgi?id=312347&action=review > Source/WebCore/Modules/mediastream/MediaStream.cpp:526 > + return m_isActive || !m_scheduledActivityEvents.isEmpty(); Changes to MediaStream look good to me. > Source/WebCore/Modules/mediastream/MediaStreamTrack.cpp:309 > + m_pendingEvent = false; I am not convinced yet that we need this m_pendingEvent flag since the endedEvent is fired *synchronously* after setting the m_ended to true
Eric Carlson
Comment 37 2017-06-09 05:24:48 PDT
Created attachment 312429 [details] One more patch
Chris Dumez
Comment 38 2017-06-09 08:34:28 PDT
Comment on attachment 312429 [details] One more patch r=me
WebKit Commit Bot
Comment 39 2017-06-09 09:14:31 PDT
Comment on attachment 312429 [details] One more patch Clearing flags on attachment: 312429 Committed r217985: <http://trac.webkit.org/changeset/217985>
WebKit Commit Bot
Comment 40 2017-06-09 09:14:33 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.