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.
<cdleary> $ python -c 'import random; print random.choice("jorendorff, dvander, Waldo, luke, bhackett, cdleary, billm".split(", "))' <cdleary> jorendorff
Yup. I'll do this next week.
Created attachment 548899 [details] [diff] [review] v1
It seemed like "winning" was ultimately that jscompartment.h could be removed from INSTALLED_HEADERS in Makefile.in.
Comment on attachment 548899 [details] [diff] [review] v1 Withdrawing review, Luke's right.
Created attachment 549721 [details] [diff] [review] v2 There.
Much obliged.
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?
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 :)
Mission creep! And do keep rolling those dice, please, whenever a pain point gets hit.
(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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/66845f1a3aad
(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.
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
(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.
(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.
> jscntxt.h Filed bug 677079.
http://hg.mozilla.org/mozilla-central/rev/66845f1a3aad
And then this was backed out http://hg.mozilla.org/mozilla-central/rev/53af4a6b8965
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
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).
Can we just make cx->compartment private?
http://hg.mozilla.org/integration/mozilla-inbound/rev/3326454d70f5 Filed bug 684400 to make cx->compartment private.
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
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.
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.
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/09e96590b9de
(In reply to Jason Orendorff [:jorendorff] from comment #28) > http://hg.mozilla.org/integration/mozilla-inbound/rev/09e96590b9de now on m-c
Followup: https://hg.mozilla.org/integration/mozilla-inbound/rev/ea41d0de5a04
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
or
