Closed Bug 672893 Opened 9 years ago Closed 9 years ago

Don't #include jscompartment in xpconnect

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla9

People

(Reporter: dvander, Assigned: jorendorff)

References

Details

Attachments

(1 file, 1 obsolete file)

v2
17.79 KB, patch
cdleary
: review+
Details | Diff | Splinter Review 
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
Assignee: general → jorendorff
Yup. I'll do this next week.
Attached patch v1 (obsolete) — — Splinter Review
Attachment #548899 - Flags: review?(cdleary)
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.
Attachment #548899 - Flags: review?(cdleary)
Attached patch v2 — — Splinter Review 
There.
Attachment #548899 - Attachment is obsolete: true
Attachment #549721 - Flags: review?(cdleary)
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?
Attachment #549721 - Flags: review?(cdleary) → review+
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.
(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
Whiteboard: [inbound]
(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
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
And then this was backed out

http://hg.mozilla.org/mozilla-central/rev/53af4a6b8965
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Whiteboard: [inbound]
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
Whiteboard: [inbound]
Target Milestone: mozilla8 → ---
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.
(In reply to Jason Orendorff [:jorendorff] from comment #28)
> http://hg.mozilla.org/integration/mozilla-inbound/rev/09e96590b9de

now on m-c
Status: REOPENED → RESOLVED
Closed: 9 years ago → 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
You need to log in before you can comment on or make changes to this bug.