ThreadData in JSContext needs to check against its JSContext, not the thread ID
Categories
(Core :: JavaScript Engine, task, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox69 | --- | fixed |
People
(Reporter: KrisWright, Assigned: KrisWright)
References
Details
Attachments
(2 files)
In bug 1559659 we want to make a global list of JSContexts for threads to take. As it is it's impossible for protected data checks to work on the contents of JSContext inside the global vector, because ThreadData examines the thread ID on creation and makes an assumption that JSContext will always do work on the thread it was created for. This is going to change if offthread tasks will take initialized JSContexts from the list and set them as their thread-local JSContext. From what I understand the ThreadData in JSContext is more context-specific than thread-specific, with an assumption that each JSContext is created for its own thread.
To fix this, the ThreadData in JSContext could compare the cx it was created in with the current TlsContext. These checks will need to be suppressed during ctor/dtor, since during construction the TlsContext doesn't exist, and TlsContext will be removed during shutdown.
The non-JSContext use of ThreadData (https://searchfox.org/mozilla-central/rev/867cbb1a2b232398616e1aa42f913f37c6cb38e4/js/src/builtin/AtomicsObject.h#141) should stay as it is because it is actually thread-specific.
Assignee | ||
Comment 1•6 years ago
|
||
Adds CheckContextLocal protected data checks and a data type to check a JSContext against the TlsContext of the thread. Objects must specify the JSContext in which they are created in to check against.
Assignee | ||
Comment 2•6 years ago
|
||
Convert ThreadData to contextLocalData from JSContext. Not buildable because of uninitialized NativeStackQuota.
Assignee | ||
Comment 3•6 years ago
|
||
Comment on attachment 9075245 [details]
bug 1562309 - 2. Convert ThreadData in JSContext to ContextData
Most of this is what we talked about a few days ago (converting ThreadData in JSContext to compare cx instead) but I realized that nativeStackQuota is an array of ThreadData<size_t> so I can't just explicitly initialize with the rest (unless there is some c++ way to do that which I have just forgotten). Off the top of my head I could make it some other type like ThreadData<js::Vector> to bypass this, or I could create an additional ContextLocalNoArgs type and a setter to make it hold its JSContext pointer. Is there a good way to address this?
Assignee | ||
Updated•6 years ago
|
Comment 4•6 years ago
|
||
(In reply to Kristen Wright :KrisWright from comment #3)
but I realized that nativeStackQuota is an array of ThreadData<size_t> so I can't just explicitly initialize with the rest (unless there is some c++ way to do that which I have just forgotten).
cx->nativeStackQuota is never read, we should just remove it.
Comment 5•6 years ago
|
||
Comment on attachment 9075245 [details]
bug 1562309 - 2. Convert ThreadData in JSContext to ContextData
This all looks good.
It looks like the check() call doesn't happen when fields are constructed. Is it possible to switch the current context when destroying them so that these assertions pass? Then you wouldn't need suppressContextChecks_.
nit: I'd prefer the name ContextData to be more in keeping with the other ProtectedData classes (they're all local to something).
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #5)
It looks like the check() call doesn't happen when fields are constructed. Is it possible to switch the current context when destroying them so that these assertions pass? Then you wouldn't need suppressContextChecks_.
While contexts are per-thread nothing will need to be done at all since TlsContext is not reset until the end of the dtor. But the context pool in the future will have to wait for the HelperThreads to finish, which happens after the main-thread context is destroyed. Maybe it'll be the best to ensure first that TlsContext.get() is equal to either the current JSContext or nullptr, so that JSContext can set itself.
Updated•6 years ago
|
Comment 7•6 years ago
|
||
(In reply to Kristen Wright :KrisWright from comment #6)
It sounds good to have some checks around this. Also we could add an optional thread ID to each context to make sure that it's never used by more than one thread at a time.
Updated•6 years ago
|
Assignee | ||
Comment 9•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #7)
(In reply to Kristen Wright :KrisWright from comment #6)
It sounds good to have some checks around this. Also we could add an optional thread ID to each context to make sure that it's never used by more than one thread at a time.
The general idea I'm going with right now for getting contexts from the global context pool is to set a thread ID variable when setting the TlsContext, then using that to check if a context from the pool is in use. It can all be done at once with an AutoSetContextAndThread (or similarly named RAII class) that sets the TlsContext and the thread ID. The main-thread context could worry about setting itself during js::NewContext so that no assumptions are made during the ctor that they are thread-specific.
Comment 10•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/636ffcf7e251
https://hg.mozilla.org/mozilla-central/rev/dc1ad84da67e
Comment 11•6 years ago
|
||
(In reply to Kristen Wright :KrisWright from comment #9)
Sounds good to me.
Description
•