1380530 - Intermittent devtools/client/canvasdebugger/test/browser_profiling-canvas.js | application crashed [@ MessageLoop::DeletePendingTasks()]

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Treeherder Bug Filer]

Filed by: wkocher [at] mozilla.com

https://treeherder.mozilla.org/logviewer.html#?job_id=113731739&repo=mozilla-inbound

https://queue.taskcluster.net/v1/task/Z1GXHZIsR8y2flhdHUZnhw/runs/0/artifacts/public/logs/live_backing.log

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to have started when bug 1376026 landed:
https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=23814434a6bc690b51bddd1ddd2b86382c32fe47&noautoclassify&filter-searchStr=b684e13e95340ff6010a5dcb9d887238b7b83181&group_state=expanded&tochange=633c98dc992ff13168710725ea94a8ac49f6d48a

Comment 2

[Lee Salzman [:lsalzman]]

Initial investigation points at the runnable stuck on the work queue to be of ObjectWatcher::Watch, which following the chain of users seems to point to ProcessWatcher::EnsureProcessTerminated. The most probable user of that to be causing that seems like GeckoChildProcessHost::~GeckoChildProcessHost. Why this would in any way be related to a change to use DirectWrite for fonts on Windows 7 is as yet undetermined, and I am still looking into it. Superficially, it seems unrelated and more likely to be a result of changed timing patterns in the tests exposing a pre-existing problem.

Comment 3

[Intermittent Failures Robot]

11 failures in 720 pushes (0.015 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* mozilla-inbound: 9
* mozilla-central: 1
* autoland: 1

Platform breakdown:
* windows7-32: 11

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1380530&startday=2017-07-10&endday=2017-07-16&tree=all

Comment 4

[Honza Bambas (:mayhemer)]

Note that the oldest open report of this assertion failure to date was from 2016-01-03 (bug 1236350).

Lee, are you willing to work on this bug?  Looks like you went deeper with hunting this down than me in bug 1355196.

Comment 5

[Lee Salzman [:lsalzman]]

(In reply to Honza Bambas (:mayhemer) from comment #4)
> Note that the oldest open report of this assertion failure to date was from
> 2016-01-03 (bug 1236350).
> 
> Lee, are you willing to work on this bug?  Looks like you went deeper with
> hunting this down than me in bug 1355196.

I am still in the process of investigating this.

Comment 6

[Honza Bambas (:mayhemer)]

Great!  Can I assign this bug then to you?

Comment 7

[Lee Salzman [:lsalzman]]

Attachment: don't allow new tasks to be posted to MessageLoop when it is shutting down

The source of this particular instance of the intermittent in DeletePendingTasks was coming from ObjectWatcher queuing a task after the message loop is already in its death throes. We let it queue the task even though it would never actually be processed and merely trigger this assertion.

This adds tracking for whether the MessageLoop is shutting down. Callers sometimes adds tasks before the Run() loop is actually started, and usually during the Run() loop itself, both of which are safe times to add tasks since they will still actually get processed. However, after the Run() loop returns, it is never invoked again, so it is thus not safe to add tasks.

The added assertion in PostTask_Helper here will help us pinpoint the intermittents at the actual time they are triggered by calls to PostTask, rather than waiting all the way till DeletePendingTasks is called.

And in particular with ObjectWatcher::DoneWaiting, we simply skip adding the pending task if the MessageLoop is shutting down. ObjectWatcher only gets used by ChildReaper, and ChildReaper will always call KillProcess() in its destructor in the worst case, to ensure the process actually gets cleaned up.

Comment 8

[Bob Owen (:bobowen)]

Comment on attachment 8887624 [details] [diff] [review]
don't allow new tasks to be posted to MessageLoop when it is shutting down

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

I'm not a peer for this code, so redirecting to billm.

(In reply to Lee Salzman [:lsalzman] from comment #7)
...
> And in particular with ObjectWatcher::DoneWaiting, we simply skip adding the
> pending task if the MessageLoop is shutting down. ObjectWatcher only gets
> used by ChildReaper, and ChildReaper will always call KillProcess() in its
> destructor in the worst case, to ensure the process actually gets cleaned up.

Well in the non-forced case, we really have to rely on ChildReaper::WillDestroyCurrentMessageLoop to do the clean-up, because I think we won't get deleted unless that happens anyway.

As far as I can tell in the non-forced case we will never kill a hung/slow-to-shutdown child process, but that is no change from how it works now.

::: ipc/chromium/src/base/message_loop.h
@@ +120,4 @@
>    // NOTE: These methods may be called on any thread.  The Task will be invoked
>    // on the thread that executes MessageLoop::Run().
>  
> +  bool IsProcessingTasks() const { return !shutting_down_; }

I think this name is slightly misleading, perhaps IsAcceptingTasks would be better?

::: ipc/chromium/src/base/object_watcher.cc
@@ +128,5 @@
>    // provided, which in turn ensures our change to did_signal can be observed
>    // on the target thread.
> +  if (watch->origin_loop->IsProcessingTasks()) {
> +    watch->origin_loop->PostTask(addrefedWatch.forget());
> +  }

I think that this is OK, because any remaining clean-up should still happen as a result of ChildReaper::WillDestroyCurrentMessageLoop and ObjectWatcher::WillDestroyCurrentMessageLoop in the non-forced case.
In the forced/ref-counted case ~ChildReaper calling KillProcess will do it as a last resort.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/04e3649ca8fc
don't allow new tasks to be posted to MessageLoop when it is shutting down. r=billm

Comment 10

[Lee Salzman [:lsalzman]]

It should be noted again that, provided this landing sticks, that we're going to essentially be trading the assertion inside MessageLoop::DeletePendingTasks() for assertions inside MessageLoop::PostTask_Helper coming from different callers. This will be nothing to be alarmed about, because in general it will point the finger at the thread that decided to queue the bad task where we actually have enough context information to track down things, as opposed to the receiver, which has lost that context.

Comment 11

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/04e3649ca8fc

Comment 12

[Intermittent Failures Robot]

4 failures in 822 pushes (0.005 failures/push) were associated with this bug in the last 7 days.   

Repository breakdown:
* autoland: 2
* try: 1
* mozilla-inbound: 1

Platform breakdown:
* windows7-32: 4

For more details, see:
https://brasstacks.mozilla.com/orangefactor/?display=Bug&bugid=1380530&startday=2017-07-17&endday=2017-07-23&tree=all