1283855 - Make most of JSAPI take JSContext* instead of JSRuntime*

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jan de Mooij [:jandem]]

Most APIs can now take JSContext instead of JSRuntime. It's nice because after that embedders only have to care about JSContext and no longer need a JSRuntime.

It's not clear if we can hide JSRuntime completely: some public GC inline functions use JSRuntime and there are some other challenges, we'll have to see how it works out.

Comment 1

[Jan de Mooij [:jandem]]

Attachment: Part 1 - JS_GetEmptyString

Comment 2

[Jan de Mooij [:jandem]]

Attachment: Part 2 - Some callbacks

Comment 3

[Jan de Mooij [:jandem]]

Attachment: Part 3 - More callbacks

Comment 4

[Jan de Mooij [:jandem]]

Attachment: Part 4 - Locale APIs

Waldo, if you're not comfortable reviewing the XPConnect bits please add an additional r?.

Comment 5

[Jan de Mooij [:jandem]]

Attachment: Part 5 - Warning reporter

Comment 6

[Jan de Mooij [:jandem]]

Attachment: Part 6 - JS_AbortIfWrongThread

Comment 7

[Jan de Mooij [:jandem]]

Attachment: Part 7 - JS_SetGCZeal

Comment 8

[Jan de Mooij [:jandem]]

Attachment: Part 8 - JIT compiler options, off-thread options

Comment 9

[Jan de Mooij [:jandem]]

Attachment: Part 9 - asm.js and build id ops

Comment 10

[Jan de Mooij [:jandem]]

Attachment: Part 10 - Some GC callbacks

Comment 11

[Jan de Mooij [:jandem]]

Attachment: Part 11 - More GC callbacks

The JS_GetContext calls are not great, but we can remove these later when we store JSContext instead of JSRuntime.

Comment 12

[Jan de Mooij [:jandem]]

Attachment: Part 12 - More GC APIs

Comment 13

[Jan de Mooij [:jandem]]

Attachment: Part 13 - JS_SetNativeStackQuota

This also removes a dead setNativeStackQuota method from devtools/shared/heapsnapshot/tests/gtest/DevTools.h

Comment 14

[Jan de Mooij [:jandem]]

Attachment: Part 14 - More callbacks

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8767183 [details] [diff] [review]
Part 9 - asm.js and build id ops

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

Thanks.

Comment 16

[Tooru Fujisawa [:arai]]

Comment on attachment 8767178 [details] [diff] [review]
Part 5 - Warning reporter

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

:)

Comment 17

[Jan de Mooij [:jandem]]

Attachment: Part 15 - {Get,Set}DOMCallbacks, SetWindowProxyClass

Not super happy with the GetJitContext call in IonBuilder, but it's not the first one and it's not too bad.

Comment 18

[Jan de Mooij [:jandem]]

Attachment: Part 16 - Source hook

Comment 19

[Jan de Mooij [:jandem]]

Attachment: Part 17 - {Get,Set}DebuggerMallocSizeOf

Comment 20

[Jan de Mooij [:jandem]]

Attachment: Part 18 - JS_DropPrincipals

Comment 21

[Jan de Mooij [:jandem]]

Attachment: Part 19 - Security callbacks

Comment 22

[Jan de Mooij [:jandem]]

Attachment: Part 20 - More principals code

Comment 23

[Jan de Mooij [:jandem]]

Attachment: Part 21 - PerformanceMonitoring APIs

Comment 24

[Jan de Mooij [:jandem]]

Attachment: Part 22 - GetErrorTypeName

Comment 25

[Jan de Mooij [:jandem]]

Attachment: Part 23 - More callbacks

Comment 26

[Jan de Mooij [:jandem]]

Attachment: Part 24 - DumpHeap

Comment 27

[Jan de Mooij [:jandem]]

Attachment: Part 25 - JS_GC

Comment 28

[Nick Fitzgerald [:fitzgen] [⏰PST; UTC-8]]

Comment on attachment 8767191 [details] [diff] [review]
Part 13 - JS_SetNativeStackQuota

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

r=me for js/ and devtools/ changes

The dom/ changes look fine as well, but I'm not sure if they need actual DOM peer stamps. Its a pretty straightforward change, so... *shrugs*

Comment 29

[Jan de Mooij [:jandem]]

Attachment: Part 26 - More GC APIs

Comment 30

[Jan de Mooij [:jandem]]

Attachment: Part 27 - More GC APIs

Comment 31

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8767190 [details] [diff] [review]
Part 12 - More GC APIs

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

As you said, the JS_GetContext in nsJSEnvironment.cpp are kind of unfortunate, but fine for now.

Comment 32

[Terrence Cole [:terrence]]

Comment on attachment 8767188 [details] [diff] [review]
Part 11 - More GC callbacks

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

Forwarding review to Andrew so that he'll be aware of these changes too.

Comment 33

[Terrence Cole [:terrence]]

Comment on attachment 8767218 [details] [diff] [review]
Part 26 - More GC APIs

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

GC changes look fine to me. Forwarding to Andrew again to okay the browser side.

Comment 34

[Andrew McCreight [:mccr8]]

Comment on attachment 8767188 [details] [diff] [review]
Part 11 - More GC callbacks

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

r=me for the non-js/ changes

::: dom/plugins/base/nsJSNPRuntime.cpp
@@ +338,5 @@
>    JSRuntime *jsRuntime = xpc::GetJSRuntime();
>    MOZ_ASSERT(jsRuntime != nullptr);
>  
>    // Register a callback to trace wrapped JSObjects.
> +  JSContext* cx = JS_GetContext(jsRuntime);

I'd just inline the definition of |jsRuntime| into here (and just delete the assertion) here and below, as it is not otherwise used. Really it feels like there should be some kind of public xpc::GetJSContext() but I'm not up on the subtleties there.

Comment 35

[Andrew McCreight [:mccr8]]

Comment on attachment 8767218 [details] [diff] [review]
Part 26 - More GC APIs

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

r- for the nsJSEnvironment changes. I'd like to see the updated patch. The rest of the non-js/ changes look fine to me.

::: dom/base/nsJSEnvironment.cpp
@@ +1201,5 @@
>    if (!nsContentUtils::XPConnect() || !sRuntime) {
>      return;
>    }
>  
> +  JSContext* cx = JS_GetContext(sRuntime);

I think it makes sense to create a new global variable sContext and use that, instead of patching in these JS_GetContext calls in a billion places in this file. Just set it appropriately right after each place that sRuntime is set.

Comment 36

[Jan de Mooij [:jandem]]

Attachment: Part 26 - More GC APIs

Fair enough. I was planning to rename sRuntime to sContext soon, but it's reasonable to add sContext now.

Comment 37

[Andrew McCreight [:mccr8]]

Comment on attachment 8767262 [details] [diff] [review]
Part 26 - More GC APIs

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

Thanks.

Comment 38

[Jan de Mooij [:jandem]]

Attachment: Part 15 - {Get,Set}DOMCallbacks, SetWindowProxyClass

This is slightly simpler.

Comment 39

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7cb219e5a1a8
part 1 - Make JS_GetEmptyString take JSContext instead of JSRuntime. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/007ccecf74c4
part 2 - Make some callback setters take JSContext instead of JSRuntime. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1be02c27400
part 5 - Make warning reporter APIs take JSContext instead of JSRuntime. r=arai
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b2279c28733
part 6 - Make JS_AbortIfWrongThread take JSContext instead of JSRuntime. r=terrence
https://hg.mozilla.org/integration/mozilla-inbound/rev/937ce4620f6b
part 7 - Make JS_SetGCZeal take JSContext instead of JSRuntime. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ca4097f77f
part 8 - Make JIT compiler option APIs take JSContext instead of JSRuntime. r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/78f84c1f58b4
part 9 - Make asm.js/buildId op setters take JSContext instead of JSRuntime. r=bbouvier

Comment 40

[Jon Coppeard (:jonco)]

Comment on attachment 8767222 [details] [diff] [review]
Part 27 - More GC APIs

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

::: dom/base/nsJSEnvironment.cpp
@@ +2447,5 @@
>  
>    // Let's make sure that our main thread is the same as the xpcom main thread.
>    MOZ_ASSERT(NS_IsMainThread());
>  
> +  sPrevGCSliceCallback = JS::SetGCSliceCallback(JS_GetContext(sRuntime), DOMGCSliceCallback);

Nit: this line is longer than 80 characters.

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d03d3e56b764
part 10 - Make some GC callback APIs take JSContext instead of JSRuntime. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/8dfcdfcdcfaf
part 11 - Make some GC callback APIs take JSContext instead of JSRuntime. r=terrence,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/57e0f9ee9ec4
part 12 - Make some GC APIs take JSContext instead of JSRuntime. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ad59028e26
part 13 - Make JS_SetNativeStackQuota take JSContext instead of JSRuntime. r=fitzgen
https://hg.mozilla.org/integration/mozilla-inbound/rev/7a08957a6aff
part 14 - Make more callback setters take JSContext instead of JSRuntime. r=shu
https://hg.mozilla.org/integration/mozilla-inbound/rev/cede06835eb1
part 15 - Make {Get,Set}DOMCallbacks, SetWindowProxyClass take JSContext instead of JSRuntime. r=h4writer

Comment 42

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7cb219e5a1a8
https://hg.mozilla.org/mozilla-central/rev/007ccecf74c4
https://hg.mozilla.org/mozilla-central/rev/d1be02c27400
https://hg.mozilla.org/mozilla-central/rev/4b2279c28733
https://hg.mozilla.org/mozilla-central/rev/937ce4620f6b
https://hg.mozilla.org/mozilla-central/rev/26ca4097f77f
https://hg.mozilla.org/mozilla-central/rev/78f84c1f58b4

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e94e4eb877c
part 16 - Make source hook functions take JSContext instead of JSRuntime. r=mrrrgn
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae90cded5407
part 18 - Make JS_DropPrincipals take JSContext instead of JSRuntime. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/a2d7ff2d4bd8
part 19 - Make security callbacks take JSContext instead of JSRuntime. r=jorendorff
https://hg.mozilla.org/integration/mozilla-inbound/rev/bdc9d8139086
part 20 - Make more principals code take JSContext instead of JSRuntime. r=luke
https://hg.mozilla.org/integration/mozilla-inbound/rev/eee13ff3e4d8
part 22 - Make GetErrorTypeName take JSContext instead of JSRuntime. r=arai

Comment 44

[Eric Faust [:efaust]]

Comment on attachment 8767176 [details] [diff] [review]
Part 3 - More callbacks

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

::: js/src/shell/js.cpp
@@ +709,1 @@
>                                             ForwardingPromiseRejectionTrackerCallback);

This still doesn't fit on one line, huh? Bummer.

Comment 45

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

https://hg.mozilla.org/mozilla-central/rev/d03d3e56b764
https://hg.mozilla.org/mozilla-central/rev/8dfcdfcdcfaf
https://hg.mozilla.org/mozilla-central/rev/57e0f9ee9ec4
https://hg.mozilla.org/mozilla-central/rev/04ad59028e26
https://hg.mozilla.org/mozilla-central/rev/7a08957a6aff
https://hg.mozilla.org/mozilla-central/rev/cede06835eb1
https://hg.mozilla.org/mozilla-central/rev/2e94e4eb877c
https://hg.mozilla.org/mozilla-central/rev/ae90cded5407
https://hg.mozilla.org/mozilla-central/rev/a2d7ff2d4bd8
https://hg.mozilla.org/mozilla-central/rev/bdc9d8139086
https://hg.mozilla.org/mozilla-central/rev/eee13ff3e4d8

Comment 46

[David Teller [:Yoric] - still alive but not very active]

Jan, can you confirm that there is a 1:1 mapping between JSContext and thread-running-JS? If so, you can turn this f+ in a r+.

Comment 47

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b09d648d9e6e
part 3 - Make some callback setters take JSContext instead of JSRuntime. r=efaust
https://hg.mozilla.org/integration/mozilla-inbound/rev/dadebac6b94d
part 17 - Make {Get,Set}DebuggerMallocSizeOf take JSContext instead of JSRuntime. r=jimb
https://hg.mozilla.org/integration/mozilla-inbound/rev/a07acf6d4de5
part 23 - Make more callback setters take JSContext instead of JSRuntime. r=evilpie
https://hg.mozilla.org/integration/mozilla-inbound/rev/e67c9acccc93
part 24 - Make js::DumpHeap take JSContext instead of JSRuntime. r=sfink
https://hg.mozilla.org/integration/mozilla-inbound/rev/9eb9bb173e50
part 25 - Make JS_GC take JSContext instead of JSRuntime. r=terrence

Comment 48

[Jan de Mooij [:jandem]]

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #46)
> Jan, can you confirm that there is a 1:1 mapping between JSContext and
> thread-running-JS? If so, you can turn this f+ in a r+.

Yup, that's correct.

Comment 49

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fdce8a8b920
part 21 - Make performance monitoring APIs take JSContext instead of JSRuntime. r=Yoric

Comment 50

[Jan de Mooij [:jandem]]

Attachment: Part 28 - More GC API changes

This covers (almost) all the remaining GC APIs. I tried to do this incrementally but patches modified a lot of the same code, so I folded them to make it easier to review.

I think this is the last patch for this bug - there's more to do but I'll do that in separate bugs because it's less trivial.

Comment 51

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a064d2e7a3c0
part 26 - Make more GC APIs take JSContext instead of JSRuntime. r=terrence,mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/d22e5cad510b
part 27 - Make more GC APIs take JSContext instead of JSRuntime. r=jonco
https://hg.mozilla.org/integration/mozilla-inbound/rev/c521197a29b7
part 28 - Make more GC APIs take JSContext instead of JSRuntime. r=terrence

Comment 52

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b09d648d9e6e
https://hg.mozilla.org/mozilla-central/rev/dadebac6b94d
https://hg.mozilla.org/mozilla-central/rev/a07acf6d4de5
https://hg.mozilla.org/mozilla-central/rev/e67c9acccc93
https://hg.mozilla.org/mozilla-central/rev/9eb9bb173e50
https://hg.mozilla.org/mozilla-central/rev/0fdce8a8b920

Comment 53

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

https://hg.mozilla.org/mozilla-central/rev/a064d2e7a3c0
https://hg.mozilla.org/mozilla-central/rev/d22e5cad510b
https://hg.mozilla.org/mozilla-central/rev/c521197a29b7

Comment 54

[Jan de Mooij [:jandem]]

Comment on attachment 8767177 [details] [diff] [review]
Part 4 - Locale APIs

Switching r? to bholley as it's XPConnect-ish and so I can maybe close this bug this week.

Comment 55

[Terrence Cole [:terrence]]

Comment on attachment 8767177 [details] [diff] [review]
Part 4 - Locale APIs

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

Stealing review. I haven't worked in this specific part of XPConnect before, but it's a simple enough patch that I don't think it matters too much.

Comment 56

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8c5ffe860
part 4 - Make localization APIs take JSContext instead of JSRuntime. r=terrence

Comment 57

[Carsten Book [:Tomcat]]

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

Comment 58

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d9c8c76813
OOMTest: Guard JS_DEFAULT_ZEAL_FREQ macro with JS_GC_ZEAL. r=jandem

Comment 59

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

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

Comment 60

[Aki Sasaki (not active)]

https://hg.mozilla.org/projects/date/rev/f3d9c8c76813e8119fe9e4bd1e46046e6e0f28c3
Bug 1283855 - OOMTest: Guard JS_DEFAULT_ZEAL_FREQ macro with JS_GC_ZEAL. r=jandem