TaskController sequencing is not atomic
Categories
(Core :: Performance, defect, P1)
Tracking
()
| Tracking | Status | |
|---|---|---|
| firefox-esr78 | --- | unaffected |
| firefox81 | --- | wontfix |
| firefox82 | --- | wontfix |
| firefox83 | --- | fixed |
| firefox84 | --- | fixed |
People
(Reporter: denispal, Assigned: denispal)
References
Details
Attachments
(1 file)
|
(deleted),
text/x-phabricator-request
|
pascalc
:
approval-mozilla-beta+
jcristau
:
approval-mozilla-release-
|
Details |
sCurrentTaskSeqNo in the TaskController is used to assign a sequence number to all tasks created. However, it is not currently updated atomically so it is possible for two threads to create a task with the same sequence number. This could lead to some tasks not being inserted into the task graph and dropped since mMainThreadTasks will think they are identical.
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 1•4 years ago
|
||
|
||
Comment 3•4 years ago
|
||
| bugherder | ||
|
|
Assignee | |
Comment 4•4 years ago
|
||
Comment on attachment 9183486 [details]
Change type of sCurrentTaskSeqNo to Atomic<uint64_t>
Beta/Release Uplift Approval Request
- User impact if declined: Without this fix, some events that are dispatched to the main event queue may be dropped and never re-executed. This can lead to events such as key presses or navigations not doing anything, or weird behavior in other circumstances. Generally, repeating the task should probably work fine.
- Is this code covered by automated tests?: Yes
- Has the fix been verified in Nightly?: No
- Needs manual test from QE?: No
- If yes, steps to reproduce:
- List of other uplifts needed: None
- Risk to taking this patch: Low
- Why is the change risky/not risky? (and alternatives if risky): The change forces atomic access to one variable so that sequential consistency is forced. It should be safe.
- String changes made/needed:
|
||
Comment 5•4 years ago
|
||
Why should we uplift this to release? Is it a new bug? Did we see evidence of it happening in the wild?
|
|
Assignee | |
Comment 6•4 years ago
|
||
(In reply to Julien Cristau [:jcristau] from comment #5)
Why should we uplift this to release? Is it a new bug? Did we see evidence of it happening in the wild?
It's a bug we just discovered that could lead to events not being added properly to the event queue. This could have a very wide range of intermittent effects from missing keystrokes to broken websites. Since it heavily depends on a race condition to exist, it's likely rare to occur and hard to reproduce. Some situations like missing keystrokes have been reported recently, but it's too hard to tell if they are related.
|
|
||
Comment 7•4 years ago
|
||
Looks like this code dates back to bug 1606706, in 80. I'd rather not uplift to release.
|
|
||
Updated•4 years ago
|
|
||
Comment 8•4 years ago
|
||
Comment on attachment 9183486 [details]
Change type of sCurrentTaskSeqNo to Atomic<uint64_t>
We are still in the early betas part of the 83 cycle, let's take it for 83.0b5 as it is a P1/S2, thanks
|
||
Comment 9•4 years ago
|
||
| bugherder uplift | ||
Description
•