1330212 - Intermittent dom/media/tests/mochitest/test_getUserMedia_mediaStreamTrackClone.html | application crashed [@ mozilla::CycleCollectedJSContext::ProcessMetastableStateQueue]

Status: RESOLVED FIXED

(Core :: Audio/Video: MediaStreamGraph, defect, P1)

Description

[Treeherder Bug Filer]

Filed by: philringnalda [at] gmail.com

https://treeherder.mozilla.org/logviewer.html#?job_id=67852903&repo=mozilla-inbound

https://archive.mozilla.org/pub/mobile/tinderbox-builds/mozilla-inbound-android-api-15/1484103726/mozilla-inbound_ubuntu64_vm_armv7_large_test-mochitest-media-2-bm51-tests1-linux64-build106.txt.gz

Comment 1

[Paul Adenot (:padenot)]

Andreas, it says "clone" in the title, any idea of why it happens or a possible dupe ? :-)

Comment 2

[Andreas Pehrson [:pehrsons]]

I think the "clone" in the title is a huge coincidence, but I know that the assert here is the same as in bug 1317501. But it looks like it's from a new callsite. MSG runnables run on main thread in stable state and at that point there are certain things they're not allowed to do.

To my untrained eye just looking at the stack, it looks like we're not allowed to destroy the SystemClockDriver directly in the MSG runnable because that will process its thread's queue, and there's some event that is not allowed to run in stable state.

smaug knows more here maybe?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Yeah, totally not acceptable to spin eventloop at that point (nsIThread::Shutdown may spin eventloop).
Call shutdown() using a runnable?

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

Attachment: Release GraphDrivers outside of StableState runnable to avoid spinning the event queue

How about this?

Comment 5

[Randell Jesup [:jesup] (needinfo me)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f610f6fd0da9f4576641b64b2d1a18688222ba0f

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8826057 [details] [diff] [review]
Release GraphDrivers outside of StableState runnable to avoid spinning the event queue

Is it guaranteed same issue can't happen in other use of SystemClockDriver?

Comment 7

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Olli Pettay [:smaug] from comment #6)
> Comment on attachment 8826057 [details] [diff] [review]
> Release GraphDrivers outside of StableState runnable to avoid spinning the
> event queue
> 
> Is it guaranteed same issue can't happen in other use of SystemClockDriver?

the issue only arises when we're switching drivers in RunInStableState; other switches happen in various Media threads or in non-StableState MainThread runnables.

Smaug: is there any way we can add assertions or warnings to event-loop-spinning in states we don't want it in, even if only for Debug builds, or even if it requires whitelisting a number of known-ok uses?  This is a usually subtle landmine that's tripped many an engineer over the years - and as with this case, the event-loop-spinning is often not obvious to the person who caused it (for example, it may depend on who's holding the last ref to an object such as a thread, like this one).

Comment 8

[Randell Jesup [:jesup] (needinfo me)]

See previous comment

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

So we would need to add some annotations on stack when spinning event loop isn't ok and then assert in thread code if someone is trying to spin the loop.
That might be useful. naAutoScriptBlocker could inherit from such "annotation class", so that we'd cover most common cases easily, and then add some more annotations for metastable and stable states.

I guess we could start from main thread only.

This might not cover all the cases, for that some rather heavy static analysis would be needed, but possibly would be useful anyhow.

Please file a bug and I'll try to find time to look at it if no one else will.
This might be useful for Quantum DOM too.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c90ec782d481
Release GraphDrivers outside of StableState runnable to avoid spinning the event queue r=smaug,padenot

Comment 11

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c90ec782d481

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Sounds like this affects 51/52 as well? Please nominate it for uplift.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

This was caused by bug 1228226, which has been backed out of 53 already (and likely will soon be backed out of 52).  The fix in this bug is still good to have in the tree; it's a good safety backstop against someone causing this problem again (until we have better checking via bug 1331036).  If we don't back the other out of 52, and this is still needed there, we can uplift it to 52.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

Fixed on 52 by the backout of bug 1228226.

Comment 15

[Intermittent Failures Robot]

1 failures in 892 pushes (0.001 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* try: 1

Platform breakdown:
* android-4-3-armv7-api15: 1

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1330212&startday=2017-06-19&endday=2017-06-25&tree=all