1559821 - JSContext needs to know when its thread is running a ParseTask without checking for HelperThread_

Status: RESOLVED FIXED

(Core :: JavaScript Engine, task)

Description

[Kris Wright :KrisWright]

js::CurrentThreadIsParseThread() does a null check on both helperThread() and helperThread()->parseTask(). This check doesn't work if you haven't set a pointer to a HelperThread, which is why the HelperThread() nullchecks have been replaced with isHelperThreadContext().

I can see two ways to do this - either add a flag for CurrentTaskIsParse (or some similar, better name), or actually have JSContext hold a pointer to the parse task (like it does with HelperThread_ [2]) and do null checks on that instead.

I believe there are some other instances of HelperThread()->ParseTask() for asserting OOM and errors that will also need to be cleaned up, which makes me lean to having a pointer to a parse task, unless these issues don't come up outside of native Spidermonkey.

[1] https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/js/src/vm/HelperThreads.cpp#1063
[2] https://searchfox.org/mozilla-central/rev/b3b401254229f0a26f7ee625ef5f09c6c31e3949/js/src/vm/JSContext.h#160

Comment 1

[Kris Wright :KrisWright]

Attachment: Bug 1559821 - teach JSContext about parse tasks

Added a ParseTask pointer to JSContext, set/removed during ParseTask::runTask

Comment 2

[Kris Wright :KrisWright]

Comment on attachment 9072894 [details]
Bug 1559821 - teach JSContext about parse tasks

Hello! Before I put up a patch that swaps out all the parse task checks, would this be the right way to go about looking for ParseTasks without a helper thread pointer present?

Comment 3

[Jon Coppeard (:jonco)]

(In reply to Kristen Wright :KrisWright from comment #2)
Yes, this looks good to me.

I think all of the remaining uses of JSContext::helperThread() are to get the parse task anyway so you can probably get rid of this completely and switch over to your new parseTask().

Comment 4

[Kris Wright :KrisWright]

Attachment: Bug 1559821 - 2: switch parse task checks for new parse task pointer, cleanup instances of HelperThread() leftover

Removed helperthread_ and its associated functions since they are no longer used & cleaned up a few missed checks that used it. Changed helperThread()->parseTask() checks to look for cx->parseTask() instead.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/321a971b53b8
teach JSContext about parse tasks r=jonco
https://hg.mozilla.org/integration/autoland/rev/2a01f6a6b7dc
2: switch parse task checks for new parse task pointer, cleanup instances of HelperThread() leftover r=jonco

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/321a971b53b8
https://hg.mozilla.org/mozilla-central/rev/2a01f6a6b7dc