1160064 - Give WatchManager an owner thread and manage lifetimes better

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Bobby Holley (:bholley)]

Followup from bug 1159558.

Comment 1

[Bobby Holley (:bholley)]

Attachment: Part 1 - Initialize the state machine task queue in the initializer list. v1

Comment 2

[Bobby Holley (:bholley)]

Attachment: Part 2 - Switch mirror/canonical initialization to happen in the constructor. v1

The goal here is to hoist all meaningful watcher/mirror/canonical manipulation
onto the owner thread. But since that must necessarily happen asynchronously,
we need to make sure that canonicals are in a sane state immediately upon
creation, since otherwise a mirror from another thread may attempt to connect to
a not-yet-initialized canonical.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 3 - Do watching/mirroring initialization on the state machine task queue. v1

We take this as an opportunity to remove connect-during-initialization. Always
connecting from the owner thread feels like a stronger invariant.

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 4 - Give WatchManager an owner thread and make sure everything happens there. v1

Comment 5

[Bobby Holley (:bholley)]

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

Comment 6

[Bobby Holley (:bholley)]

Attachment: Part 0 - Explicitly initialize logging at the right time. v1

The patches in this bug can cause us to log things before some of the Ensure*Log
macros are called. Let's just do this right.

Comment 7

[Bobby Holley (:bholley)]

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

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8600180 [details] [diff] [review]
Part 4 - Give WatchManager an owner thread and make sure everything happens there. v1

Review of attachment 8600180 [details] [diff] [review]:
-----------------------------------------------------------------

Good!

Comment 9

[Bobby Holley (:bholley)]

(In reply to Bobby Holley (:bholley) from comment #7)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=904b8ab90fd4

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

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/65c1a77ed87c
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/931d1e1603f4
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/fe579e65165e
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6979a9e3e60a
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/924a8ed0b89c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/65c1a77ed87c
https://hg.mozilla.org/mozilla-central/rev/931d1e1603f4
https://hg.mozilla.org/mozilla-central/rev/fe579e65165e
https://hg.mozilla.org/mozilla-central/rev/6979a9e3e60a
https://hg.mozilla.org/mozilla-central/rev/924a8ed0b89c

Comment 11

[Nathan Toone [:toonetown]]

This breaks building when PR_LOGGING is not set due to line 130:
  gMediaDecoderLog = PR_NewLogModule("MediaDecoder");


Suggest wrapping that line with:
#ifdef PR_LOGGING
  gMediaDecoderLog = PR_NewLogModule("MediaDecoder");
#endif

since the extern is only defined when PR_LOGGING is defined.

Comment 12

[Nathan Toone [:toonetown]]

I have created a new bug to track the build breakage: bug 1161186