WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
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
Show Obsolete
(10)
View All
Add attachment
proposed patch, testcase, etc.
Alexey Proskuryakov
Comment 1
2017-03-31 12:18:58 PDT
Marked as flaky in
http://trac.webkit.org/r214678
Radar WebKit Bug Importer
Comment 2
2017-03-31 12:19:16 PDT
<
rdar://problem/31376041
>
Per Arne Vollan
Comment 3
2017-05-17 01:24:26 PDT
Created
attachment 310357
[details]
Patch
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
Created
attachment 310380
[details]
Patch
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
Created
attachment 310427
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug