Make most of JSAPI take JSContext* instead of JSRuntime*

RESOLVED FIXED in Firefox 50

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: jandem, Assigned: jandem)

Tracking

unspecified
mozilla50
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(28 attachments, 2 obsolete attachments)

2.10 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
12.14 KB, patch
luke
: review+
Details | Diff | Splinter Review
13.50 KB, patch
efaust
: review+
Details | Diff | Splinter Review
14.95 KB, patch
terrence
: review+
Details | Diff | Splinter Review
14.74 KB, patch
arai
: review+
Details | Diff | Splinter Review
7.73 KB, patch
terrence
: review+
Details | Diff | Splinter Review
10.87 KB, patch
sfink
: review+
Details | Diff | Splinter Review
7.14 KB, patch
nbp
: review+
Details | Diff | Splinter Review
6.78 KB, patch
bbouvier
: review+
Details | Diff | Splinter Review
13.84 KB, patch
jonco
: review+
Details | Diff | Splinter Review
6.60 KB, patch
terrence
: review+
mccr8
: review+
Details | Diff | Splinter Review
25.25 KB, patch
sfink
: review+
Details | Diff | Splinter Review
11.61 KB, patch
fitzgen
: review+
Details | Diff | Splinter Review
10.67 KB, patch
shu
: review+
Details | Diff | Splinter Review
4.73 KB, patch
mrrrgn
: review+
Details | Diff | Splinter Review
6.15 KB, patch
jimb
: review+
Details | Diff | Splinter Review
7.22 KB, patch
luke
: review+
Details | Diff | Splinter Review
7.28 KB, patch
jorendorff
: review+
Details | Diff | Splinter Review
10.73 KB, patch
luke
: review+
Details | Diff | Splinter Review
14.76 KB, patch
Yoric
: review+
Details | Diff | Splinter Review
3.47 KB, patch
arai
: review+
Details | Diff | Splinter Review
5.28 KB, patch
evilpie
: review+
Details | Diff | Splinter Review
3.71 KB, patch
sfink
: review+
Details | Diff | Splinter Review
36.58 KB, patch
terrence
: review+
Details | Diff | Splinter Review
22.09 KB, patch
jonco
: review+
Details | Diff | Splinter Review
30.19 KB, patch
mccr8
: review+
Details | Diff | Splinter Review
7.75 KB, patch
h4writer
: review+
Details | Diff | Splinter Review
55.46 KB, patch
terrence
: review+
Details | Diff | Splinter Review
Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
Attachment #8767173 - Flags: review?(jorendorff)
Assignee

Comment 2

3 years ago
Attachment #8767174 - Flags: review?(luke)
Assignee

Comment 3

3 years ago
Attachment #8767176 - Flags: review?(efaustbmo)
Assignee

Comment 4

3 years ago
Waldo, if you're not comfortable reviewing the XPConnect bits please add an additional r?.
Attachment #8767177 - Flags: review?(jwalden+bmo)
Assignee

Comment 5

3 years ago
Attachment #8767178 - Flags: review?(arai.unmht)
Assignee

Comment 6

3 years ago
Attachment #8767179 - Flags: review?(terrence)
Assignee

Comment 7

3 years ago
Attachment #8767180 - Flags: review?(sphink)
Assignee

Comment 8

3 years ago
Attachment #8767181 - Flags: review?(nicolas.b.pierron)
Assignee

Comment 9

3 years ago
Attachment #8767183 - Flags: review?(bbouvier)
Assignee

Comment 10

3 years ago
Attachment #8767186 - Flags: review?(jcoppeard)
Assignee

Comment 11

3 years ago
The JS_GetContext calls are not great, but we can remove these later when we store JSContext instead of JSRuntime.
Attachment #8767188 - Flags: review?(terrence)
Assignee

Comment 12

3 years ago
Attachment #8767190 - Flags: review?(sphink)
Assignee

Comment 13

3 years ago
This also removes a dead setNativeStackQuota method from devtools/shared/heapsnapshot/tests/gtest/DevTools.h
Attachment #8767191 - Flags: review?(nfitzgerald)
Assignee

Comment 14

3 years ago
Attachment #8767192 - Flags: review?(shu)
Comment on attachment 8767183 [details] [diff] [review]
Part 9 - asm.js and build id ops

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

Thanks.
Attachment #8767183 - Flags: review?(bbouvier) → review+
Comment on attachment 8767178 [details] [diff] [review]
Part 5 - Warning reporter

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

:)
Attachment #8767178 - Flags: review?(arai.unmht) → review+
Attachment #8767186 - Flags: review?(jcoppeard) → review+
Assignee

Comment 17

3 years ago
Not super happy with the GetJitContext call in IonBuilder, but it's not the first one and it's not too bad.
Attachment #8767199 - Flags: review?(hv1989)
Assignee

Comment 18

3 years ago
Attachment #8767200 - Flags: review?(winter2718)
Assignee

Comment 20

3 years ago
Attachment #8767204 - Flags: review?(luke)
Assignee

Comment 21

3 years ago
Attachment #8767207 - Flags: review?(jorendorff)
Assignee

Comment 22

3 years ago
Attachment #8767209 - Flags: review?(luke)
Assignee

Comment 23

3 years ago
Attachment #8767210 - Flags: review?(dteller)
Assignee

Comment 24

3 years ago
Attachment #8767211 - Flags: review?(arai.unmht)
Assignee

Comment 25

3 years ago
Attachment #8767212 - Flags: review?(evilpies)
Assignee

Comment 26

3 years ago
Attachment #8767213 - Flags: review?(sphink)
Assignee

Comment 27

3 years ago
Attachment #8767214 - Flags: review?(terrence)
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*
Attachment #8767191 - Flags: review?(nfitzgerald) → review+
Assignee

Comment 29

3 years ago
Posted patch Part 26 - More GC APIs (obsolete) — Splinter Review
Attachment #8767218 - Flags: review?(terrence)

Updated

3 years ago
Attachment #8767174 - Flags: review?(luke) → review+
Assignee

Comment 30

3 years ago
Attachment #8767222 - Flags: review?(jcoppeard)

Updated

3 years ago
Attachment #8767204 - Flags: review?(luke) → review+

Updated

3 years ago
Attachment #8767209 - Flags: review?(luke) → review+
Attachment #8767211 - Flags: review?(arai.unmht) → review+
Attachment #8767173 - Flags: review?(jorendorff) → review+
Attachment #8767207 - Flags: review?(jorendorff) → review+
Attachment #8767180 - Flags: review?(sphink) → review+
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.
Attachment #8767190 - Flags: review?(sphink) → review+
Attachment #8767213 - Flags: review?(sphink) → review+
Attachment #8767179 - Flags: review?(terrence) → review+
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.
Attachment #8767188 - Flags: review?(terrence)
Attachment #8767188 - Flags: review?(continuation)
Attachment #8767188 - Flags: review+
Attachment #8767214 - Flags: review?(terrence) → review+
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.
Attachment #8767218 - Flags: review?(terrence)
Attachment #8767218 - Flags: review?(continuation)
Attachment #8767218 - Flags: review+
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.
Attachment #8767188 - Flags: review?(continuation) → review+
Attachment #8767200 - Flags: review?(winter2718) → review+
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.
Attachment #8767218 - Flags: review?(continuation) → review-
Assignee

Comment 36

3 years ago
Fair enough. I was planning to rename sRuntime to sContext soon, but it's reasonable to add sContext now.
Attachment #8767218 - Attachment is obsolete: true
Attachment #8767262 - Flags: review?(continuation)
Comment on attachment 8767262 [details] [diff] [review]
Part 26 - More GC APIs

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

Thanks.
Attachment #8767262 - Flags: review?(continuation) → review+

Updated

3 years ago
Attachment #8767192 - Flags: review?(shu) → review+
Assignee

Comment 38

3 years ago
This is slightly simpler.
Attachment #8767199 - Attachment is obsolete: true
Attachment #8767199 - Flags: review?(hv1989)
Attachment #8767398 - Flags: review?(hv1989)
Attachment #8767181 - Flags: review?(nicolas.b.pierron) → review+
Attachment #8767212 - Flags: review?(evilpies) → review+
Attachment #8767398 - Flags: review?(hv1989) → review+

Comment 39

3 years ago
Pushed by jandemooij@gmail.com:
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
Assignee

Updated

3 years ago
Keywords: leave-open
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.
Attachment #8767222 - Flags: review?(jcoppeard) → review+

Comment 41

3 years ago
Pushed by jandemooij@gmail.com:
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 43

3 years ago
Pushed by jandemooij@gmail.com:
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

Updated

3 years ago
Attachment #8767203 - Flags: review?(jimb) → review+
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.
Attachment #8767176 - Flags: review?(efaustbmo) → review+
Attachment #8767210 - Flags: review?(dteller) → feedback+
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+.
Flags: needinfo?(jdemooij)

Comment 47

3 years ago
Pushed by jandemooij@gmail.com:
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
Assignee

Comment 48

3 years ago
(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.
Flags: needinfo?(jdemooij)
Attachment #8767210 - Flags: feedback+ → review+

Comment 49

3 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0fdce8a8b920
part 21 - Make performance monitoring APIs take JSContext instead of JSRuntime. r=Yoric
Assignee

Comment 50

3 years ago
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.
Attachment #8768433 - Flags: review?(terrence)
Attachment #8768433 - Flags: review?(terrence) → review+

Comment 51

3 years ago
Pushed by jandemooij@gmail.com:
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
Assignee

Comment 54

3 years ago
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.
Attachment #8767177 - Flags: review?(jwalden+bmo) → review?(bobbyholley)
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.
Attachment #8767177 - Flags: review?(bobbyholley) → review+

Comment 56

3 years ago
Pushed by jandemooij@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d8c5ffe860
part 4 - Make localization APIs take JSContext instead of JSRuntime. r=terrence
Assignee

Updated

3 years ago
Keywords: leave-open
Assignee

Updated

3 years ago
Summary: Make JSAPI take JSContext* instead of JSRuntime* → Make most of JSAPI take JSContext* instead of JSRuntime*

Comment 57

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/e8d8c5ffe860
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50

Comment 58

2 years ago
Pushed by npierron@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3d9c8c76813
OOMTest: Guard JS_DEFAULT_ZEAL_FREQ macro with JS_GC_ZEAL. r=jandem
You need to log in before you can comment on or make changes to this bug.