1242268 - Intermittent test_WebAudioMemoryReporting.html | Non-zero usage for explicit/webaudio/audio-node/ScriptProcessorNode/dom-nodes

Status: RESOLVED FIXED

(Core :: Web Audio, defect, P2)

Description

[Phil Ringnalda (:philor)]

+++ This bug was initially created as a clone of Bug #1224022 +++

https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=19459958
https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=6508646
https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=19566139
https://treeherder.mozilla.org/logviewer.html#?repo=b2g-inbound&job_id=3739444
https://treeherder.mozilla.org/logviewer.html#?repo=b2g-inbound&job_id=3740628
https://treeherder.mozilla.org/logviewer.html#?repo=fx-team&job_id=6591889
https://treeherder.mozilla.org/logviewer.html#?repo=b2g-inbound&job_id=3765839
https://treeherder.mozilla.org/logviewer.html#?repo=mozilla-inbound&job_id=19984081
https://treeherder.mozilla.org/logviewer.html#?job_id=20396967&repo=mozilla-inbound

Comment 1

[Karl Tomlinson (:karlt)]

https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1242268&entireHistory=true&tree=trunk
says this was a short term problem, occurring on revisions from bbfbd1e73bf1
to 31373f8aaa98, mochitest-3 and mochitest-e10s-3, Win 7, opt and pgo.

However, this has shown up again on Ubuntu 16.04 in
https://bugzilla.mozilla.org/show_bug.cgi?id=1281179#c8

Comment 2

[Karl Tomlinson (:karlt)]

I suspect the Linux failures at least are either due to timeout during wait or the use of wait without checking the condition.

http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/media/MediaStreamGraph.cpp#3474

man 3p pthread_cond_timedwait:

"When using condition variables there is always a Boolean predicate involv‐
ing shared variables associated with each condition wait that is  true  if
the  thread  should proceed. Spurious wakeups from the pthread_cond_timed‐
wait() or pthread_cond_wait() functions may occur. Since the  return  from
pthread_cond_timedwait()  or  pthread_cond_wait()  does not imply anything
about the value of this predicate, the predicate  should  be  re-evaluated
upon such return."

https://hg.mozilla.org/try/rev/00cb64ed7e3a82c449ce049cfe65fbfa25702239
may be a good enough bandaid if this is holding back bug 1281179, but I'll look at using RegisterWeakAsyncMemoryReporter() to do this properly.

Comment 3

[Karl Tomlinson (:karlt)]

Attachment: bug 1242268 document mLifecycleState multithread access management

Review commit: https://reviewboard.mozilla.org/r/62076/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62076/

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: bug 1242268 don't try to wake up an empty MSG that is shutting down to report memory

Review commit: https://reviewboard.mozilla.org/r/62078/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62078/

Comment 5

[Karl Tomlinson (:karlt)]

Attachment: bug 1242268 use pointers instead of copies of string literals in AudioNode memory reporting

Review commit: https://reviewboard.mozilla.org/r/62080/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62080/

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: bug 1242268 store a node type string pointer on engines and use that for memory reporting

removing some off-main-thread usage of AudioNodeEngine::mNode.

Review commit: https://reviewboard.mozilla.org/r/62082/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62082/

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: bug 1242268 collect memory reports of AudioNode dom objects on the main thread

This will permit allowing the main thread to run while collecting
reports from graph thread objects.

Review commit: https://reviewboard.mozilla.org/r/62084/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62084/

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: bug 1242268 record MSG memory sizes asynchronously

This removes the one second timeout for MSG collection, extending the timeout
period to the 50 second timeout of nsMemoryReporterManager.

Also removed:

* The condition variable logic that can stop waiting without checking the
  condition set when memory reports are complete.

* Races with mAudioStreamSizes modification on two threads after wait timeout.

Memory from streams in offline graphs that are not yet running is now also
included.

Review commit: https://reviewboard.mozilla.org/r/62086/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/62086/

Comment 9

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (ni?:karlt) from comment #1)
> However, this has shown up again on Ubuntu 16.04 in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1281179#c8

These patches address this:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6afe6747bb1

Another try run here.  The many linux failures are due to using a different vm.
Other platform results are good.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0c63c4b16c8c

Comment 10

[Paul Adenot (:padenot)]

Comment on attachment 8767610 [details]
bug 1242268 document mLifecycleState multithread access management

https://reviewboard.mozilla.org/r/62076/#review58800

Comment 11

[Paul Adenot (:padenot)]

Comment on attachment 8767611 [details]
bug 1242268 don't try to wake up an empty MSG that is shutting down to report memory

https://reviewboard.mozilla.org/r/62078/#review58802

Comment 12

[Paul Adenot (:padenot)]

Comment on attachment 8767612 [details]
bug 1242268 use pointers instead of copies of string literals in AudioNode memory reporting

https://reviewboard.mozilla.org/r/62080/#review58804

Comment 13

[Paul Adenot (:padenot)]

Comment on attachment 8767613 [details]
bug 1242268 store a node type string pointer on engines and use that for memory reporting

https://reviewboard.mozilla.org/r/62082/#review58806

Comment 14

[Paul Adenot (:padenot)]

Comment on attachment 8767614 [details]
bug 1242268 collect memory reports of AudioNode dom objects on the main thread

https://reviewboard.mozilla.org/r/62084/#review58808

Comment 15

[Paul Adenot (:padenot)]

Comment on attachment 8767615 [details]
bug 1242268 record MSG memory sizes asynchronously

https://reviewboard.mozilla.org/r/62086/#review58810

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

is there anything blocking this from landing?

Comment 17

[Paul Adenot (:padenot)]

Karl is on PTO, and I don't know why he didn't land that earlier, but he must have had a good reason. I don't know much, sorry.

Comment 18

[Joel Maher ( :jmaher ) (UTC -8)]

another week has passed, possibly :karlt is back and caught up- would like to figure out what remains here and get this landed :)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0c21f0c5a296
document mLifecycleState multithread access management r=padenot
https://hg.mozilla.org/integration/autoland/rev/20c9343e899c
don't try to wake up an empty MSG that is shutting down to report memory r=padenot
https://hg.mozilla.org/integration/autoland/rev/3640f261f9e0
use pointers instead of copies of string literals in AudioNode memory reporting r=padenot
https://hg.mozilla.org/integration/autoland/rev/86bf660ce796
store a node type string pointer on engines and use that for memory reporting r=padenot
https://hg.mozilla.org/integration/autoland/rev/f66344471c52
collect memory reports of AudioNode dom objects on the main thread r=padenot
https://hg.mozilla.org/integration/autoland/rev/a85f3a50fd5f
record MSG memory sizes asynchronously r=padenot

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0c21f0c5a296
https://hg.mozilla.org/mozilla-central/rev/20c9343e899c
https://hg.mozilla.org/mozilla-central/rev/3640f261f9e0
https://hg.mozilla.org/mozilla-central/rev/86bf660ce796
https://hg.mozilla.org/mozilla-central/rev/f66344471c52
https://hg.mozilla.org/mozilla-central/rev/a85f3a50fd5f