Closed
Bug 672893
Opened 14 years ago
Closed 14 years ago
Don't #include jscompartment in xpconnect
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla9
People
(Reporter: dvander, Assigned: jorendorff)
References
Details
Attachments
(1 file, 1 obsolete file)
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.
Comment 1•14 years ago
|
||
<cdleary> $ python -c 'import random; print random.choice("jorendorff, dvander, Waldo, luke, bhackett, cdleary, billm".split(", "))'
<cdleary> jorendorff
Assignee: general → jorendorff
Assignee | ||
Comment 2•14 years ago
|
||
Yup. I'll do this next week.
Assignee | ||
Comment 3•14 years ago
|
||
Attachment #548899 -
Flags: review?(cdleary)
![]() |
||
Comment 4•14 years ago
|
||
It seemed like "winning" was ultimately that jscompartment.h could be removed from INSTALLED_HEADERS in Makefile.in.
Assignee | ||
Comment 5•14 years ago
|
||
Comment on attachment 548899 [details] [diff] [review]
v1
Withdrawing review, Luke's right.
Attachment #548899 -
Flags: review?(cdleary)
Assignee | ||
Comment 6•14 years ago
|
||
There.
Attachment #548899 -
Attachment is obsolete: true
Attachment #549721 -
Flags: review?(cdleary)
![]() |
||
Comment 7•14 years ago
|
||
Much obliged.
Comment 8•14 years ago
|
||
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+
![]() |
||
Comment 9•14 years ago
|
||
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•14 years ago
|
||
Mission creep!
And do keep rolling those dice, please, whenever a pain point gets hit.
Assignee | ||
Comment 11•14 years ago
|
||
(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.
Assignee | ||
Comment 12•14 years ago
|
||
Whiteboard: [inbound]
Assignee | ||
Comment 13•14 years ago
|
||
(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•14 years ago
|
||
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]
![]() |
||
Comment 15•14 years ago
|
||
(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.
Comment 17•14 years ago
|
||
> jscntxt.h
Filed bug 677079.
Status: NEW → RESOLVED
Closed: 14 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 → ---
Assignee | ||
Comment 20•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Can we just make cx->compartment private?
Assignee | ||
Comment 23•14 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/3326454d70f5
Filed bug 684400 to make cx->compartment private.
Whiteboard: [inbound]
Comment 24•14 years ago
|
||
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]
Updated•14 years ago
|
Target Milestone: mozilla8 → ---
Assignee | ||
Comment 25•14 years ago
|
||
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•14 years ago
|
||
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.
Assignee | ||
Comment 28•14 years ago
|
||
Whiteboard: [inbound]
Comment 29•14 years ago
|
||
(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: 14 years ago → 14 years ago
Resolution: --- → FIXED
Updated•14 years ago
|
Target Milestone: --- → mozilla9
Comment 30•13 years ago
|
||
Comment 31•13 years ago
|
||
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
Whiteboard: [inbound]
You need to log in
before you can comment on or make changes to this bug.
Description
•