Bug 170355 – fast/mediastream/MediaStream-page-muted.html times out and asserts

• Component: Media
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

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&)

[Alexey Proskuryakov]

Marked as flaky in http://trac.webkit.org/r214678

[Radar WebKit Bug Importer]

<rdar://problem/31376041>

[Per Arne Vollan]

Created attachment 310357 [details]
Patch

[Build Bot]

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]

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 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]

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]

Created attachment 310380 [details]
Patch

[Build Bot]

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]

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]

Will this extend the lifetime forever, until the root object goes away?

[Eric Carlson]

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]

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]

Created attachment 310427 [details]
Patch

[Per Arne Vollan]

(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 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]

This needs a bit of refinement and a test, I will take it.

[Eric Carlson]

Created attachment 312292 [details]
Proposed patch.

[Eric Carlson]

Created attachment 312300 [details]
Proposed patch.

[Chris Dumez]

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 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 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 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 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 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]

Created attachment 312313 [details]
Updated patch.

[Chris Dumez]

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 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]

(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 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 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 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 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]

(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]

Created attachment 312347 [details]
Another patch

[Chris Dumez]

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]

Created attachment 312429 [details]
One more patch

[Chris Dumez]

Comment on attachment 312429 [details]
One more patch

r=me

[WebKit Commit Bot]

Comment on attachment 312429 [details]
One more patch

Clearing flags on attachment: 312429

Committed r217985: <http://trac.webkit.org/changeset/217985>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.