672893 - Don't #include jscompartment in xpconnect

Status: RESOLVED FIXED

Description

[David Anderson [:dvander]]

jscompartment.h has a few struct members #ifdef'd on certain configure-time JIT options. These JIT flags are not passed down to XPConnect so the struct layout doesn't match. I hit this in IonMonkey and it took a while to track down:

JS_TRACE_MONITOR_FROM_CONTEXT_WITH_LAZY_INIT(JSContext *cx)

This function was seeing a NULL entry in xpc and a non-NULL entry in jstracer. How does this work for JS_TRACER then? There is a hack in xpconnect/src/Makefile.in:

> CONFIG := $(shell cat $(DEPTH)/js/src/js-confdefs.h)
> 
> ENABLE_JIT = $(filter JS_TRACER, $(CONFIG))
> ifneq (,$(ENABLE_JIT))
> 
> # Ugly! We need the AVMPLUS defines out of js/src/js-confdefs.h to make the
> # nanojit headers happy. Gotta figure out a better way to get them, bug 483677.
> DEFINES += \
>  -DJS_TRACER=1 \

This doesn't apply to all xpc files though, and there is no such hack for JS_METHODJIT, so I'm left wondering exactly which files are reading from JSCompartment and whether they just happen to be reading fields before traceMonitor/jaegerCompartment.

For IonMonkey I'm just putting ionCompartment at the bottom of the struct, but this will happen again someday so we should fix it.

Comment 1

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

<cdleary> $ python -c 'import random; print random.choice("jorendorff, dvander, Waldo, luke, bhackett, cdleary, billm".split(", "))'
<cdleary> jorendorff

Comment 2

[Jason Orendorff [:jorendorff]]

Yup. I'll do this next week.

Comment 3

[Jason Orendorff [:jorendorff]]

Created attachment 548899 [details] [diff] [review]
v1

Comment 4

[Luke Wagner [:luke]]

It seemed like "winning" was ultimately that jscompartment.h could be removed from INSTALLED_HEADERS in Makefile.in.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 548899 [details] [diff] [review]
v1

Withdrawing review, Luke's right.

Comment 6

[Jason Orendorff [:jorendorff]]

Created attachment 549721 [details] [diff] [review]
v2

There.

Comment 7

[Luke Wagner [:luke]]

Much obliged.

Comment 8

[Chris Leary [:cdleary] (not checking bugmail)]

Comment on attachment 549721 [details] [diff] [review]
v2

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

::: js/src/jsfriendapi.h
@@ +75,5 @@
> +#ifdef __cplusplus
> +
> +namespace JS {
> +
> +class JS_PUBLIC_API(AutoPreserveCompartment) {

This duplication is unfortunate -- can we typedef these guys to the js::PreserveCompartment and js::SwitchToCompartment within jscompartment.h?

::: js/src/xpconnect/wrappers/AccessCheck.cpp
@@ +57,4 @@
>  GetCompartmentPrincipal(JSCompartment *compartment)
>  {
> +    JSPrincipals *prin = JS_GetCompartmentPrincipals(compartment);
> +    return static_cast<nsJSPrincipals *>(prin)->nsIPrincipalPtr;

Why don't we need to preserve the NULL check here?

Comment 9

[Luke Wagner [:luke]]

I just remembered that I was trying to remove jsgc.h from INSTALLED_HEADERS and jscompartment.h is what stopped me.  Any chance jsgc.h can go too with this patch?  (Maybe we should just roll the dice again :)

Comment 10

[Jeff Walden [:Waldo] (not taking new requests -- poke me on IRC if it's important)]

Mission creep!

And do keep rolling those dice, please, whenever a pain point gets hit.

Comment 11

[Jason Orendorff [:jorendorff]]

(In reply to Chris Leary [:cdleary] from comment #8)
> ::: js/src/jsfriendapi.h
> @@ +75,5 @@
> > +#ifdef __cplusplus
> > +
> > +namespace JS {
> > +
> > +class JS_PUBLIC_API(AutoPreserveCompartment) {
> 
> This duplication is unfortunate -- can we typedef these guys to the
> js::PreserveCompartment and js::SwitchToCompartment within jscompartment.h?

I was trying to make a real API layer. Those classes have inline members that require including jscntxt.h.

I considered deleting the inline versions, but AtomizeInline uses SwitchToCompartment.

I don't think we can have both versions for less code than this.

> ::: js/src/xpconnect/wrappers/AccessCheck.cpp
> @@ +57,4 @@
> >  GetCompartmentPrincipal(JSCompartment *compartment)
> >  {
> > +    JSPrincipals *prin = JS_GetCompartmentPrincipals(compartment);
> > +    return static_cast<nsJSPrincipals *>(prin)->nsIPrincipalPtr;
> 
> Why don't we need to preserve the NULL check here?

Oh. Took me a minute to figure out what you meant because Splinter helpfully omitted the - line, but yes, we probably do need it. Added it back.

Comment 12

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/66845f1a3aad

Comment 13

[Jason Orendorff [:jorendorff]]

(In reply to comment #9)
> I just remembered that I was trying to remove jsgc.h from INSTALLED_HEADERS
> and jscompartment.h is what stopped me.  Any chance jsgc.h can go too with
> this patch?  (Maybe we should just roll the dice again :)

I took a look, and jsgc.h can't be un-INSTALLED until jscntxt.h is. That'll be a really annoying job, as jscntxt.h is included from literally dozens of files all through dom/, content/, and jetpack/. But I would happily roll the dice again for it.

Comment 14

[Phil Ringnalda (:philor)(back in August)]

Backed out in http://hg.mozilla.org/integration/mozilla-inbound/rev/53af4a6b8965 for debug startup crashes in the precompile make package step, like tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312665018.1312666270.27207.gz

Comment 15

[Luke Wagner [:luke]]

(In reply to Jason Orendorff [:jorendorff] from comment #13)
Mmmm, jscntxt.h; there be the real dragon to slay.  I guess that'll entail adding real auto-rooters to jsapi.h, something we've needed to do for a long time.  I'll roll.

Comment 16

[Bill McCloskey (:billm)]

(In reply to Luke Wagner [:luke] from comment #15)
> Mmmm, jscntxt.h; there be the real dragon to slay.  I guess that'll entail
> adding real auto-rooters to jsapi.h, something we've needed to do for a long
> time.  I'll roll.

I'm definitely in for that one.

Comment 17

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

> jscntxt.h

Filed bug 677079.

Comment 18

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

http://hg.mozilla.org/mozilla-central/rev/66845f1a3aad

Comment 19

[Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary)]

And then this was backed out

http://hg.mozilla.org/mozilla-central/rev/53af4a6b8965

Comment 20

[Jason Orendorff [:jorendorff]]

I haven't forgotten about this. I keep bouncing it off the try server and there is always something horribly wrong. The latest thing is that the browser always crashes under JSObject::makeLazyType.

http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=336699bf2514

Comment 21

[Brian Hackett (:bhackett)]

The writes to cx->compartment need to be wrapped with cx->setCompartment, which maintains inference-enabled state for the cx.  cx->compartment should I think have accessor functions, or at least have a consistency assertion in JSContext::typeInferenceEnabled (sorry).

Comment 22

[Luke Wagner [:luke]]

Can we just make cx->compartment private?

Comment 23

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/3326454d70f5

Filed bug 684400 to make cx->compartment private.

Comment 24

[Marco Bonardo [::mak]]

backed out Bug 677957 and Bug 672893 since one of these caused Android mochitest 5 permaorange in test_jQuery.html and I couldn't identify the culprit at first glance.

1120 INFO TEST-PASS | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | fx module: JS 0 to show - Height must be reset to 0: 0px: 0
1121 INFO TEST-PASS | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | fx module: JS 0 to show - Opacity must be reset to 0: 0: 0
1122 INFO TEST-PASS | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | fx module: JS 0 to show - Make sure height is auto.
1123 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | Test timed out.
1124 INFO TEST-END | /tests/dom/tests/mochitest/ajax/jquery/test_jQuery.html | finished in 300232ms

Comment 25

[Jason Orendorff [:jorendorff]]

I refreshed this patch and asked the Try Server to run it on Android:
  https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=4266f14667c3

8 orange, 3 purple. Android always looks like that to me though. I found no evidence test_jQuery.html failed, but who knows what this means. I asked what to do about this in #developers. No response. Currently asking around pretty much at random to see what I should do about this.

Comment 26

[Andrew McCreight [:mccr8]]

I usually rebuild individual failing tests on Android if I'm nervous.  Though I have had a Talos test fail 3 times in a row, so that may not always be helpful.

Comment 27

[Bill McCloskey (:billm)]

It looks like test_jQuery passed on try, so why not reland? The perma-orange may have been caused by bug 677957. I guess you could push that bug to try for extra assurance (that it fails), but it seems unnecessary.

Comment 28

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/09e96590b9de

Comment 29

[Justin Wood (:Callek)]

(In reply to Jason Orendorff [:jorendorff] from comment #28)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/09e96590b9de

now on m-c

Comment 30

[Ed Morley (Away 27th-2nd) [:emorley]]

Followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea41d0de5a04

Comment 31

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

Followup was backed out and relanded, and now merged.

https://hg.mozilla.org/mozilla-central/rev/b7f926cfa8c8
https://hg.mozilla.org/mozilla-central/rev/5e9adf3343de
https://hg.mozilla.org/mozilla-central/rev/ea41d0de5a04