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&)
Marked as flaky in http://trac.webkit.org/r214678
<rdar://problem/31376041>
Created attachment 310357 [details] Patch
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
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
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
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
Created attachment 310380 [details] Patch
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
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
Will this extend the lifetime forever, until the root object goes away?
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.
The track won't dispatch events if activeDOMObjectsAreSuspended or activeDOMObjectsAreStopped. We should figure out how the wrapper gets garbage collected on a active activeDOMObject.
Created attachment 310427 [details] Patch
(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; }
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".
This needs a bit of refinement and a test, I will take it.
Created attachment 312292 [details] Proposed patch.
Created attachment 312300 [details] Proposed patch.
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.
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.
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.
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.
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).
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.
Created attachment 312313 [details] Updated patch.
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.
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.
(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? :)
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?
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?
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(); }
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?
(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.
Created attachment 312347 [details] Another patch
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
Created attachment 312429 [details] One more patch
Comment on attachment 312429 [details] One more patch r=me
Comment on attachment 312429 [details] One more patch Clearing flags on attachment: 312429 Committed r217985: <http://trac.webkit.org/changeset/217985>
All reviewed patches have been landed. Closing bug.