Closed Bug 1177282 Opened 6 years ago Closed 6 years ago

Clean up SharedThreadPool

Categories

(Core :: Audio/Video, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(3 files)

Bug 1175768 got backed out for some crashes that look like memory corruption in SharedThreadPool. Some of the stuff we do for creation/destruction of the hashtables looks sketchy to me, so I'm going to clean it up and see if it fixes the crashes.
Well, looks green, but it didn't solve my problem. Probably worth getting into the tree anyway though.

No hurry reviewing this.
Attachment #8626046 - Flags: review?(cpearce) → review+
Attachment #8626047 - Flags: review?(cpearce) → review+
Comment on attachment 8626047 [details] [diff] [review]
Part 2 - Clean up SharedThreadPool::Release. v1

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

::: dom/media/SharedThreadPool.cpp
@@ +120,5 @@
> +  // the nsIThreadPool. The Runnable here  will add a refcount to the pool,
> +  // and when the Runnable releases the nsIThreadPool it will be deleted.
> +  nsCOMPtr<nsIRunnable> r = NS_NewRunnableMethod(mPool, &nsIThreadPool::Shutdown);
> +  mPool = nullptr;
> +  mEventTarget = nullptr;

Destructor will effectively nullify mPool and mEventTarget. We don't need to reset these members here, right?
Comment on attachment 8626046 [details] [diff] [review]
Part 1 - Create and destroy static SharedThreadPool state at startup/shutdown. v1

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

::: dom/media/SharedThreadPool.cpp
@@ +33,3 @@
>  {
>    MOZ_ASSERT(NS_IsMainThread());
> +  MOZ_ASSERT(!sMonitor && !sPools);

With InitStatics(), we can now remove the restriction of only creating SharedThreadPool on the main thread which I've been thinking about.
(In reply to JW Wang [:jwwang] from comment #5)
> Destructor will effectively nullify mPool and mEventTarget. We don't need to
> reset these members here, right?

I just wanted to make sure that they would be released predictably on the target thread. When trying to interpret crashes, the "dispatch+destroy" pattern can often make it difficult to figure out what's going on, since members may be destroyed in lexical scope _or_ in the dispatched method, depending on the order of execution.

Note that the current code doesn't actually guarantee that, because nsIEventTarget::Dispatch doesn't take already_AddRefed yet. But it will, soon, hopefully.
Attachment #8626909 - Flags: review?(jwwang) → review+
(In reply to Bobby Holley (:bholley) from comment #7)
> I just wanted to make sure that they would be released predictably on the
> target thread. When trying to interpret crashes, the "dispatch+destroy"
> pattern can often make it difficult to figure out what's going on, since
> members may be destroyed in lexical scope _or_ in the dispatched method,
> depending on the order of execution.

That's the merit of tail dispatch which makes things more predicable.

|mPool = nullptr; AbstractThread::MainThread()->Dispatch(r.forget());| and
|AbstractThread::MainThread()->Dispatch(r.forget()); mPool = nullptr;| should work effectively the same.
(In reply to JW Wang [:jwwang] from comment #9)
> (In reply to Bobby Holley (:bholley) from comment #7)
> > I just wanted to make sure that they would be released predictably on the
> > target thread. When trying to interpret crashes, the "dispatch+destroy"
> > pattern can often make it difficult to figure out what's going on, since
> > members may be destroyed in lexical scope _or_ in the dispatched method,
> > depending on the order of execution.
> 
> That's the merit of tail dispatch which makes things more predicable.

good point!
You need to log in before you can comment on or make changes to this bug.