Created attachment 129620 [details] [review]
be more --enable-debug friendly

Adds a bunch JS_BeginRequest / JS_EndRequest calls all around. Not really sure about performance impact.

Doesn't fix the internal assertions completely. Remaining one is from testImporter.js:
    Assertion failure: scope->object == obj, at jsapi.cpp:4315
via JS_NextProperty
gjs/importer.c (load_module_elements)

and I've no idea what it means. The asserted condition seems to be valid in JS_NewPropertyIterator


 gi/closure.c            |    5 +++++
 gi/keep-alive.c         |    6 +++++-
 gi/ns.c                 |   19 ++++++++++++++-----
 gi/object.c             |    4 ++++
 gi/repo.c               |   17 +++++++++++++----
 gi/value.c              |    3 +++
 gjs/context.c           |   15 ++++++++++++---
 gjs/importer.c          |   28 ++++++++++++++++++++++------
 gjs/jsapi-util-array.c  |    2 ++
 gjs/jsapi-util-error.c  |    2 ++
 gjs/jsapi-util-string.c |    6 ++++++
 gjs/jsapi-util.c        |   19 ++++++++++++++++---
 modules/mainloop.c      |    4 ++++
 13 files changed, 108 insertions(+), 22 deletions(-)

why doesn't "(cx)->thread == (cx)->runtime->gcThread" ?

according to
https://developer.mozilla.org/en/SpiderMonkey/JSAPI_Reference/JS_BeginRequest

We could definitely cause a problem with these, by either holding the request while generating too much stuff that needs GC, or while doing some blocking operation or callback invocation. (note to self to look at in review)

Should we do
 #ifdef DEBUG
 #define gjs_begin_request(ctx) \
   JS_BeginRequest(ctx)
 #else
 #define gjs_begin_request(ctx)
 #endif

?

        JS_CALL_OBJECT_TRACER(tracer, child->child, "child");

unrelated change?

        JS_BeginRequest(c->context);
        gjs_keep_alive_remove_global_child(c->context,
        JS_EndRequest(c->context);

Seems like we need a sane rule for whether you have to begin/end request around gjs APIs, or whether they do it for you. The rule that makes sense to me on first glance is that they do it for you and you only have to begin/end request when calling an API if you need the mutex held across multiple API calls. So never begin/end to call a single function, basically.

        closure_marshal

Does this hold begin/end across an arbitrary function invocation? That seems like a bad idea. But then JS_CallFunctionValue indicates that it requires a request. If it's true that GC doesn't happen while in a BeginRequest, what happens if we hold a BeginRequest while invoking Mainloop.run() for example? Hmm. Clearly I don't understand some aspect of the picture.

Obviously same question applies to gjs_call_function_value

And it looks like we might hold a request across main loop source functions? Some of those definitely block / generate a lot of stuff to GC ...

I should probably emphasize I was driven by 'make check' failures, any correctness issues not covered by make check are therefore not addressed.


(In reply to comment #2)
> why doesn't "(cx)->thread == (cx)->runtime->gcThread" ?

My guess would be that GC is not running at the moment.


(In reply to comment #4)

>         JS_CALL_OBJECT_TRACER(tracer, child->child, "child");
> 
> unrelated change?

It's related to making 'make check' go further with --enable-debug / -DDEBUG build. Not all problems are Begin/EndRequest.


>         JS_BeginRequest(c->context);
>         gjs_keep_alive_remove_global_child(c->context,
>         JS_EndRequest(c->context);
> 
> Seems like we need a sane rule for whether you have to begin/end request around
> gjs APIs, or whether they do it for you. The rule that makes sense to me on
> first glance is that they do it for you and you only have to begin/end request
> when calling an API if you need the mutex held across multiple API calls. So
> never begin/end to call a single function, basically.

Having public entry points in GJS do Begin/End for you sounds reasonable when dealing with GjsContext. Not so sure about all the utility functions taking JSContext, since chances are high we already are in the right context.

I think the only odd case is dealing with crossing over from one context to another, namely with keepalive and load contexts.


>         closure_marshal
> 
> Does this hold begin/end across an arbitrary function invocation? That seems
> like a bad idea. But then JS_CallFunctionValue indicates that it requires a
> request. If it's true that GC doesn't happen while in a BeginRequest, what
> happens if we hold a BeginRequest while invoking Mainloop.run() for example?
> Hmm. Clearly I don't understand some aspect of the picture.

From what I read from JS_BeginRequest is that whenever we're making spidermonkey calls we need to be in BeginRequest. Whenever we make a blocking native call (I presume this means non-spidermonkey calls) we should wrap the blocking call in JS_SuspendRequest / JS_ResumeRequest.

So g_main_loop_run() would become
    JS_SuspendRequest();
    g_main_loop_run();
    JS_ResumeRequest();

and as closure_marshal calls JS_BeginRequest we ought to be re-entering the request appropriately. Though I am only guessing Suspend and Begin can be mixed in that way. We could probably also do instead
    JS_EndRequest();
    g_main_loop_run();
    JS_BeginRequest();

Another possible approach here, since we are not interested in threads support, would be to add a runtime switch to spidermonkey to disable multi thread support (even if enabled at build time, so that apps like firefox can continue to work as usual).

Looks like Jasper fixed all this stuff in https://bugzilla.gnome.org/show_bug.cgi?id=703068, but I'll happily take over this bug to add some more fixes for running against a debug mozjs24 build.

Created attachment 336312 [details] [review]
coverage: Enter correct compartment

To use JS_GetGlobalForObject(), the context argument must be in the same
compartment as the second argument. Since this call was part of another
call entering that compartment, then the context was not already in that
compartment, so the call triggers an assertion if mozjs is compiled with
debug flags.

Instead, we can use the second argument directly to indicate which
compartment to enter.

Created attachment 336313 [details] [review]
object: Check for error on gjs_value_to_g_value()

Without this error check, it is possible for an exception to be left
pending, and mozjs will assert about that if compiled with debug flags.

Adding the error check exposed an additional bug in
testExceptionInPropertySetterWithBinding(); without the G_PARAM_CONSTRUCT
flag the getter would return undefined because the property had never
been set.

Review of attachment 336312 [details] [review]:

Looks good to me.

Review of attachment 336313 [details] [review]:

Nice catch!

Got one additional assert failure while trying on another system; this is bizarre because examining the variables with gdb shows that the assertion shouldn't trigger! There's nothing at all from GJS in the stack trace so I think this is actually a mozjs bug, we'll see if it's already been fixed in a later version.

Starting program: /home/philip/.cache/jhbuild/build/gjs/.libs/lt-jsunit -p /js/EverythingBasic
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
/js/EverythingBasic: [New Thread 0x7fffe6a06700 (LWP 16986)]
[New Thread 0x7fffe6205700 (LWP 16987)]
Gjs-Message: JS LOG: running test testLifeUniverseAndEverything
Gjs-Message: JS LOG: running test testLimits
Assertion failure: ui == uint32_t((2147483647)) + 1, at /home/philip/checkout/mozjs-24.2.0/js/src/jsnum.cpp:512

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff49038e8 in js::Int32ToString<(js::AllowGC)1> (cx=cx@entry=0x6205a0, 
    si=si@entry=-2147483648)
    at /home/philip/checkout/mozjs-24.2.0/js/src/jsnum.cpp:512
512	        JS_ASSERT_IF(si == INT32_MIN, ui == uint32_t(INT32_MAX) + 1);
(gdb) print si == -0x7fffffff-1
$8 = true
(gdb) print ui == ((uint32_t)0x7fffffff)+1
$9 = true
(gdb) call gjs_dumpstack()
== Stack trace for context 0x61d000 ==
assertEquals@resource:///org/gnome/gjs/modules/jsUnit.js:147
testLimits@/home/philip/install/libexec/gjs/installed-tests/js/testEverythingBasic.js:99
gjstestRun@resource:///org/gnome/gjs/modules/jsUnit.js:438
@/home/philip/install/libexec/gjs/installed-tests/js/testEverythingBasic.js:734

(gdb) 

In any case, I'll push these two patches and close the bug, thanks for the review.

Attachment 336312 [details] pushed as 313de70 - coverage: Enter correct compartment
Attachment 336313 [details] pushed as b136107 - object: Check for error on gjs_value_to_g_value()