Bug 170355 - fast/mediastream/MediaStream-page-muted.html times out and asserts
Summary: fast/mediastream/MediaStream-page-muted.html times out and asserts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Media (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Eric Carlson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-03-31 12:15 PDT by Alexey Proskuryakov
Modified: 2017-06-09 09:14 PDT (History)
14 users (show)

See Also:


Attachments
Patch (2.13 KB, patch)
2017-05-17 01:24 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
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 Details
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 Details
Patch (2.13 KB, patch)
2017-05-17 04:40 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
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 Details
Patch (2.36 KB, patch)
2017-05-17 12:23 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Proposed patch. (13.78 KB, patch)
2017-06-08 04:50 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Proposed patch. (13.91 KB, patch)
2017-06-08 06:38 PDT, Eric Carlson
cdumez: review-
Details | Formatted Diff | Diff
Updated patch. (13.98 KB, patch)
2017-06-08 09:38 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
Another patch (15.17 KB, patch)
2017-06-08 14:44 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff
One more patch (13.87 KB, patch)
2017-06-09 05:24 PDT, Eric Carlson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Alexey Proskuryakov 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&)
Comment 1 Alexey Proskuryakov 2017-03-31 12:18:58 PDT
Marked as flaky in http://trac.webkit.org/r214678
Comment 2 Radar WebKit Bug Importer 2017-03-31 12:19:16 PDT
<rdar://problem/31376041>
Comment 3 Per Arne Vollan 2017-05-17 01:24:26 PDT
Created attachment 310357 [details]
Patch
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Per Arne Vollan 2017-05-17 04:40:20 PDT
Created attachment 310380 [details]
Patch
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Alexey Proskuryakov 2017-05-17 09:56:09 PDT
Will this extend the lifetime forever, until the root object goes away?
Comment 12 Eric Carlson 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.
Comment 13 Eric Carlson 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.
Comment 14 Per Arne Vollan 2017-05-17 12:23:40 PDT
Created attachment 310427 [details]
Patch
Comment 15 Per Arne Vollan 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;
}
Comment 16 Darin Adler 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".
Comment 17 Eric Carlson 2017-06-06 14:21:52 PDT
This needs a bit of refinement and a test, I will take it.
Comment 18 Eric Carlson 2017-06-08 04:50:32 PDT
Created attachment 312292 [details]
Proposed patch.
Comment 19 Eric Carlson 2017-06-08 06:38:11 PDT
Created attachment 312300 [details]
Proposed patch.
Comment 20 Chris Dumez 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.
Comment 21 Chris Dumez 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.
Comment 22 Geoffrey Garen 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.
Comment 23 youenn fablet 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.
Comment 24 Chris Dumez 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).
Comment 25 Eric Carlson 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.
Comment 26 Eric Carlson 2017-06-08 09:38:10 PDT
Created attachment 312313 [details]
Updated patch.
Comment 27 Chris Dumez 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.
Comment 28 Eric Carlson 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.
Comment 29 Chris Dumez 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? :)
Comment 30 Chris Dumez 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?
Comment 31 Chris Dumez 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?
Comment 32 Chris Dumez 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();
}
Comment 33 Chris Dumez 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?
Comment 34 Eric Carlson 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.
Comment 35 Eric Carlson 2017-06-08 14:44:31 PDT
Created attachment 312347 [details]
Another patch
Comment 36 Chris Dumez 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
Comment 37 Eric Carlson 2017-06-09 05:24:48 PDT
Created attachment 312429 [details]
One more patch
Comment 38 Chris Dumez 2017-06-09 08:34:28 PDT
Comment on attachment 312429 [details]
One more patch

r=me
Comment 39 WebKit Commit Bot 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>
Comment 40 WebKit Commit Bot 2017-06-09 09:14:33 PDT
All reviewed patches have been landed.  Closing bug.