Closed Bug 1190496 Opened 4 years ago Closed 4 years ago

Hoist SharedThreadPool into XPCOM

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox42 --- fixed

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(7 files)

No description provided.
This happens after xpcom-shutdown, and is the notification we're supposed to
listen for to shutdown off-main-thread event queues.
Attachment #8642656 - Flags: review?(cpearce)
Attachment #8642652 - Flags: review?(cpearce) → review+
Attachment #8642653 - Flags: review?(cpearce) → review+
Attachment #8642654 - Flags: review?(cpearce) → review+
Attachment #8642655 - Flags: review?(cpearce) → review+
Comment on attachment 8642656 [details] [diff] [review]
Part 5 - Do shutdown on xpcom-shutdown-threads. v1

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

Was there a failure you were hitting that prompted you to make this change, or are you just making this change to ensure we're doing things the way they're supposed to be done?
Attachment #8642656 - Flags: review?(cpearce) → review+
(In reply to Chris Pearce (:cpearce) from comment #9)
> Comment on attachment 8642656 [details] [diff] [review]
> Part 5 - Do shutdown on xpcom-shutdown-threads. v1
> 
> Review of attachment 8642656 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Was there a failure you were hitting that prompted you to make this change,
> or are you just making this change to ensure we're doing things the way
> they're supposed to be done?

I'm doing it because it doesn't make sense for an XPCOM thing to depend on the MediaShutdownManager (or nsContentUtils) to shut itself down. And if we just listen for xpcom-shutdown (like MediaShutdownManager does), we have no guarantee that MediaShutdownManager runs before SpinUntilEmpty, the current behavior we need to preserve.
Attachment #8642657 - Flags: review?(nfroyd) → review+
Comment on attachment 8642658 [details] [diff] [review]
Part 7 - Hoist SharedThreadPool into xpcom. v1

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

r=me with a couple of nits that should be addressed in followup bugs.

SharedThreadPool::Get should have a main thread assertion to back up the comment in SharedThreadPool.h.

Now that we have nsThreadPool and SharedThreadPool, we should have some documentation somewhere (headers, preferably) describing why you might prefer one or the other.

EnsureThreadLimitIsAtLeast should probably ditch the reference to "thread that we use for media".

I guess SharedThreadPool::AddRef/Release can't be done via the standard XPCOM macros, even with threadsafe ones, since they require locking?  That's a bummer.  I suggest swapping the stabilization of the refcount with the dispatch of the event to the main thread in Release, though, and using a NonOwning runnable method, to make things a little less confusing...
Attachment #8642658 - Flags: review?(nfroyd) → review+
Blocks: 1191063
You need to log in before you can comment on or make changes to this bug.