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

RESOLVED FIXED in Firefox 50

Status

()

Core
XPCOM
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: cyu, Assigned: cyu)

Tracking

Trunk
mozilla52
Points:
---

Firefox Tracking Flags

(firefox49 wontfix, firefox50 fixed, firefox51 fixed, firefox52 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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 hidden (mozreview-request)
(Assignee)

Updated

2 years ago
Attachment #8792769 - Flags: review?(nfroyd)

Updated

2 years ago
Summary: MessageLopo::PostDelayedTask() schedules the runnable to the calling thread → MessageLoop::PostDelayedTask() schedules the runnable to the calling thread

Comment 2

2 years ago
mozreview-review
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.
Attachment #8792769 - Flags: review?(nfroyd) → review+
Thank you for catching this!

Comment 4

2 years ago
Pushed by cyu@mozilla.com:
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
(Assignee)

Updated

2 years ago
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected

Comment 5

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b3cc2b2d346c
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Assignee)

Comment 6

2 years ago
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.
Attachment #8792769 - Flags: approval-mozilla-beta?
(Assignee)

Comment 7

2 years ago
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.
Attachment #8792769 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 8

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

Comment 9

2 years ago
(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 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.
Attachment #8792769 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 11

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/6113e3515815
status-firefox51: affected → fixed
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+
Attachment #8792769 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
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.
status-firefox49: affected → wontfix

Comment 14

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4b4d0ec14dc5
status-firefox50: affected → fixed
You need to log in before you can comment on or make changes to this bug.