1341924 - "Assertion failure: cancelable (Only nsICancelableRunnable may be dispatched to a worker!)" triggered by profiler

Status: RESOLVED FIXED

(Core :: Gecko Profiler, defect)

Description

[Nicholas Nethercote [inactive]]

STR: debug build, start profiler.
Click on profiler button, then Settings, Threads; and enter
"," (a single comma) in the box.  Profiler typically asserts shortly afterwards. Sometimes the assertion in bug 1340193 hits first, though.

Stack trace:

> #0 0x00007fffe7989a66 in mozilla::dom::workers::WorkerThread::Dispatch(already_AddRefed<nsIRunnable>, unsigned int) (this=0x4139a40, aRunnable=..., aFlags=<optimized out>) at /home/sewardj/MOZ/MC-TALL/dom/workers/WorkerThread.cpp:235
> #1 0x00007fffe8f5d6d9 in nsIEventTarget::Dispatch(nsIRunnable*, unsigned int) (aFlags=0, aEvent=<optimized out>, this=0x4139a40) at /home/sewardj/MOZ/MC-TALL/ff-Og-linux64-dbg/dist/include/nsIEventTarget.h:37
> #2 0x00007fffe8f5d6d9 in ThreadResponsiveness::Update(bool, nsIThread*) (this=this@entry=0x7ffecc1bde80, aIsMainThread=<optimized out>, aThread=0x4139a40) at /home/sewardj/MOZ/MC-TALL/tools/profiler/gecko/ThreadResponsiveness.cpp:105
> #3 0x00007fffe8f5d8e9 in ThreadInfo::UpdateThreadResponsiveness() (this=0x7ffecc1bdbe0) at /home/sewardj/MOZ/MC-TALL/tools/profiler/core/ThreadInfo.h:81
> #4 0x00007fffe8f5d8e9 in SigprofSender(void*) (aArg=<optimized out>) at /home/sewardj/MOZ/MC-TALL/tools/profiler/core/platform-linux.cc:290
> #5 0x00007ffff7bc06ca in start_thread (arg=0x7ffeebfff700) at pthread_create.c:333
> #6 0x00007ffff6e4ef7f in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:105

The cause here is clear: we dispatch a non-cancelable runnable to a worker thread, which is not allowed. Changing CheckResponsivenessTask to be cancelable should fix it. This should be as simple as adding a Cancel() method to it.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Make CheckResponsivenessTask cancelable

This avoids the assertion. An even simpler option was to use the default
Cancel() which is a no-op, but changing Terminate() to Cancel() seemed
reasonable.

Comment 2

[Nicholas Nethercote [inactive]]

bkelly, we discussed this on IRC. Do you have any comments? Thanks.

Comment 3

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8840252 [details] [diff] [review]
Make CheckResponsivenessTask cancelable

This seems reasonable, but I think there are more problems in ThreadResponsiveness.cpp when dealing with worker threads.

For example, it appears you can dispatch this thing at a worker thread.  That's fine.  But then it schedules a timer.  When the timer fires it dispatches at the main thread.  Which triggers another timer, but targeting the main thread this time.  After that it just keeps running on the main thread and not on the originally targeted worker thread.

I have to question what this is trying to do and if it works at all for anything other than main thread.

Also, in Cancel(), please check for the existence of mTimer and call mTimer->Cancel() if its present.

Comment 4

[Ben Kelly [:bkelly, not reviewing]]

Markus, do you have any ideas about the issues in comment 3?

Comment 5

[Markus Stange [:mstange]]

Now that you mention it, you're right! Responsiveness detection has never worked for non-main threads. I even added a condition to the UI so that it would only display responsiveness information for the main thread, because the numbers for non-main threads were all wrong. And then I completely forgot about it and never tried to find out why that is.
So thanks for spotting this!

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1341924 - Don't attempt to dispatch CheckResponsivenessTasks to non-main threads.

This is more of a workaround than a real fix, but the old code wasn't working
for non-main threads either, and we'd like to change the way this information
is computed anyway (bug 1340714) and then we won't need CheckResponsivenessTask
any more.

Review commit: https://reviewboard.mozilla.org/r/120206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/120206/

Comment 7

[Markus Stange [:mstange]]

Comment on attachment 8847202 [details]
Bug 1341924 - Don't attempt to dispatch CheckResponsivenessTasks to non-main threads.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120206/diff/1-2/

Comment 8

[Markus Stange [:mstange]]

Comment on attachment 8847202 [details]
Bug 1341924 - Don't attempt to dispatch CheckResponsivenessTasks to non-main threads.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120206/diff/2-3/

Comment 9

[Markus Stange [:mstange]]

Comment on attachment 8847202 [details]
Bug 1341924 - Don't attempt to dispatch CheckResponsivenessTasks to non-main threads.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/120206/diff/3-4/

Comment 10

[Nicholas Nethercote [inactive]]

Comment on attachment 8847202 [details]
Bug 1341924 - Don't attempt to dispatch CheckResponsivenessTasks to non-main threads.

https://reviewboard.mozilla.org/r/120206/#review122252

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ec3fa55ae9e5
Don't attempt to dispatch CheckResponsivenessTasks to non-main threads. r=njn

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ec3fa55ae9e5