1303940 - MessageLoop::PostDelayedTask() schedules the runnable to the calling thread

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Cervantes Yu [:cyu] [:cervantes]]

Internally in MessageLoop::PostDelayedTask() a nsITimer is created to delay the execution, but it doesn't set the target thread. The timer then uses the calling thread as the target. This makes delayed tasks posted to another thread run on the wrong thread and may create races or assertion failures.

Comment 1

[Cervantes Yu [:cyu] [:cervantes]]

Attachment: Bug 1303940 - MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread.

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

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8792769 [details]
Bug 1303940 - MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread.

https://reviewboard.mozilla.org/r/79672/#review78536

I'm surprised that nothing seems to have gone seriously wrong with a mistake like this, though maybe there are some intermittents and things that started cropping up related to this.

Comment 3

[Nathan Froyd [:froydnj]]

Thank you for catching this!

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b3cc2b2d346c
MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread. r=froydnj

Comment 5

[Carsten Book [:Tomcat]]

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

Comment 6

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8792769 [details]
Bug 1303940 - MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread.

Approval Request Comment
[Feature/regressing bug #]: 1269056
[User impact if declined]: Potential stability issues. This is a bug in the threading component in the lower layer, which makes some code scheduled on the wrong thread. Runtime aborts or race condition may result from this bug.
[Describe test coverage new/current, TreeHerder]: Patch just landed on m-c and passed auto regression tests.
[Risks and why]: Low. This patch is relatively simple and is unlikely to have other side effects.

Comment 7

[Cervantes Yu [:cyu] [:cervantes]]

Comment on attachment 8792769 [details]
Bug 1303940 - MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread.

Approval Request Comment
[Feature/regressing bug #]: 1269056
[User impact if declined]: Potential stability issues. This is a bug in the threading component in the lower layer, which makes some code scheduled on the wrong thread. Runtime aborts or race condition may result from this bug.
[Describe test coverage new/current, TreeHerder]: Patch just landed on m-c and passed auto regression tests.
[Risks and why]: Low. This patch is relatively simple and is unlikely to have other side effects.

Comment 8

[Cervantes Yu [:cyu] [:cervantes]]

This also affects 49. Should we ship this in the next point release?

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Cervantes Yu [:cyu] [:cervantes] from comment #8)
> This also affects 49. Should we ship this in the next point release?

You can nominate it for uplift to moz-release and let 49 release owner (Liz) decide but just looking at the patch and the fact that we haven't seen any real end-user feedback tied to this issue yet in Fx49, I'd say the likelihood of this going in a dot release is pretty low.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792769 [details]
Bug 1303940 - MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread.

Let's uplift this to Aurora51 today and stabilize over the weekend before uplifting to 50.0b2 next week.

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/6113e3515815

Comment 12

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8792769 [details]
Bug 1303940 - MessageLoop::PostDelayedTask() should schedule the runnable on the target thread instead of the current thread.

Fix has stabilized on Nightly and Aurora for a few days, Beta50+

Comment 13

[Liz Henry (:lizzard) (relman/hg->git project)]

Looks like this may have been a regression from 49. Unless we know it will improve stability in some particular case, I don't think fixing this on the release channel should be a priority.

Comment 14

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

https://hg.mozilla.org/releases/mozilla-beta/rev/4b4d0ec14dc5