1452410 - ThrottledEventQueue has unneeded shutdown handling

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Jim Blandy :jimb]

ThrottledEventQueue includes a number of behaviors around shutdown that seem to be unnecessary, and are implemented in a way that makes unit testing difficult.

ThrottledEventQueue registers itself as an observer of the "xpcom-shutdown" topic, and spins the main event loop until its own queue is empty. This should be unnecessary for any queue that feeds into the main thread's queue, as the XPCOM shutdown process already drains the event queue at several points after the "xpcom-shutdown" notification.

ThrottledEventQueue dispatches a separate runnable to remove itself as an observer. There's no reason this needs to happen in a separate runnable; it should simply remove itself directly.

I removed all the above code from ThrottledEventQueue, and the Try push was green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a2b9cdc4257292e62f0e3b467ad86522b1c49e91

Comment 1

[Jim Blandy :jimb]

Attachment: Simplify ThrottledEventQueue's shutdown behavior.

I'll request review once I have a good-looking try push for all platforms.

Comment 2

[Jim Blandy :jimb]

Full try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=f29df26c3dc17c7c7832f5402e4171a38fdb025c

Comment 3

[Jim Blandy :jimb]

Comment on attachment 8965988 [details] [diff] [review]
Simplify ThrottledEventQueue's shutdown behavior.

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

I've retriggered almost all the oranges and got greens. Nothing looks related.

Comment 4

[Nathan Froyd [:froydnj]]

Comment on attachment 8965988 [details] [diff] [review]
Simplify ThrottledEventQueue's shutdown behavior.

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

I am a little worried about this patch, as our testsuite probably doesn't stress our shutdown behavior very well.  OTOH, the current shutdown setup is odd, and I can't see why we shouldn't do this.  So r=me.

::: xpcom/threads/ThrottledEventQueue.cpp
@@ +43,4 @@
>  //
> +// The executor holds a strong reference to the Inner object. This means that if
> +// the outer object is dereferenced and destroyed, the Inner object will remain
> +// live for as long as the executor exists - that is, until its queue is empty.

Maybe say "the Inner's queue is empty" so it's more obvious which object we're referring to?

@@ -335,5 @@
>  
> -    // If we are shutting down, just fall back to our base target
> -    // directly.
> -    if (mShutdownStarted) {
> -      return mBaseTarget->Dispatch(Move(aEvent), aFlags);

Can you file a followup bug to fix a pre-existing problem: what happens if we don't have mExecutor, but creating and dispatching mExecutor fails?  We then have a passed in reference for aEvent that we have failed to do anything with.

I think the correct thing to do in that case *is* to leak the reference, but it'd be good to be explicit about it; already_AddRefed will complain in DEBUG builds if we ever hit this situation.

Comment 5

[Jim Blandy :jimb]

(In reply to Nathan Froyd [:froydnj] from comment #4)
> I am a little worried about this patch, as our testsuite probably doesn't
> stress our shutdown behavior very well.  OTOH, the current shutdown setup is
> odd, and I can't see why we shouldn't do this.  So r=me.

Yeah, I'm feeling a bit cowboy-ish pushing this, but:

- We have no documentation for why this behavior is necessary; no tests that catch the problem; and no institutional knowledge locked away in people's heads as far as I can tell.

- The status quo is interfering with my attempts to test ThrottledEventQueue's other behaviors, and has delayed my other work.

I think these are roughly the conditions under which it's justified to take it out and then watch for fallout.

> Maybe say "the Inner's queue is empty" so it's more obvious which object
> we're referring to?

Okay.

> Can you file a followup bug to fix a pre-existing problem: what happens if
> we don't have mExecutor, but creating and dispatching mExecutor fails?  We
> then have a passed in reference for aEvent that we have failed to do
> anything with.

Filed as bug 1453110.

Comment 6

[Jim Blandy :jimb]

Attachment: Simplify ThrottledEventQueue's shutdown behavior. r=froydnj

Updated per review. Carrying over r+.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/27d658560a63
Simplify ThrottledEventQueue's shutdown behavior. r=froydnj

Comment 8

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/27d658560a63