1366322 - Baldr: remove JSContext dependencies from wasm::TlsData

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Luke Wagner [:luke]]

Attachment: rm-cx-dep

I noticed two cases (cx, stackLimit) where we save JSContext-/thread-specific pointers in wasm::TlsData which won't get updated as execution for a single instance hops between JSContexts via cooperative scheduling.  Sounds like this will soon be turned on in the browser so time to fix.

Comment 1

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8869505 [details] [diff] [review]
rm-cx-dep

Review of attachment 8869505 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, thanks for the patch!

Bonus points if you could make test cases that would have crashed, using the JS shell cooperative intrinsics: http://searchfox.org/mozilla-central/source/js/src/shell/js.cpp#6242,6246

::: js/src/wasm/WasmDebug.cpp
@@ +289,5 @@
>          return true;
>  
>      stepModeCounters_.remove(p);
>  
> +    AutoWritableJitCode awjc(fop->runtime(), code_->segment().base() + codeRange.begin(),

Why not just passing the runtime as an argument to decrementStepModeCount, instead of the fop? (it would make more sense for one of the call sites where there's actually a small dance to get the fop from the runtime, and here we get back to the runtime from the fop)

Comment 2

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Why not just passing the runtime as an argument to decrementStepModeCount,
> instead of the fop?

Just symmetry with JSScript::decrementStepModeCount().

Comment 3

[Luke Wagner [:luke]]

(In reply to Benjamin Bouvier [:bbouvier] from comment #1)
> Bonus points if you could make test cases that would have crashed, using the
> JS shell cooperative intrinsics:
> http://searchfox.org/mozilla-central/source/js/src/shell/js.cpp#6242,6246

Good idea.  I tried but unfortunately it looks like it's not possible to get a single JSObject (wasm instance) to hop between JSContexts atm.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9733f84ff5cc
Baldr: remove dependency of TlsData on specific JSContext (r=bbouvier)

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9733f84ff5cc