Last Comment Bug 672893 - Don't #include jscompartment in xpconnect
: Don't #include jscompartment in xpconnect
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks: 554088
  Show dependency treegraph
 
Reported: 2011-07-20 12:03 PDT by David Anderson [:dvander]
Modified: 2012-01-25 07:24 PST (History)
14 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (15.21 KB, patch)
2011-07-27 13:39 PDT, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
v2 (17.79 KB, patch)
2011-07-31 23:08 PDT, Jason Orendorff [:jorendorff]
cdleary: review+
Details | Diff | Splinter Review

Description David Anderson [:dvander] 2011-07-20 12:03:08 PDT
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 2011-07-20 12:26:43 PDT
<cdleary> $ python -c 'import random; print random.choice("jorendorff, dvander, Waldo, luke, bhackett, cdleary, billm".split(", "))'
<cdleary> jorendorff
Comment 2 Jason Orendorff [:jorendorff] 2011-07-20 12:37:25 PDT
Yup. I'll do this next week.
Comment 3 Jason Orendorff [:jorendorff] 2011-07-27 13:39:36 PDT
Created attachment 548899 [details] [diff] [review]
v1
Comment 4 Luke Wagner [:luke] 2011-07-29 14:58:34 PDT
It seemed like "winning" was ultimately that jscompartment.h could be removed from INSTALLED_HEADERS in Makefile.in.
Comment 5 Jason Orendorff [:jorendorff] 2011-07-30 20:44:16 PDT
Comment on attachment 548899 [details] [diff] [review]
v1

Withdrawing review, Luke's right.
Comment 6 Jason Orendorff [:jorendorff] 2011-07-31 23:08:00 PDT
Created attachment 549721 [details] [diff] [review]
v2

There.
Comment 7 Luke Wagner [:luke] 2011-08-01 08:35:38 PDT
Much obliged.
Comment 8 Chris Leary [:cdleary] (not checking bugmail) 2011-08-01 10:17:06 PDT
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] 2011-08-01 16:53:45 PDT
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] (remove +bmo to email) 2011-08-02 10:11:50 PDT
Mission creep!

And do keep rolling those dice, please, whenever a pain point gets hit.
Comment 11 Jason Orendorff [:jorendorff] 2011-08-06 08:42:39 PDT
(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] 2011-08-06 14:16:59 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/66845f1a3aad
Comment 13 Jason Orendorff [:jorendorff] 2011-08-06 14:43:07 PDT
(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) 2011-08-06 15:41:02 PDT
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] 2011-08-06 18:28:00 PDT
(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) 2011-08-06 21:22:49 PDT
(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 2011-08-07 04:16:47 PDT
> jscntxt.h

Filed bug 677079.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-08 05:44:09 PDT
http://hg.mozilla.org/mozilla-central/rev/66845f1a3aad
Comment 19 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-08-08 05:44:46 PDT
And then this was backed out

http://hg.mozilla.org/mozilla-central/rev/53af4a6b8965
Comment 20 Jason Orendorff [:jorendorff] 2011-09-01 10:01:42 PDT
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) 2011-09-01 10:17:01 PDT
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] 2011-09-01 15:18:49 PDT
Can we just make cx->compartment private?
Comment 23 Jason Orendorff [:jorendorff] 2011-09-02 16:30:54 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/3326454d70f5

Filed bug 684400 to make cx->compartment private.
Comment 24 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-09-03 03:26:46 PDT
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] 2011-09-08 12:56:47 PDT
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] 2011-09-08 13:07:34 PDT
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) 2011-09-08 13:16:38 PDT
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] 2011-09-09 14:54:54 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/09e96590b9de
Comment 29 Justin Wood (:Callek) 2011-09-09 22:44:27 PDT
(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 [:emorley] 2012-01-24 13:10:11 PST
Followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ea41d0de5a04

Note You need to log in before you can comment on or make changes to this bug.