Closed Bug 1213050 Opened 9 years ago Closed 8 years ago

Intermittent test_zmedia_cleanup.html | application crashed [@ mozilla::(anonymous namespace)::RunWatchdog(void*)]

Categories

(Core :: WebRTC, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox48 --- fixed
firefox49 --- fixed

People

(Reporter: KWierso, Assigned: jesup)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Priority: -- → P5
Component: Audio/Video → WebRTC
Priority: P5 → --
Rank: 35
Priority: -- → P3
Andreas - what's the reason for the 'blocks'?  I see this jumped around 2 weeks ago; what's the regression source, if you have an idea?

We need to find a way to get more info about why something fails shutdown....
Flags: needinfo?(pehrsons)
It blocks because of https://treeherder.mozilla.org/#/jobs?repo=try&revision=da5e30ca94c6.

I have no idea of any regression source really. Cloning?
Flags: needinfo?(pehrsons)
Maire, this is our number 3 intermittent.  Can we get someone from the media team to take a look at it?
Flags: needinfo?(mreavy)
We need to clean up the TaskQueue for VideoFrameConverter *before* dropping the reference to it, since if the last reference is held by an event in the TaskQueue, it will try to clean itself up (and fail).  Also adds an assertion to TaskQueue against self-immolation
Attachment #8751547 - Flags: review?(cpearce)
Assignee: nobody → rjesup
Status: NEW → ASSIGNED
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff16a3031beb

Nathan: I added an assert against a TaskQueue trying to swallow it's tail and kill itself.  

Once thing we should consider is if a TaskQueue should allow BeginShutdown/AwaitShutdownAndIdle() more than once (return a new promise that resolves when the original does or immediately, for exmaple), or if it shoudl be held to be an error (MOZ_ASSERT).  I sidestepped it by coding such that I'll continue to call them once.
Flags: needinfo?(nfroyd)
Rank: 35 → 15
Flags: needinfo?(mreavy)
Priority: P3 → P1
Comment on attachment 8751547 [details] [diff] [review]
clean up TaskQueue before dropping references to it

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

::: xpcom/threads/TaskQueue.cpp
@@ +98,5 @@
>  
>  void
>  TaskQueue::AwaitShutdownAndIdle()
>  {
> +  MOZ_ASSERT(!IsCurrentThreadIn());

Fingers crossed this doesn't turn the tree red!

Maybe it'll make bug 1264694 permafail.
Attachment #8751547 - Flags: review?(cpearce) → review+
https://hg.mozilla.org/mozilla-central/rev/52abfa1b6229
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Please request Aurora approval on this when you get a chance. I don't see any obvious hits of this on Beta, but feel free to nominate there too if you feel it's warranted.
Flags: needinfo?(rjesup)
Blocks: 1208371
Flags: needinfo?(rjesup)
Comment on attachment 8751547 [details] [diff] [review]
clean up TaskQueue before dropping references to it

Approval Request Comment
[Feature/regressing bug #]: 1208371 (only in 48 and newer)

[User impact if declined]: Random shutdown hangs, mochitest intermittent

[Describe test coverage new/current, TreeHerder]: It shows up as an intermittent

[Risks and why]: Very low risk; mostly moves where BeginShutdown/etc is called to avoid it happening on the TaskQueue thread itself (and added assertion against this happening, here or elsewhere).

[String/UUID change made/needed]: none
Attachment #8751547 - Flags: approval-mozilla-aurora?
(In reply to Randell Jesup [:jesup] from comment #10)
> Once thing we should consider is if a TaskQueue should allow
> BeginShutdown/AwaitShutdownAndIdle() more than once (return a new promise
> that resolves when the original does or immediately, for exmaple), or if it
> shoudl be held to be an error (MOZ_ASSERT).  I sidestepped it by coding such
> that I'll continue to call them once.

I assume this is the reason for the needinfo.  I think you could reasonably do it either way.  Our threads and thread pool implementations permit Shutdown to be called on them multiple times (though nsThread will complain at you in a debug build).  Trying to enforce a single call to Shutdown will help make our shutdown code more sensible, I think, so let's go with that if possible.
Flags: needinfo?(nfroyd)
Comment on attachment 8751547 [details] [diff] [review]
clean up TaskQueue before dropping references to it

Fix an intermittent, taking it
Attachment #8751547 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.