Closed Bug 1265965 Opened 9 years ago Closed 9 years ago

Clean up nsThread creation

Categories

(Core :: XPCOM, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox48 --- affected

People

(Reporter: n.nethercote, Assigned: n.nethercote)

Details

Attachments

(1 file, 2 obsolete files)

nsThread creation can be made nicer.
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: nobody → n.nethercote
Status: NEW → ASSIGNED
Attached patch (part 2) - Move ~nsThread() (obsolete) — Splinter Review
This moves the destructor after all the constructors. No code has been changed.
Attachment #8743132 - Flags: review?(nfroyd)
Attachment #8743132 - Flags: review?(nfroyd) → review+
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)
Attachment #8743131 - Attachment is obsolete: true
Attachment #8743131 - Flags: review?(nfroyd)
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+
This isn't worth pursuing.
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: