1282150 - Rename all our JSContext stuff to be nicer

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Or not, as the case may be.  Specifically, the plan is to add mozilla::dom::danger::GetJSContext() and mozilla::dom::GetJSRuntime(), both going via CycleCollectedJSRuntime::Get().  Then we replace our existing users of GetDefaultJSContextForThread with either the runtime (when used for rooting, if we can) or the danger thing in the few cases when it's really needed.  Then we see about some of the other bits, like RootingCxForThread and GetSafeJSContext.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Add a mozilla::dom::danger::GetJSContext API

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Get rid of GetDefaultJSContextForThread

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Add a way to get the JSRuntime for the thread, and use it in various places for rooting

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Have just one way of getting the rooting JSContext, and call it nsContentUtils::RootingCx()

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Kill off all the unused GetJSContextForEventHandlers cruft

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Get rid of GetSafeJSContext

Comment 7

[Jan de Mooij [:jandem]]

bz, do you see any problems with converting APIs that take JSRuntime* to take JSContext* instead, and then to stop exposing JSRuntime to JSAPI?

I'm asking because you're adding a "danger" namespace for GetJSContext, while GetJSRuntime is more readily available.

Comment 8

[Bobby Holley (:bholley)]

(In reply to Jan de Mooij [:jandem] from comment #7)
> bz, do you see any problems with converting APIs that take JSRuntime* to
> take JSContext* instead, and then to stop exposing JSRuntime to JSAPI?
> 
> I'm asking because you're adding a "danger" namespace for GetJSContext,
> while GetJSRuntime is more readily available.

The point of the 'danger' namespace is that in reminds the caller that they should be getting the JSContext via the AutoJSAPI-derived RAII helpers. The historical confusion around "which JSContext is the right one?" ironically provided a good incentive for people to use the helpers, and in a single-cx world I'm worried we'll start to screw that up.

Historically, things that took JSRuntimes were generally less dependent on the particular state of the stack (setting runtime callbacks and whatnot), which is why it feels less important to use proper RAII hygiene in those cases. If we're converting APIs to take a JSRuntime, then we should just put GetJSRuntime in the danger namespace as well.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 8765070 [details] [diff] [review]
part 3.  Add a way to get the JSRuntime for the thread, and use it in various places for rooting

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

::: dom/base/ScriptSettings.cpp
@@ +296,5 @@
> +JSRuntime*
> +GetJSRuntime()
> +{
> +  return CycleCollectedJSRuntime::Get()->Runtime();
> +} 

Whitespace error.

Comment 10

[Bobby Holley (:bholley)]

Comment on attachment 8765071 [details] [diff] [review]
part 4.  Have just one way of getting the rooting JSContext, and call it nsContentUtils::RootingCx()

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

Hm, do we need this at all anymore? What APIs do we have that actually still require a cx to root rather than an rt? I thought those had gone away?

r=me I guess, but please file a followup for eliminating this baggage.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8765072 [details] [diff] [review]
part 5.  Kill off all the unused GetJSContextForEventHandlers cruft

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

\o/

Comment 12

[Bobby Holley (:bholley)]

Comment on attachment 8765073 [details] [diff] [review]
part 6.  Get rid of GetSafeJSContext

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

\o/

Comment 13

[Jan de Mooij [:jandem]]

(In reply to Bobby Holley (busy) from comment #8)
> If we're converting APIs to take a JSRuntime, then we should just put
> GetJSRuntime in the danger namespace as well.

The other way around - all/most APIs will take JSContext instead of JSRuntime, but I see your point. We'll have to use danger::JSContext more probably. I think a lot of these JSRuntime* APIs are initialization stuff, so there probably won't be that many places that have to change (famous last words).

Comment 14

[Boris Zbarsky [:bzbarsky]]

With any sort of luck init stuff will have just created the context/runtime and won't need to danger::JSContext at all.

> Whitespace error.

Fixed.

> What APIs do we have that actually still require a cx to root

Anything that inherits from CustomAutoRooter, for a start.  But also js::Add/RemoveRawValueRoot.  Basically anything I touched in part 4 requires a cx to root; I switched the things that didn't to use the runtime in part 3.

I filed bug 1285377 on at least the CustomAutoRooter bits.

Comment 15

[Jan de Mooij [:jandem]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Basically anything I touched in part 4 requires
> a cx to root; I switched the things that didn't to use the runtime in part 3.

This week I've been actively working on hiding JSRuntime from the embedding (bug 1283855 for instance), so we only expose JSContext to the API... It seems part 3 is going to make that harder?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> It seems part 3 is going to make that harder?

Yes.  I could convert all those places to use RootingCx() instead, I guess.  It wasn't clear to me which direction the JS team was going with all this stuff and I had been lead to believe that JSRuntime was the new hotness for rooting.

Comment 17

[Bobby Holley (:bholley)]

So, the basic issue here is that we generally want to have an AutoJSAPI-ish thing on the stack when we call into JSAPI. The easiest way to ensure this is to have these APIs take a JSContext the way they do now, and then make AutoJSAPI the obvious way to get a JSContext. 

But there are also APIs (specifically rooting ones) that we often need to use outside the scope of an AutoJSAPI, and which don't require any of the error reporting state or other machinery that AutoJSAPI sets up. That's why it's nice to have a distinct token that we can pass to the rooting constructors - one that is easily accessible, and that people aren't tempted to use when invoking JS_GetProperty.

There are various ways to solve this I'm sure. If the JS team wants to remove JSRuntime from the public facing APIs, maybe we can have all the rooting APIs take some other opaque type (that is secretly just a JSContext*), and have JSContext coerce to that type?

Comment 18

[Boris Zbarsky [:bzbarsky]]

OK, looks like nothing in bug 1283855 removes the Rooted(JSRuntime*) bits so I'm ok using them for now.  We can remove them later as needed...

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e21358504aa9
part 1.  Add a mozilla::dom::danger::GetJSContext API.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/57628a2c33d6
part 2.  Get rid of GetDefaultJSContextForThread.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/9870763b6a4c
part 3.  Add a way to get the JSRuntime for the thread, and use it in various places for rooting.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/958a6ec2d7c0
part 4.  Have just one way of getting the rooting JSContext, and call it nsContentUtils::RootingCx().  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/a54aca4aa8c9
part 5.  Kill off all the unused GetJSContextForEventHandlers cruft.  r=bholley
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8029c072776
part 6.  Get rid of GetSafeJSContext.  r=bholley

Comment 20

[Ryan VanderMeulen [:RyanVM]]

This added a new rooting hazard from the looks of it:

Function '_ZN7mozilla3dom18SimpleGlobalObject6CreateENS1_10GlobalTypeEN2JS6HandleINS3_5ValueEEE$JSObject* mozilla::dom::SimpleGlobalObject::Create(int32, class JS::Handle<JS::Value>)' has unrooted '<returnvalue>' of type 'JSObject*' live across GC call '_ZN7mozilla3dom9AutoJSAPID1Ev$void mozilla::dom::AutoJSAPI::~AutoJSAPI()' at dom/bindings/SimpleGlobalObject.cpp:149
    SimpleGlobalObject.cpp:149: Assign(86,87, return := __temp_32**)
    SimpleGlobalObject.cpp:149: Call(87,88, globalObject.~SimpleGlobalObject>())
    SimpleGlobalObject.cpp:149: Call(88,89, ac.~JSAutoCompartment())
    SimpleGlobalObject.cpp:149: Call(89,90, global.~Rooted())
    SimpleGlobalObject.cpp:149: Call(90,91, jsapi.~AutoJSAPI()) [[GC call]]
    SimpleGlobalObject.cpp:150:  [[end of function]]
GC Function: _ZN7mozilla3dom9AutoJSAPID1Ev$void mozilla::dom::AutoJSAPI::~AutoJSAPI()
    void mozilla::dom::AutoJSAPI::~AutoJSAPI(int32)
    void mozilla::dom::AutoJSAPI::ReportException()
    JSObject* xpc::FindExceptionStackForConsoleReport(nsPIDOMWindowInner*, class JS::Handle<JS::Value>)
    FieldCall: nsIStackFrame.GetNativeSavedFrame

Inbound is closed for this at the moment, so please fix or backout ASAP.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Grrr.  Stupid false positive.  Fixing.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9acdf39431fc
followup: placate the rooting analysis in SimpleGlobalObject::Create so we can reopen the CLOSED TREE

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e21358504aa9
https://hg.mozilla.org/mozilla-central/rev/57628a2c33d6
https://hg.mozilla.org/mozilla-central/rev/9870763b6a4c
https://hg.mozilla.org/mozilla-central/rev/958a6ec2d7c0
https://hg.mozilla.org/mozilla-central/rev/a54aca4aa8c9
https://hg.mozilla.org/mozilla-central/rev/c8029c072776
https://hg.mozilla.org/mozilla-central/rev/9acdf39431fc