1184807 - "ASSERTION: Failed NS_DispatchToMainThread() in shutdown" in CreateAndDispatchTrackRunner()

Status: RESOLVED FIXED

(Core :: Audio/Video, defect)

Description

[Jesse Ruderman]

Attachment: testcase

1. Load the testcase
2. Quit immediately

###!!! ASSERTION: Failed NS_DispatchToMainThread() in shutdown; leaking: 'false', file xpcom/glue/nsThreadUtils.cpp, line 171

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Andrew McCreight [:mccr8]]

This is a "regression" from bug 1155059.

I also get this assertion just from running the Service Workers mochitest locally:

 ./mach mochitest -f plain dom/workers/test/serviceworkers/

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

> I also get this assertion just from running the Service Workers mochitest
> locally:
> 
>  ./mach mochitest -f plain dom/workers/test/serviceworkers/

Sounds like the fix for bug 1174381 isn't complete, and the shutdown is still happening late - when it won't actually work.

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

FYI, the stack shows this getting used in the final CC pass, which is after shutdown-threads (and after DispatchToMainThread stops working.

It might greatly ease things if disabling DispatchToMainThread (and GetMainThread()) happened *after* final-CC (with a pass to process the event queue after CC).  Or look at (roughly):
  CC();
  if (events_queued && !timeout) {
    ProcessEvents(); // those queued by the CC pass
    CC();
  }

Comment 5

[Nathan Froyd [:froydnj]]

I am unclear to the actual question you're asking here.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Nathan Froyd [:froydnj][:nfroyd] from comment #5)
> I am unclear to the actual question you're asking here.

To be clear, I meant to ask your (and bsmedberg's) thoughts on continuing to process events (at least in a limited way) on MainThread through the final CC pass (comment 4), instead of starting to fail them *before* final CC (as we do today).  This causes more than a bit of hoop-jumping (though in some cases switching to DispatchToCurrentThread() will help - not because it will succeed, since it will also fail, but because it will free instead of leak (though that's not the same as calling Run()).

It may be worth opening a separate bug on this (even if we decide not to do it)

Comment 7

[Benjamin Smedberg]

I would be very unhappy if we continued processing events at that point. Everything which can send events should be observing earlier shutdown notifications and doing proper shutdown instead of relying on CC.

Comment 8

[Nathan Froyd [:froydnj]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> I would be very unhappy if we continued processing events at that point.
> Everything which can send events should be observing earlier shutdown
> notifications and doing proper shutdown instead of relying on CC.

Yup.

Comment 9

[Randell Jesup [:jesup] (needinfo me)]

Ok, then we have a pile of bugs to file like this one. :-)  Thanks

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

-> baku

This is failing in dom::TextTrackList::RemoveTextTrack(), which calls CreateAndDispatchTrackRunner().  Sounds like these need to get yanked earlier, or neutered in some manner, since whatever they're trying to do in the final CC pass won't work per bsmedberg/froyd.

Comment 11

[Jesse Ruderman]

Should some objects assert that they aren't destroyed during final CC? (Would that have helped us discover this bug earlier?)

Comment 12

[Andrew McCreight [:mccr8]]

There's no particular problem with destroying these things late, except for the dispatch not working.

Comment 13

[Randell Jesup [:jesup] (needinfo me)]

(In reply to Andrew McCreight [:mccr8] from comment #12)
> There's no particular problem with destroying these things late, except for
> the dispatch not working.

That depends on what the Dispatch is meant to do.  In many cases, this will mean a leak (which is ok, but will cause automation to complain).  In other cases, it might lead to unexpected surprises (hard to say exactly what), but usually they'll be benign.  I'm not 100% sure they'll always be so, but it's certainly possible, perhaps even likely.

Comment 14

[Andrea Marchesini [:baku]]

mccr8, this seems a bigger problem than something just related to TextTrack. Wondering if we can provide a more generic solution.

Why do we assert there instead just returning an error code?
I don't think we want to add xpcom-shutdown observers everywhere we call DispatchToMainThread.

Comment 15

[Randell Jesup [:jesup] (needinfo me)]

We assert, because generally this is a signal of a design or logic flaw.  If you *need* a soft failure on DispatchToMainThread, you can get MainThread and only Dispatch if MainThread is available (which is what happens internally in DispatchToMainThread if you aren't DEBUG).  But then it's on you to be sure that this is safe and the right thing to do (release on the current thread perhaps, or leak if the item that was being sent was mainthread-only-refcounting/etc), and it's on you to know what the impact of not doing it is and be ok with that.

It was made to assert because people were thinking they were doing stuff when they weren't; stuff wasn't getting freed, things weren't getting properly shut down, etc.

See also comment 7 by bsmedberg and comment 8 by Nathan

Comment 16

[Andrea Marchesini [:baku]]

Attachment: track.patch

Comment 17

[Andrea Marchesini [:baku]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44034f41ef1c

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8650398 [details] [diff] [review]
track.patch

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

It's possible (though quite unlikely) this can still fail, since holding a ref to MainThread does not stop MainThread from entering later shutdown and turning off DispatchToMainThread() (and asserting).  If you're going to use this trick, you should use thread->Dispatch(eventRunner.forget()) (or even better, thread->Dispatch(do_AddRef(new TrackEventRunner(...))) instead of a local nsCOMPtr<>).  Dispatch() will free the passed-in ref on (very unlikely) failure (mostly if the target thread has entered final shutdown and is no longer processing events).

Also note (though this may be ok) that whatever action was requested in the runnable will not happen if Dispatch fails, so if you're doing this to avoid the assertion, you should probably either comment that this is ok, and/or throw a warning that something didn't happen.

Comment 19

[Andrea Marchesini [:baku]]

Attachment: track2.patch

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8650543 [details] [diff] [review]
track2.patch

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

Add a comment that this can happen during final CC, and that it's ok if it fails

Comment 21

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f0f0d55bd55

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/44034f41ef1c
https://hg.mozilla.org/mozilla-central/rev/5f0f0d55bd55