981201 - Remove JS_IsRunning

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Bobby Holley (:bholley)]

Bug 981187 is the main consumer, but there are a few other stragglers that we'll need to write patches for. Should be very easy to sort out once bug 981187 is done.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Remaining callers are:

* one in ~AutoLastFrameCheck (jsapi.cpp), behind an autoJSAPIOwnsErrorReporting check
* one in ReportError (jscntxt.cpp), behind an autoJSAPIOwnsErrorReporting check
* one in js::ReportOutOfMemory (jscntxt.cpp)
* one in PreciseGCRunnable::run (XPCComponents.cpp)
* one in /js/src/jsapi-tests/testContexts.cpp
* one in /js/src/shell/js.cpp

Comment 2

[Bobby Holley (:bholley)]

(In reply to :Ms2ger from comment #1)
> Remaining callers are:
> 
> * one in ~AutoLastFrameCheck (jsapi.cpp), behind an
> autoJSAPIOwnsErrorReporting check
> * one in ReportError (jscntxt.cpp), behind an autoJSAPIOwnsErrorReporting
> check

Yeah, these are the big ones - we basically need patches to convert everything over to TakeOwnershipOfErrorReporting, after which point this should be relatively straightforward.

Comment 3

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

(In reply to :Ms2ger from comment #1)
> * one in js::ReportOutOfMemory (jscntxt.cpp)

This one's moving behind an autoJSAPIOwnsErrorReporting check in bug 1248233.

Comment 4

[Jan de Mooij [:jandem]]

According to bug 1274193 comment 25, we should be able to remove this now (or at least the saved frame chain behavior).

Comment 5

[Jan de Mooij [:jandem]]

Nowadays there's only 1 caller left, PreciseGCRunnable::Run:

http://searchfox.org/mozilla-central/rev/846adaea6ccd1428780ed47d0d94da18478acdbb/js/xpconnect/src/XPCComponents.cpp#2631

We dispatch the runnable if we have JS on the stack and else we GC.

bholley, can we do something else there? Maybe we can check ScriptSettingsStack::EntryGlobal()?

Comment 6

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #5)
> Nowadays there's only 1 caller left, PreciseGCRunnable::Run:

Hm. Given that this event is already being dispatched from the event loop, the only time there could be JS on the stack is if somebody is spinning a nested event loop. And if they are, we do enough state-hiding that I'm not even sure that the JS_IsRunning check would catch it.

This was added in bug 661927. From what I gather there, the concern may have been more about other threads or something? If so, we can maybe just get rid of it.

CCing a few other people who might have something to say about this. Blake, is there context from that bug that I'm missing?

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #6)
> This was added in bug 661927. From what I gather there, the concern may have
> been more about other threads or something? If so, we can maybe just get rid
> of it.

It's been a while, but I don't think the worry was necessarily other threads. Instead, I think it was more that someone could be spinning a nested event loop on one JSContext while we scheduled the GC and tried to run it one a different JSContext. That obviously can't happen anymore. I don't remember how well we hid the state when spinning the event loop back then, so it's very possible that all of this code was simply added out of an abundance of caution (read: paranoia) and has never actually been useful.

I also don't really know the contract of SchedulePreciseGC too well. It might not be the end of the world if we were to schedule a GC with paused, event-loop-spinning code on the stack. If we truly wanted to avoid that, we could probably hook into the AfterProcessTask code found at [1] somehow to strongly guarantee that we're the only code running. That seems like overkill. Do we even have non-precise GCs anymore? Wasn't the point of this API to avoid the stack scanner? Can't we simply get rid of it or change it to just GC off the event loop (to preserve semantics for the objects in the stack frame that calls the function)?

[1] http://searchfox.org/mozilla-central/rev/4b6cab91f93c73ae591dafaea40fd5704b41810e/js/xpconnect/src/XPCJSContext.cpp#3601

Comment 8

[Bobby Holley (:bholley)]

(In reply to Blake Kaplan (:mrbkap) from comment #7)
> Can't we simply get rid of it or change it
> to just GC off the event loop (to preserve semantics for the objects in the
> stack frame that calls the function)?

SGTM.

Comment 9

[Bobby Holley (:bholley)]

(That's basically what this function does right now, AFAICT, since I don't think JS_IsRunning would ever return true).

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Patch

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8810319 [details] [diff] [review]
Patch

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

\o/

Comment 12

[Luke Wagner [:luke]]

\o/

Comment 13

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

\o/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf95e2986737
Stop using JS_IsRunning in PreciseGCRunnable::Run and remove JS_IsRunning. r=bholley

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/bf95e2986737