Created attachment 247749 [details] [review]
context: Add missing BeginRequest / EndRequest

Created attachment 247750 [details] [review]
util: Use JS reserved slots for our global slots

The JS object doesn't technically allow globals with privates,
so we need to use the reserved slot system.

Created attachment 247751 [details] [review]
stack: Add missing BeginRequest / EndRequest

Created attachment 247752 [details] [review]
keep-alive: Fix an unpaired BeginRequest / EndRequest

And fix an atrociously bad cast error.

Created attachment 247753 [details] [review]
function: Fix a memory leak and simplify code

Created attachment 247754 [details] [review]
context: Make sure to quit early when logError() is given incorrect args

Created attachment 247755 [details] [review]
util: Seal up a missing EndRequest

Review of attachment 247749 [details] [review]:

With stuff like this it tends to be a bit of an open question whether we should require callers to do it, but eh, this is fine.

Review of attachment 247750 [details] [review]:

The global slots API is a bit funky.  This looks right though, and is clearly a code improvement.

Review of attachment 247751 [details] [review]:

::: gjs/stack.c
@@ +64,3 @@
         !JSVAL_IS_OBJECT(v_constructor)) {
         g_error("??? Missing Error constructor in global object?");
+        goto out;

This one should just be g_assert_not_reached() or something, since g_error() won't actually return in the default configuration, and if we hit it we won't need the error message.

I've never found g_error() to be a super useful API myself, since without the __attribute__((noreturn)) it's annoying to use, as you have to placate GCC for conditions that won't actually be reached.

Review of attachment 247752 [details] [review]:

Right.

Review of attachment 247753 [details] [review]:

::: gi/function.c
@@ +1648,2 @@
                            GJS_MODULE_PROP_FLAGS)) {
         gjs_debug(GJS_DEBUG_GFUNCTION, "Failed to define function");

Need function = NULL here.

Review of attachment 247754 [details] [review]:

Ok.

Review of attachment 247755 [details] [review]:

Ouch, yes, looks right.

Attachment 247749 [details] pushed as c505967 - context: Add missing BeginRequest / EndRequest
Attachment 247750 [details] pushed as 7d19f28 - util: Use JS reserved slots for our global slots
Attachment 247752 [details] pushed as b99e504 - keep-alive: Fix an unpaired BeginRequest / EndRequest
Attachment 247753 [details] pushed as e5abda7 - function: Fix a memory leak and simplify code
Attachment 247754 [details] pushed as ef2b134 - context: Make sure to quit early when logError() is given incorrect args
Attachment 247755 [details] pushed as 028c696 - util: Seal up a missing EndRequest


Pushed with suggested fixes, and removed the stack patch. I confused
g_error with g_critical and thought it would continue onward. It wasn't
really fixing anything, it's just something I looked into while trying
to find the missing EndRequest.

Don't know why this was left open.