Closed Bug 1630802 Opened 3 months ago Closed 3 months ago

Remove AutoEnter

Categories

(Core :: XPCOM, task)

task

Tracking

()

RESOLVED FIXED
mozilla77
Tracking Status
firefox77 --- fixed

People

(Reporter: jya, Assigned: jya)

References

Details

Attachments

(9 files)

47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review
47 bytes, text/x-phabricator-request
Details | Review

AbstractThread::AutoEnter was added in bug 1364821 has a work around conflicts between the main-thread AbstractThread and the MessageLoopAbstractThreadWrapper that happened to get initialized from the main thread.

MessageLoopAbstractThreadWrapper was set the AbstractThread::sCurrentThreadTLS object which would later influence what AbstractThread::GetCurrent() would return.

MessageLoopAbstractThreadWrapper no longer exists. So AutoEnter is no longer necessary and should be removed as it caused unnecessary logical burden.

prior bug 1364821, AbstractThread::GetCurrent() would always return AbstractThread::MainThread() when called from the main thread.
After this change, we had to run within an AutoEnter scope.

A hidden side effect of this change was that under most cases AbstractThread::MainThread::Dispatch() would no longer use the tail dispatcher to dispatch a task.

It can be safely assume that whenever you're on the main thread, the equivalent AbstractThread is usable.

In the next commit, we will be removing AutoEnter entirely.

AbstractThread::MainThread() requires TailDispatcher to work which itself relies on the nsCycleCollectors to have been initialised.

This early in the XPCOM initilization, it doesn't work.

A bug was hiding the fact that the tail dispatcher wasn't available on the main thread by default.

Attachment #9141172 - Attachment description: Bug 1592488 - P2. Do not use AbstractThread too early. r?padenot → Bug 1630802 - P1. Do not use AbstractThread too early. r?padenot
Attachment #9141136 - Attachment description: Bug 1630802 - P1. Make AbstractThread::GetCurrent() return MainThread on the main thread. r?bholley → Bug 1630802 - P2. Make AbstractThread::GetCurrent() return MainThread on the main thread. r?bholley

AutoEnter was an attempt around a race between AbstractThread and MessageLoopAbstractThreadWrap that would cause AbstractThread::GetCurrent() to return an incorrect value. MessageLoopAbstractThreadWrapper is no more and as such AutoEnter is no longer required.

Include reversal of commit 3218d3c2662a (bug 1529399)

Depends on D71148

Attachment #9141452 - Attachment description: Bug 1630802- P1. Cleanup AbstractThread TLS lookup. r?gerald → Bug 1630802 - P1. Cleanup AbstractThread TLS lookup. r?gerald
Attachment #9141172 - Attachment description: Bug 1630802 - P1. Do not use AbstractThread too early. r?padenot → Bug 1630802 - P2. Do not use AbstractThread too early. r?padenot
Attachment #9141136 - Attachment description: Bug 1630802 - P2. Make AbstractThread::GetCurrent() return MainThread on the main thread. r?bholley → Bug 1630802 - P3. Make AbstractThread::GetCurrent() return MainThread on the main thread. r?bholley

It lead AbstractThread assumption that there can only be one active per thread to be false.

Depends on D71148

Attachment #9141177 - Attachment description: Bug 1630802 - P3. Remove unnecessary AutoEnter. r?bholley → Bug 1630802 - P5. Remove unnecessary AutoEnter. r?bholley

It was required once upon a time to be able to use MozPromise on Workers.
Today a MozPromise work with nsISerialEventTarget and no longer rely on this. It can go.

Depends on D71279

Once upon a time, a MozPromise required an AbstractThread as target thread. So we had to wrap the current child manager thread into an AbstractThread.

This is no longer the case, MozPromise now work with nsISerialEventTarger, which a nsIThread is. The AbstractThread used didn't support tail dispatch so the replacement was straight forward.

Attachment #9141515 - Attachment description: Bug 1630802 - P7. Remove no longer required GetManagerAbstractThread. r?bholley,mattwoodrow → Bug 1630802 - P5. Remove no longer required GetManagerAbstractThread. r?bholley,mattwoodrow
Attachment #9141177 - Attachment description: Bug 1630802 - P5. Remove unnecessary AutoEnter. r?bholley → Bug 1630802 - P6. Remove unnecessary AutoEnter. r?bholley
Attachment #9141462 - Attachment description: Bug 1630802 - P6. EventTargetWrapper runners don't need to be cancellable. r?bholley → Bug 1630802 - P7. EventTargetWrapper runners don't need to be cancellable. r?bholley

This mechanism is currently unused (see bug 1624819), we can re-assess the issue once it becomes needed.

Attachment #9141582 - Attachment description: Bug 1630802 - P8. Always return AbstractThread::MainThread for AbstractMainThreadFor. r?bholley → Bug 1630802 - P6. Always return AbstractThread::MainThread for AbstractMainThreadFor. r?bholley
Attachment #9141583 - Attachment description: Bug 1630802 - P9. Enforce that only a single EventTargetWrapper can exist par thread. r?bholley → Bug 1630802 - P7. Enforce that only a single EventTargetWrapper can exist par thread. r?bholley
Attachment #9141177 - Attachment description: Bug 1630802 - P6. Remove unnecessary AutoEnter. r?bholley → Bug 1630802 - P8. Remove unnecessary AutoEnter. r?bholley
Attachment #9141462 - Attachment description: Bug 1630802 - P7. EventTargetWrapper runners don't need to be cancellable. r?bholley → Bug 1630802 - P9. EventTargetWrapper runners don't need to be cancellable. r?bholley
Attachment #9141583 - Attachment description: Bug 1630802 - P7. Enforce that only a single EventTargetWrapper can exist par thread. r?bholley → Bug 1630802 - P7. Remove EventTargetWrapper and ensure that only a single AbstractThread exists par nsIThread. r?bholley
Attachment #9141583 - Attachment description: Bug 1630802 - P7. Remove EventTargetWrapper and ensure that only a single AbstractThread exists par nsIThread. r?bholley → Bug 1630802 - P7. Remove CreateEventTargetWrapper and ensure that only a single AbstractThread exists par nsIThread. r?bholley
Pushed by jyavenard@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1e5316093afd
P1. Cleanup AbstractThread TLS lookup. r=gerald
https://hg.mozilla.org/integration/autoland/rev/a5b19d93ab3f
P2. Do not use AbstractThread too early. r=padenot
https://hg.mozilla.org/integration/autoland/rev/9ecfeb1915cd
P3. Make AbstractThread::GetCurrent() return MainThread on the main thread. r=bholley
https://hg.mozilla.org/integration/autoland/rev/b94e2c408ecc
P4. Remove unused member. r=bholley
https://hg.mozilla.org/integration/autoland/rev/5c3d7c51ecc3
P5. Remove no longer required GetManagerAbstractThread. r=mattwoodrow
https://hg.mozilla.org/integration/autoland/rev/8d9486cd19e8
P6. Always return AbstractThread::MainThread for AbstractMainThreadFor. r=bholley
https://hg.mozilla.org/integration/autoland/rev/7d104d25415c
P7. Remove CreateEventTargetWrapper and ensure that only a single AbstractThread exists par nsIThread. r=bholley
https://hg.mozilla.org/integration/autoland/rev/c2a43696053d
P8. Remove unnecessary AutoEnter. r=bholley
https://hg.mozilla.org/integration/autoland/rev/ce0291e3f743
P9. EventTargetWrapper runners don't need to be cancellable. r=bholley
You need to log in before you can comment on or make changes to this bug.