677079 - Remove jscntxt.h from INSTALLED_HEADERS

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

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

See bug 672893 comment 13 and following.

Besides bug 332648, content/xul/document/src/nsXULContentSink.cpp and content/base/src/nsScriptLoader.cpp need js::VersionFlags::HAS_XML or VersionHasXML/VersionSetXML.

:jorendorff, :luke, and :billm are ready to take a 1/6 chance of having to fix this bug in order to get it fixed; three more heroes are required.

Comment 1

[Brian Hackett [Laid off!]]

I volunteer.  Actually, I auto-volunteer for bugs removing any of the installed headers except js*api.h

Comment 2

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

Volunteer.

Comment 3

[Andreas Gal :gal]

Volunteer. Non-API includes must die.

Comment 4

[Jeff Walden [:Waldo]]

"Give me the victim, give me the victim!"

http://www.youtube.com/watch?v=Qxys99Wn_NE#t=1m23s

I am happy to volunteer, with the caveat that I think I need to finish bug 586842 before doing this, should I be the lucky victim.

Comment 5

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

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

(I'll be removing one include in bug 677101, fwiw.)

Comment 6

[Jeff Walden [:Waldo]]

Well then!  I guess that caveat will actually matter.  (Unless we were waiting for more people to volunteer?  It wasn't clear to me that everyone had a chance to see/volunteer, not in the space of eight hours on a Sunday morning.)

I will probably be busy with that for most, if not all, of the time until August 18, when I disappear for a couple weeks of vacation.  So it's unlikely this bug will see much progress before September.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

I had an odd urge to fix this last night, so here it is.

I definitely cheated here. Although jscntxt.h is no longer part of INSTALLED_HEADERS, I whitelisted several directories by added js/src to LOCAL_INCLUDES in their Makefiles. The directories are XPConnect, jsd, and dom/base. The reason for the last one is that nsGlobalWindow.cpp defines a wrapper, and I couldn't think of a good way to disentangle the wrapper code from jscntxt.h.

Nevertheless, I think the patch is valuable. It should shorten build times and it gets us closer to the goal of a real API. The main problems that we still have to tackle are wrappers and typed arrays. I'm worried that both of these will be difficult to fix.

Comment 9

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

Comment on attachment 562206 [details] [diff] [review]
patch

>--- a/dom/workers/ListenerManager.cpp
>+++ b/dom/workers/ListenerManager.cpp

>+  nsTArray<jsval> listeners;
>-    if ((eventIsTrusted || listener->mWantsUntrusted) &&
>-        !listeners.append(listener->mListenerVal)) {
>+    if (eventIsTrusted || listener->mWantsUntrusted) {
>       return false;
>     }
>+    listeners.AppendElement(listener->mListenerVal);

If you use nsTArray infallibly, can you please make it an InfallibleTArray for clarity?

>   }
> 
>-  if (listeners.empty()) {
>+  if (!listeners.Length()) {

IsEmpty()

>--- a/js/src/Makefile.in
>+++ b/js/src/Makefile.in
>-		jscntxt.h \
>-		jscrashreport.h \
>-		jsdate.h \
>-		jsemit.h \
>-		jsfun.h \
>-		jsgc.h \
>-		jsgcchunk.h \
>-		jsgcstats.h \
>-		jsinterp.h \
>-		jsiter.h \
>-		jsparse.h \
>-		jsproxy.h \
>-		jsreflect.h \
>-		jsscan.h \
>-		jsscope.h \
>-		jstracer.h \
>-		jswrapper.h \

\o/

>-/* FIXME(bug 332648): Move this into a public header. */

Don't forget to close that bug.

>--- a/js/src/jsdbgapi.h
>+++ b/js/src/jsdbgapi.h
>+#include "jsprvtd.h"
> #include "jsopcode.h"
>-#include "jsprvtd.h"

Why this change?

>--- a/js/src/jsfriendapi.cpp
>+++ b/js/src/jsfriendapi.cpp
>+JS_FRIEND_API(JSVersion)
>+JS_VersionSetXML(JSVersion version, JSBool enable)
>+{
>+    js::VersionSetXML(&version, enable);

Any reason you didn't match the internal signature?


I wonder if nsXPConnect should go into a public header... That seems like a better approach than duplicating those static functions in mozilla::xpconnect. (In any case, Gecko should use those as xpconnect::Foo(), with a 'using namespace mozilla;' at the top of .cpps that aren't yet using namespaces.)

I'm not entirely sure I like all of this patch, but I do think there are a lot of improvements in it. I'll leave it to your reviewer to decide whether it's better to add the LOCAL_INCLUDES or to keep the file in INSTALLED_HEADERS for now, while we take the rest of the patch.

It's rather big, though, and I think the js_GetErrorMessage->JS_GetErrorMessage should definitely go in a separate patch, and perhaps AutoGCRooter and friends as well (if only for ease of review).

Comment 10

[Jeff Walden [:Waldo]]

Comment on attachment 562206 [details] [diff] [review]
patch

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

So I mentioned the INSTALLED_HEADERS->LOCAL_INCLUDES thing to bhackett, and he thought it smelled a bit dodgy, too, even as a halfway step (especially since that opens Pandora's box for all the internal headers, not just for a small subset of them).  He also said you'd agreed to defer this to later, when dependencies can be cut more thoroughly, which makes sense to me.  So I'm going to cancel this review for now, and hopefully next time around we won't need this one-step-forward-one-and-a-half-steps-back approach.

Comment 11

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

Shouldn't we land the other parts of the patch, though? Even if we can't remove jscntxt.h from INSTALLED_HEADERS just yet, moving in that direction would be useful already.

Comment 12

[Jeff Walden [:Waldo]]

I would probably be in favor of various other parts of the patch, but given all the parts are intermingled, it's hard to review and be sure which parts are needed for which changes.  I'm happy to review smaller, self-contained parts in new patches.

Comment 13

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: autorooters patch

I'm updating the this to completely remove jscntxt.h. This first patch just fixes up the autorooters. I put most of the autorooters in jsapi.h. The ones that depend on vectors I put in jsfriendapi.h. I also changed the autorooter implementation to use virtual function dispatch rather than the tagging scheme; I think this is cleaner.

Comment 14

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: main patch

This patch mostly just adds a bunch of shadow declarations and accessors to jsfriendapi.h. This parallel's Brian's approach. There are a few other things here, but they're pretty local.

The one thing that might need explanation in the GC trigger stuff from nsJSEnvironment. Previously that code had been reading out of cx->rt->gcTriggerCompartment to figure out if we were doing a compartment GC. This was wrong, and it always got NULL (so it though every GC was a full GC). I fixed this by adding a new flag that is set correctly by the GC.

Comment 16

[Jeff Walden [:Waldo]]

Comment on attachment 564998 [details] [diff] [review]
autorooters patch

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

::: js/src/jsapi.h
@@ +4840,1 @@
>  JS_END_EXTERN_C

Let's move this above this entire section, then we can avoid having oddnesses like this:

#ifdef __cplusplus
JS_END_EXTERN_C

...

JS_BEGIN_EXTERN_C
#endif /* __cplusplus */

/* yes, really there's *nothing in this space */

JS_END_EXTERN_C

@@ +4859,5 @@
> +        JS_ASSERT(this != reinterpret_cast<shadow::Context *>(cx)->autoGCRooters);
> +        reinterpret_cast<shadow::Context *>(cx)->autoGCRooters = this;
> +    }
> +
> +    ~AutoGCRooter() {

Is this not being virtual going to create Windows compiler warnings, given the virtual trace method below?  Or does the = 0 save you?

@@ +4967,5 @@
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    void changeLength(size_t newLength) {
> +        length = ptrdiff_t(newLength);

This cast is no longer necessary, right?

@@ +5011,5 @@
> +};
> +
> +class AutoIdArray : private AutoGCRooter {
> +  public:
> +    AutoIdArray(JSContext *cx, JSIdArray *ida JS_GUARD_OBJECT_NOTIFIER_PARAM)

Newline and appropriate indentation after *ida.  There's no good way to express what the macro does, but at least getting it away from the arguments we expect callers to provide will hurt readability less.

::: js/src/jsfriendapi.h
@@ +300,5 @@
>  
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(Value *vec, size_t len)
> +{
> +    PodZero(vec, len);

Add an assert that |vec <= vec + len|.

@@ +306,5 @@
> +
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(Value *beg, Value *end)
> +{
> +    PodZero(beg, end - beg);

Add an assert that |beg <= end|.

@@ +312,5 @@
> +
> +static JS_ALWAYS_INLINE void
> +MakeRangeGCSafe(jsid *beg, jsid *end)
> +{
> +    for (jsid *id = beg; id != end; ++id)

Can you make this |id < end| while you're here?  Belt-and-suspenders protection against wild overwrites if garbage comes in.

@@ +326,5 @@
> +template<class T>
> +class AutoVectorRooter : protected AutoGCRooter
> +{
> +  public:
> +    explicit AutoVectorRooter(JSContext *cx JS_GUARD_OBJECT_NOTIFIER_PARAM)

Put the guard param on a new line.

@@ +380,5 @@
> +
> +    const T &back() const { return vector.back(); }
> +
> +  protected:
> +    typedef Vector<T, 8> VectorImpl;

It might be worth considering (not here, separate bug!) making 8 configurable.  The switch-on-tag scheme made that difficult, but with virtual dispatch it'd be pretty easy.  Worthwhile?  *shrug*  Just throwing the idea out.

@@ +401,5 @@
> +
> +    const jsval *jsval_end() const { return end(); }
> +    jsval *jsval_end() { return end(); }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

This should go in the protected section -- and at the end, so as not to distort offsets in debug builds versus in optimized builds.

@@ +417,5 @@
> +    {
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

Same.

@@ +433,5 @@
> +    {
> +        JS_GUARD_OBJECT_NOTIFIER_INIT;
> +    }
> +
> +    JS_DECL_USE_GUARD_OBJECT_NOTIFIER

Same.

Comment 17

[Jeff Walden [:Waldo]]

Comment on attachment 565003 [details] [diff] [review]
main patch

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

So, honestly, I kind of feel like moving every helper method into friendapi, and not even trying to answer the question of whether whatever's being done would be better done another way, is a copout.

But I guess this is a step forward at least abstraction-wise.  And it looks like the same hackaround was used for jsobj.h already.

So I guess even if this is better (if only somewhat), I am decidely meh about it.  :-|

::: content/xul/document/src/nsXULContentSink.cpp
@@ +1060,5 @@
>                // By default scripts in XUL documents have E4X turned on. We use
>                // our implementation knowledge to reuse JSVERSION_HAS_XML as a
>                // safe version flag. This is still OK if version is
>                // JSVERSION_UNKNOWN (-1),
> +              version = JS_VersionSetXML(JSVersion(version), JS_TRUE);

This could be just |true| here, and for all the other callers.  Not important to change, just might be nicer to read.  And I suspect when PRBool->bool gets applied to JSBool, someday, this would happen eventually.  So there's an earlier-is-better contention here, I think.

::: js/src/jscntxt.h
@@ +263,5 @@
>                          js::DefaultHasher<void *>,
>                          js::SystemAllocPolicy> Map;
>  
> +    /* Opaque thread-id, from NSPR's PR_GetCurrentThread(). */
> +    void                *id;

This is more a comment for the entire file than for this line, but: any chance any of the members you've rearranged here, at all, could be made private at the same time you're doing all this work?  At least a followup or something would be nice.

@@ +352,5 @@
>  
>  }
>  
>  struct JSRuntime {
> +    /* Literal table maintained by jsatom.c functions. */

jsatom.cpp

::: js/src/jsfriendapi.cpp
@@ +222,5 @@
> +    return js_DateGetMsecSinceEpoch(cx, obj);
> +}
> +
> +JS_FRIEND_API(JSBool)
> +JS_WasLastGCCompartmental(JSContext *cx)

It seems pretty sad that we have/need this API right now.

::: js/src/jsfriendapi.h
@@ +131,5 @@
>  
>  #ifdef __cplusplus
>  
> +#include "jsatom.h"
> +#include "jsvector.h"

Including headers in the middle of a file like this seems worrisome.  Is there any way to avoid that, perhaps by moving this section to the top of the file or something?

::: js/src/xpconnect/shell/xpcshell.cpp
@@ +542,5 @@
>      JSRuntime *rt;
>      uint32 preBytes;
>  
> +    rt = JS_GetRuntime(cx);
> +    preBytes = JS_GetGCParameter(rt, JSGC_BYTES);

Unify the initializations with the declarations while you're here.

@@ +1002,5 @@
>  {
>      if ((errorNumber > 0) && (errorNumber < JSShellErr_Limit))
> +        return &jsShell_ErrorFormatString[errorNumber];
> +    else
> +        return NULL;

I'm guessing this was just copypasta'd from js.msg and uses, but isn't it just an error if an error is reported with a number outside the range its reporter is supposed to handle?  I think the latter half of this should be a JS_NOT_REACHED.

::: js/src/xpconnect/src/nsXPConnect.cpp
@@ +953,2 @@
>              cb.NoteScriptChild(nsIProgrammingLanguage::JAVASCRIPT,
> +                               js::GetContextGlobalObject(cx));

if (JSObject* global = js::Get...) to avoid repeating yourself?

::: js/src/xpconnect/src/xpcmaps.cpp
@@ +41,5 @@
>  /* Private maps (hashtables). */
>  
>  #include "xpcprivate.h"
>  #include "jsbit.h"
> +#include "jshash.h"

Why this?  Is this intentional?

::: js/src/xpconnect/src/xpcstring.cpp
@@ -90,5 @@
> -    JSAtom *atom;
> -    if (length == 0 && (atom = cx->runtime->atomState.emptyAtom))
> -    {
> -        return STRING_TO_JSVAL(atom);
> -    }

Mind-boggling.

::: toolkit/components/startup/nsAppStartup.cpp
@@ +706,5 @@
>  MaybeDefineProperty(JSContext *cx, JSObject *obj, const char *name, PRTime timestamp)
>  {
>    if (!timestamp)
>      return;
> +  JSObject *date = JS_NewDateObjectMsec(cx, timestamp/PR_USEC_PER_MSEC);

Spaces around the / here.

Comment 18

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

Comment on attachment 565003 [details] [diff] [review]
main patch

>--- a/dom/base/nsJSEnvironment.cpp
>+++ b/dom/base/nsJSEnvironment.cpp
>-    switch(cx->debugHooks->debuggerHandler(cx, script, ::JS_GetFramePC(cx, fp),
>+    JSDebugHooks *hooks = js::GetContextDebugHooks(cx);
>+    switch(hooks->debuggerHandler(cx, script, ::JS_GetFramePC(cx, fp),
>                                            &rval,
>-                                           cx->debugHooks->
>-                                           debuggerHandlerData)) {
>+                                           hooks->debuggerHandlerData)) {

Reindent, please.

>--- a/dom/workers/ListenerManager.cpp
>+++ b/dom/workers/ListenerManager.cpp
> #include "jsapi.h"
>-#include "jscntxt.h"
>+#include "jsfriendapi.h"
> #include "jsvector.h"

Can this include go now?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cb50e96dbee0

Comment 20

[Marco Bonardo [:mak]]

backed out for causing permanent failures in Windows PGO mochitests (1,4)

See philor's topic in tree-management
Here is some log:
https://tbpl.mozilla.org/php/getParsedLog.php?id=6981332&tree=Mozilla-Inbound
33463 ERROR TEST-UNEXPECTED-FAIL | /tests/content/base/test/test_bug664916.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: tagNameGetter.call is not a function at http://mochi.test:8888/tests/content/base/test/test_bug664916.html:34

https://tbpl.mozilla.org/php/getParsedLog.php?id=6981247&tree=Mozilla-Inbound
293 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug462428.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: getter.call is not a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug462428.html:34
341 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug585745.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: property descriptor's getter field is neither undefined nor a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug585745.html:29
373 ERROR TEST-UNEXPECTED-FAIL | /tests/js/xpconnect/tests/mochitest/test_bug628794.html | [SimpleTest/SimpleTest.js, window.onerror] - An error occurred: property descriptor's getter field is neither undefined nor a function at http://mochi.test:8888/tests/js/xpconnect/tests/mochitest/test_bug628794.html:29

Unfortunately there is no way to get pgo testing on Try atm, please contact releng to figure out a strategy for that.

Comment 21

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

So far it looks like the autorooters portion of the patch is what's causing the orange. Unfortunately that's the part that's harder to bisect over. I guess I'll try converting only JSAutoObjectRooter and leave the other rooters in jscntxt.h.

Comment 22

[Tom S [:evilpie]]

Any news?

Comment 23

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

I happen to have been looking at bug 332648 today:

https://tbpl.mozilla.org/?tree=Try&rev=2198dd8ad887
https://tbpl.mozilla.org/?tree=Try&rev=1503c844adb0

Comment 24

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I haven't had time for this lately. I did find that the part of the patch that causes orange in PGO. It's the change where I took the tag field out of AutoGCRooter and made the trace method be virtual. I don't know why that would cause problems, but it is. If someone wants to modify the patch to remove that part, I think it could land.

Ms2ger, to get useful results from tryserver, you need to change the mozconfigs to enable PGO builds. Normally we don't do PGO builds on tryserver (AFAIK), and the patch was only failing with PGO. See this page:
  https://wiki.mozilla.org/ReleaseEngineering/TryServer#Using_a_custom_mozconfig

Comment 25

[Luke Wagner [:luke]]

By any chance does Ms2ger wish to drive this ship home?

Comment 26

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

Attachment: Part a: Move AutoLockGC to jsfriendapi.h.

Used by XPCJSRuntime.cpp.

Comment 27

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

Attachment: Part b: Use existing JSAPI functions where those are available.

Comment 28

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

Attachment: Part c: Stop installing jsdate.h.

Comment 29

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

Attachment: Part d: Move js_GetErrorMessage to jsfriendapi.h.

Comment 30

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

Attachment: Part e: Move js_GetSCOffset to jsfriendapi.h.

Comment 31

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

Attachment: Part f: Expose structuredCloneCallbacks in jsfriendapi.h.

Comment 32

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

Attachment: Part g: Expose VersionSetXML in jsfriendapi.h.

Comment 33

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

Attachment: Part h: Expose debuggerHandler in jsfriendapi.h.

Comment 34

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

Attachment: Part i: Expose errorReporter in jsapi.h.

Comment 35

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

Attachment: Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp.

Comment 36

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

Attachment: Part k: Expose outstandingRequests in jsfriendapi.h.

Comment 37

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

Attachment: Part l: Use the existing JSAPI for the global object for a context.

Comment 38

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

Attachment: Part m: Expose context's compartment in jsfriendapi.h.

Comment 39

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

Attachment: Part n: Expose JSOPTION_UNROOTED_GLOBAL in jsfriendapi.h.

Comment 40

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

Attachment: Part o: Expose setActivityCallback in jsfriendapi.h.

Comment 41

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

Attachment: Part p: Remove AutoLockJSGC in favour of js::AutoLockGC.

Comment 42

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

Attachment: Part q: Expose gcLock in jsfriendapi.h.

Comment 43

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

Attachment: Part r: Provide AutoSkipConservativeScan in jsfriendapi.h.

Comment 44

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

Attachment: Part s: Make JS_TRACER_INIT a function instead of a macro

Comment 45

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

Attachment: Part t: Provide IsContextRunningJS in jsfriendapi.h.

Comment 46

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

Attachment: Part u: Provide TriggerOperationCallbacksForActiveContexts in jsfriendapi.h.

Comment 47

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

Attachment: Part v: Expose rt->compartments in jsfriendapi.h.

Comment 48

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

Attachment: Part w: Move AutoVectorRooter to jsapi.h.

Comment 49

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

Attachment: Part x: Move AutoValueVector to jsapi.h.

Comment 50

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

Attachment: Part z: Move JS_CHECK_RECURSION to jsfriendapi.h.

Comment 51

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

Attachment: Part y: Move AutoIdVector to jsapi.h.

Comment 52

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

Attachment: Part aa: Expose sizeof(JSContext) in jsfriendapi.h.

Comment 53

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

Attachment: Part ab: Remove jscntxt.h and dependencies from INSTALLED_HEADERS.

Comment 54

[Igor Bukanov]

Comment on attachment 586672 [details] [diff] [review]
Part a: Move AutoLockGC to jsfriendapi.h.

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

r+ with the comment below addressed.

::: js/src/jsfriendapi.h
@@ +460,5 @@
> +{
> +  public:
> +    explicit AutoLockGC(JSRuntime *rt = NULL
> +                        MOZILLA_GUARD_OBJECT_NOTIFIER_PARAM);
> +    ~AutoLockGC();

Add  couple of private static functions LockGC(JSRutime *rt), UnlockGC(JSRuntime *rt) functions and use them to implement the constructors/destructors. This way the compiler can optimize the case of no locking and can also see that a reference to AutoLockGC::runtime is not leaked so it can remove a stack slot for it.

Comment 55

[Igor Bukanov]

Comment on attachment 586679 [details] [diff] [review]
Part h: Expose debuggerHandler in jsfriendapi.h.

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

::: js/src/jsfriendapi.cpp
@@ +515,5 @@
> +JS_FRIEND_API(JSTrapStatus)
> +CallContextDebugHandler(JSContext *cx, JSScript *script, jsbytecode *bc, Value *rval)
> +{
> +    MOZ_ASSERT(CanCallContextDebugHandler(cx));
> +    return cx->debugHooks->debuggerHandler(cx, script, bc, rval,

We should fix the preexisting bug here - after the haddler was checked for existence it could have been removed. So do not assert that the handler is not null, rather return JSTRAP_RETURN in this case.

Comment 56

[Andreas Gal :gal]

Comment on attachment 586686 [details] [diff] [review]
Part n: Expose JSOPTION_UNROOTED_GLOBAL in jsfriendapi.h.

I am not a big fan of friend APIs, we should probably turn this stuff into public apis, but other related stuff around this api is friend to, so not a problem for this patch.

Comment 57

[Andreas Gal :gal]

Comment on attachment 586688 [details] [diff] [review]
Part o: Expose setActivityCallback in jsfriendapi.h.

What is that #if 0 garbage there?

Comment 58

[Andreas Gal :gal]

Comment on attachment 586690 [details] [diff] [review]
Part q: Expose gcLock in jsfriendapi.h.

Not the cleanest abstraction, but I don't have any better suggestion either. Well worth the end goal of getting rid of the header include and we can cleanup the interface from there.

Comment 59

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

(In reply to Andreas Gal :gal from comment #57)
> Comment on attachment 586688 [details] [diff] [review]
> Part o: Expose setActivityCallback in jsfriendapi.h.
> 
> What is that #if 0 garbage there?

My todo list; it's gone entirely by the end of this patch queue. I could get rid of it, but I'd kind of rather not spend my time on the merging that will result.

(In reply to Andreas Gal :gal from comment #58)
> Comment on attachment 586690 [details] [diff] [review]
> Part q: Expose gcLock in jsfriendapi.h.
> 
> Not the cleanest abstraction, but I don't have any better suggestion either.
> Well worth the end goal of getting rid of the header include and we can
> cleanup the interface from there.

Yeah. Maybe a function of rt that returns JS_NEW_CONDVAR(rt->gcLock) would be somewhat better... I dunno.

Comment 60

[Luke Wagner [:luke]]

Comment on attachment 586676 [details] [diff] [review]
Part e: Move js_GetSCOffset to jsfriendapi.h.

>-#include "jsclone.h"
>+#include "jscntxt.h" // structuredCloneCallbacks

I'm probably missing the grand plan, but, don't you mean 'jsfriendapi.h' ?

Comment 61

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

(In reply to Luke Wagner [:luke] from comment #60)
> Comment on attachment 586676 [details] [diff] [review]
> Part e: Move js_GetSCOffset to jsfriendapi.h.
> 
> >-#include "jsclone.h"
> >+#include "jscntxt.h" // structuredCloneCallbacks
> 
> I'm probably missing the grand plan, but, don't you mean 'jsfriendapi.h' ?

It needs both jsfriendapi and jscntxt at this point. I added jsfriendapi.h in attachment 586677 [details] [diff] [review], but it makes more sense to add it here, indeed.

Comment 62

[Jason Orendorff [:jorendorff]]

Comment on attachment 586696 [details] [diff] [review]
Part v: Expose rt->compartments in jsfriendapi.h.

In jsfriendapi.h and also in the .cpp file:
>+JS_FRIEND_API(CompartmentVector&)
>+GetRuntimeCompartments(JSRuntime *rt)

const CompartmentVector &, please.

>-        js::CompartmentVector &vector = rt->compartments;
>+        js::CompartmentVector &vector = js::GetRuntimeCompartments(rt);
>         for (JSCompartment **p = vector.begin(); p != vector.end(); ++p) {

Add const at the beginning of the line you changed, please.

It would also be best to change the for-loop to insert the `const` keyword between the two stars, like so:

          for (JSCompartment * const *p = vector.begin() ......

and I encourage you to do it even though (due to a bug in js::Vector) C++ might let you get away without it.

Comment 63

[Jason Orendorff [:jorendorff]]

Comment on attachment 586677 [details] [diff] [review]
Part f: Expose structuredCloneCallbacks in jsfriendapi.h.

Stealing review.

Comment 64

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 586681 [details] [diff] [review]
Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

I tried to avoid XPCOM-ish stuff in these js bindings in case we decided to move them into the js engine or autogenerate them somehow. I'd really prefer not to add nsTArray here, is it just not possible to use Vector outside of the engine?

Also, this patch adds jsfriendapi.h but as far as I can tell never uses anything in it.

Comment 65

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

We can't use js::ContextAllocPolicy outside the engine.

Comment 66

[Brian Hackett [Laid off!]]

Comment on attachment 586678 [details] [diff] [review]
Part g: Expose VersionSetXML in jsfriendapi.h.

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

::: js/src/jsfriendapi.cpp
@@ +501,5 @@
>  
> +JS_FRIEND_API(JSVersion)
> +VersionSetXML(JSVersion version, bool enable)
> +{
> +    VersionSetXML(&version, enable);

Can you remove the VersionSetXML from jscntxt.h and put its implementation here, then change the single caller in jscntxt.h to use this function?

Comment 67

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

Attachment: Part j: Stop using js::ContextAllocPolicy in ListenerManager.cpp. (v2)

Like this more, perhaps?

Comment 68

[Bobby Holley (:bholley)]

Comment on attachment 586684 [details] [diff] [review]
Part l: Use the existing JSAPI for the global object for a context.


>         JSContext *cx = static_cast<JSContext*>(n);
>-        NS_ASSERTION(cx->globalObject, "global object NULL before unlinking");
>-        cx->globalObject = nsnull;
>+        JSAutoRequest ar(cx);
>+        NS_ASSERTION(JS_GetGlobalObject(cx), "global object NULL before unlinking");
>+        JS_SetGlobalObject(cx, NULL);

Uh oh. Does this mean we were racy before?

Comment 69

[Bobby Holley (:bholley)]

Comment on attachment 586689 [details] [diff] [review]
Part p: Remove AutoLockJSGC in favour of js::AutoLockGC.

Huzzah!

Comment 70

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

Attachment: Stop using js::ContextAllocPolicy in ListenerManager.cpp.

Comment 71

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

Thanks!

Comment 72

[Steve Fink [:sfink] [:s:]]

Comment on attachment 587776 [details] [diff] [review]
Stop using js::ContextAllocPolicy in ListenerManager.cpp.

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

I'm ok with this as is (modulo the constructor name fix), though the naming is clunky.

::: js/src/jsalloc.h
@@ +115,5 @@
>  
>      JS_FRIEND_API(void) reportAllocOverflow() const;
>  };
>  
> +class ExternallyUsableContextAllocPolicy

I don't understand the layering anymore. Could this live in js/public instead of here?

Would JSContextAllocPolicy be too confusing as a name?

@@ +120,5 @@
> +{
> +    JSContext *const cx;
> +
> +  public:
> +    ContextAllocPolicy(JSContext *cx) : cx(cx) {}

Cut & paste error -- wrong class name.

Comment 73

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

https://hg.mozilla.org/mozilla-central/rev/b2ef431401ff
https://hg.mozilla.org/mozilla-central/rev/1668811de954
https://hg.mozilla.org/mozilla-central/rev/4f665744b6e6
https://hg.mozilla.org/mozilla-central/rev/ebd92ba20fd1
https://hg.mozilla.org/mozilla-central/rev/10519108297f
https://hg.mozilla.org/mozilla-central/rev/6ffce4fb752b
https://hg.mozilla.org/mozilla-central/rev/58eef6ad44ad
https://hg.mozilla.org/mozilla-central/rev/2b2c3d92ae9f
https://hg.mozilla.org/mozilla-central/rev/f9fe45249420
https://hg.mozilla.org/mozilla-central/rev/be5a53604bc3
https://hg.mozilla.org/mozilla-central/rev/46b5d4c3b3d8
https://hg.mozilla.org/mozilla-central/rev/29a2d146c5ff
https://hg.mozilla.org/mozilla-central/rev/41ea9acca641
https://hg.mozilla.org/mozilla-central/rev/fe8240db5834
https://hg.mozilla.org/mozilla-central/rev/33d5a1f2af51
https://hg.mozilla.org/mozilla-central/rev/e798120914f7
https://hg.mozilla.org/mozilla-central/rev/35247789f332
https://hg.mozilla.org/mozilla-central/rev/fee37d705c87
https://hg.mozilla.org/mozilla-central/rev/346328bd1f2e
https://hg.mozilla.org/mozilla-central/rev/c8ba49dafe5e
https://hg.mozilla.org/mozilla-central/rev/d848942fc440
https://hg.mozilla.org/mozilla-central/rev/b2bffccbc28b
https://hg.mozilla.org/mozilla-central/rev/aa34f07b7f67
https://hg.mozilla.org/mozilla-central/rev/4b70a75df702
https://hg.mozilla.org/mozilla-central/rev/9ee7f6571842
https://hg.mozilla.org/mozilla-central/rev/cf1294a14a4c
https://hg.mozilla.org/mozilla-central/rev/9f706102086f
https://hg.mozilla.org/mozilla-central/rev/dfc618d2f28f

Thanks all!