903270 - set thread name for initial event of NS_NewNamedThread()

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Karl Tomlinson (:karlt)]

Some clients are using NS_NewNamedThread() to start a thread, with a single event.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: patch

Comment 2

[Andrew McCreight [:mccr8]]

Comment on attachment 787952 [details] [diff] [review]
patch

I'm not an XPCOM peer, and jlebar is out for a few days, so bouncing this to bsmedberg.

Comment 3

[Doug Turner (:dougt)]

Comment on attachment 787952 [details] [diff] [review]
patch

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

Could you drop the last argument from the following places:

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#253
http://mxr.mozilla.org/mozilla-central/source/content/media/MediaDecoderStateMachine.cpp#1755

http://mxr.mozilla.org/mozilla-central/source/content/media/webrtc/MediaEngineWebRTC.h#114

::: xpcom/glue/nsThreadUtils.h
@@ +73,5 @@
> +  nsresult rv = NS_NewThread(result, nullptr, stackSize);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  NS_SetThreadName<LEN>(*result, name);
> +  if (initialEvent)
> +    rv = (*result)->Dispatch(initialEvent, NS_DISPATCH_NORMAL);

Did we use to just drop the initial event?  

http://mxr.mozilla.org/mozilla-central/source/content/media/MediaStreamGraph.cpp#1427

@@ +75,5 @@
> +  NS_SetThreadName<LEN>(*result, name);
> +  if (initialEvent)
> +    rv = (*result)->Dispatch(initialEvent, NS_DISPATCH_NORMAL);
> +
> +  return rv;

NS_WARN (or whatever) before the return?

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Doug Turner (:dougt) from comment #3)
> Did we use to just drop the initial event?  

It was passed to NS_NewThread() before.

> @@ +75,5 @@
> > +  NS_SetThreadName<LEN>(*result, name);
> > +  if (initialEvent)
> > +    rv = (*result)->Dispatch(initialEvent, NS_DISPATCH_NORMAL);
> > +
> > +  return rv;
> 
> NS_WARN (or whatever) before the return?

-  if (initialEvent)
+  if (initialEvent) {
     rv = (*result)->Dispatch(initialEvent, NS_DISPATCH_NORMAL);
+    NS_WARN_IF_FALSE(NS_SUCCEEDED(rv), "Initial event dispatch failed");
+  }

Comment 5

[Doug Turner (:dougt)]

+1

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: remove default null initial event parameter from NS_NewNamedThread() call site

Requested in comment 3.

Comment 7

[Karl Tomlinson (:karlt)]

(In reply to Doug Turner (:dougt) from comment #3)
> Could you drop the last argument from the following places:

http://hg.mozilla.org/mozilla-central/annotate/60c382ba1773/content/media/MediaDecoderStateMachine.cpp#l1755 has a non-default stack-size.

Looks like http://hg.mozilla.org/mozilla-central/annotate/60c382ba1773/content/media/MediaDecoderStateMachine.cpp#l253 had a non-default stack-size before
http://hg.mozilla.org/mozilla-central/rev/8eb1a29b4aa0
Bug 691096 comment 25 indicates that was intentional.

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: remove default null initial event parameter from NS_NewNamedThread() call site

Requested in comment 3.

Comment 9

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25742c3becd6
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b5af3c9e1b1

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/25742c3becd6
https://hg.mozilla.org/mozilla-central/rev/5b5af3c9e1b1

Comment 11

[Karl Tomlinson (:karlt)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e2bad66d274d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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