Convert NS_GetCurrentThread uses in dom/media

RESOLVED FIXED in Firefox 56

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

(Blocks 1 bug)

unspecified
mozilla56
Points:
---

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment)

Posted patch patchSplinter Review
This conversion makes us operate on event targets instead of threads. The advantage of event targets is that there is only one main thread event target in the Quantum DOM world, while there are potentially many main threads.

I'll send out an email about this change to dev-platform tomorrow. However, I want to get some patches posted so people can see what the change will look like.

There's some discussion of the API in bug 1361164 in case that helps.
Attachment #8868328 - Flags: review?(cpearce)
Comment on attachment 8868328 [details] [diff] [review]
patch

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

::: dom/media/VideoUtils.cpp
@@ +352,4 @@
>    if (aTarget) {
>      target = aTarget;
>    } else {
> +    target = GetMainThreadEventTarget();

In MediaManager you kept the checks for GetMainThreadEventTarget() failing; should you do that here too?

I don't think we could run this code after XPCOM shutdown has progressed past xpcom-shutdown-threads, but the prescence of "Get" in the name of GetMainThreadEventTarget() implies it can fail and that we should check for failure.

Should there be a "Get" in GetMainThreadEventTarget()?

::: dom/media/gmp/GMPContentParent.cpp
@@ +73,5 @@
>  
>  void
>  GMPContentParent::CheckThread()
>  {
> +  MOZ_ASSERT(mGMPEventTarget->IsOnCurrentThread());

For consistency, may as well use GMPEventTarget() instead of mGMPEventTarget.

::: dom/media/gmp/GMPParent.cpp
@@ +385,5 @@
>    // Note: GeckoMediaPluginService::GetThread() is threadsafe, and returns
>    // nullptr if the GeckoMediaPluginService has started shutdown.
>    nsCOMPtr<nsIThread> gmpThread;
>    mps->GetThread(getter_AddRefs(gmpThread));
> +  return gmpThread->EventTarget();

We need to null check gmpThread here, as per the comment starting line 385 above.
Attachment #8868328 - Flags: review?(cpearce) → review+
Component: Audio/Video → XPCOM

Comment 3

2 years ago
Pushed by wmccloskey@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/54f926563d9d
Convert NS_GetCurrentThread uses in dom/media (r=cpearce)

Comment 4

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/54f926563d9d
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
You need to log in before you can comment on or make changes to this bug.