Clean up nsThread creation

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
3 years ago
3 years ago

People

(Reporter: njn, Assigned: njn)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(firefox48 affected)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
nsThread creation can be made nicer.
(Assignee)

Comment 1

3 years ago
Created attachment 8743131 [details] [diff] [review]
(part 1) - Refactor nsThread creation

nsThread has a constructor and two init functions with quite different
purposes: Init() (which creates a new thread) and InitCurrentThread() (which
wraps an existing thread).

This patch replaces this trio with two new nsThread constructors, both of which
call a common (non-public) delegate constructor. This has the following
benefits.

- It allows distinct argument lists for the two cases.

- It makes it impossible to half-initialize an nsThread.

- It means nsThread::mStackSize can be removed; it was only needed to save
  state between the constructor and the Init() method.

The patch also removes the return value (and corresponding checking at call
sites) from the "wrap the current thread" case, because it's infallible.
Attachment #8743131 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8743132 [details] [diff] [review]
(part 2) - Move ~nsThread()

This moves the destructor after all the constructors. No code has been changed.
Attachment #8743132 - Flags: review?(nfroyd)
Attachment #8743132 - Flags: review?(nfroyd) → review+
(Assignee)

Comment 3

3 years ago
Created attachment 8744125 [details] [diff] [review]
Refactor nsThread creation

From the dev-platform thread it's clear that everyone loves factory methods, so
I've rewritten this patch to use them. There are now two |bool& aOk| outparams
in constructors, but they're in non-public constructors so hopefully that won't
be controversial.
Attachment #8744125 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8743131 - Attachment is obsolete: true
Attachment #8743131 - Flags: review?(nfroyd)
(Assignee)

Updated

3 years ago
Attachment #8743132 - Attachment is obsolete: true
Comment on attachment 8744125 [details] [diff] [review]
Refactor nsThread creation

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

I am not completely convinced about the safety of this patch, or my proposed modifications.

I confess to a partiality for factory methods, but I also see from this that they don't work all that well with derived classes.

::: xpcom/threads/nsThread.cpp
@@ +519,5 @@
>  {
>    // spawn thread and wait until it is fully setup
>    RefPtr<nsThreadStartupEvent> startup = new nsThreadStartupEvent();
>  
>    NS_ADDREF_THIS();

Please comment that this is for the reference that is held over the life of ThreadFunc.  But I think this needs to move *after* the call to PR_CreateThread, see below.

@@ +528,2 @@
>    if (!thr) {
>      NS_RELEASE_THIS();

OK, so in the unlikely even that PR_CreateThread fails, we're going to release a ref to |this|.  Doing this Release worked before, because whoever called |Init| was (ideally) already holding a ref.  But doing a Release in the constructor means that nobody else is holding a ref, just us.  So this Release() will actually call the destructor, and when the constructor finishes, whoever called the constructor is going to be holding onto freed memory, and that's not going to end well.

ThreadFunc normally does the matching Release to the AddRef we did above.  But to make this all work in our new world, I think we have to do:

  PRThread* thr = ...;

  // Check |thr|, return if nullptr.

  // ThreadFunc is not yet running.  The ref here will be balanced by ThreadFunc
  // Release'ing when it exits.
  NS_ADDREF_THIS();

  // Insert initial event into the queue.

  // Wait for initialization to complete.

  aOk = true;  // See how nicely the |a| prefixing works here. ;)

Hm, can we ever be in a scenario where:

T1 Call nsThread::nsThread
T1 PR_NewThread succeeds, we AddRef in the constructor
T1 Dispatch the startup event, wait.
T2 Run the entirety of ThreadFunc, including the matching Release to our AddRef
T1 Return from the constructor with newly-freed memory from the Release.

?  Doesn't seem entirely out of the realm of possibility, and that'd be an ugly one to debug.  Maybe to be safe, we should move the dispatch of the initial event and the wait into the factory creation function, so our factory creation function guarantees holding the thread alive while we do all this?  That doesn't play very nicely with WorkerThread, though...

::: xpcom/threads/nsThread.h
@@ +47,2 @@
>  
> +  // Create this as a wrapper for the current PRThread. Infallible.

It's too bad we have to return a can-be-null thing for an infallible call.
Attachment #8744125 - Flags: review?(nfroyd) → feedback+
(Assignee)

Comment 5

3 years ago
This isn't worth pursuing.
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.