Closed Bug 1282150 Opened 4 years ago Closed 4 years ago

Rename all our JSContext stuff to be nicer

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

Attachments

(6 files)

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.
Blocks: 1281886
Depends on: 767938
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8765073 - Flags: review?(bobbyholley)
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.
(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.
Attachment #8765068 - Flags: review?(bobbyholley) → review+
Attachment #8765069 - Flags: review?(bobbyholley) → review+
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.
Attachment #8765070 - Flags: review?(bobbyholley) → review+
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.
Attachment #8765071 - Flags: review?(bobbyholley) → review+
Comment on attachment 8765072 [details] [diff] [review]
part 5.  Kill off all the unused GetJSContextForEventHandlers cruft

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

\o/
Attachment #8765072 - Flags: review?(bobbyholley) → review+
Comment on attachment 8765073 [details] [diff] [review]
part 6.  Get rid of GetSafeJSContext

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

\o/
Attachment #8765073 - Flags: review?(bobbyholley) → review+
(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).
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.
(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?
> 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.
Flags: needinfo?(jdemooij)
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?
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...
Flags: needinfo?(jdemooij)
Pushed by bzbarsky@mozilla.com:
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
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.
Flags: needinfo?(bzbarsky)
Grrr.  Stupid false positive.  Fixing.
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9acdf39431fc
followup: placate the rooting analysis in SimpleGlobalObject::Create so we can reopen the CLOSED TREE
Flags: needinfo?(bzbarsky)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.