[Edbrowse-dev] gc rooted

[Edbrowse-dev] gc rooted

[Karl Dahlke]

I finally read the gc rooted guide, and it makes my head spin,
especially since I am still learning C++ on top of it.

I wanted to start at the heart of the matter, jwin,
which is a global pointer to a js object.
The guide talks about local variables, parameters, returns,
heap, but nothing about global or static.
So from the start I am a bit lost.
But I think I will pretend like it is a local variable,
that just never goes out of scope.
So should be declared and defined like this.

[extern] JS::RootedObject *jwin;

Ok then a context switch, moving from buffer 1 to buffer 3,
needs to swap in the new java window.
This is done by jMyContext().
 at 1109,
        jwin = cw->jsw;
This should work, even if jsw remains void * as it is today.
Should be ok.
But how is it set in the first place?

line 877
    jwin = JS_NewGlobalObject(jcx, &window_class, NULL);
I assume NewGlobalObject is part of their API?
I don't see it in my stuff.
Anyways what does it return?
If just an object pointer then it is not rooted,
and maybe should be

jwin = new JS::RootedObject(jcx, JS_NewGlobalObject(jcx, &window_class, NULL));

When we quit a buffer, or ^ to pop the stack, we would free this window,
I suppose by a delete operation,
I wonder if that would clean everything up properly?

Well this is probably the first in a long line of
thoughts, observations, etc, trying to put it all together.
The rooted guide suggests we have to change everything touching js objects.
Unless it is really just stored as void * like cw->jsw, which is probably ok.
But when we use them in any way shape or form, well you know.

Karl Dahlke

Re: [Edbrowse-dev] gc rooted

[Chris Brannon]

Karl Dahlke <eklhad@comcast.net> writes:

> The guide talks about local variables, parameters, returns,
> heap, but nothing about global or static.

Ok, here's my suggestion.
Actually I've been implementing it along with my other work on html.cpp
and buffers.c.
Let's get rid of the global jwin altogether.
It duplicates the data in cw->jsw.  There's always going to be
a current edbrowse window, and that window's jsc and jsw should never go
out of sync with the globals jcx and jwin.
Instead of the globals, let's pack all of this JS stuff into an opaque
struct on the heap, with a pointer stored in ebWindow, like so:

/* file eb.h */
/* Forward decl, fully declared in js.h. */
struct ebJSState;

struct ebWindow {
...
    struct ebJSState *jsstate;
/* Struct is opaque outside js-related files. */
};

/* File js.h */
struct ebJSState {
    JSContext *jcx;
    jS::Heap<JSObject *> jwin;
    JS::Heap<JSObject *> jdoc;
};

So now jdoc and jwin are rooted.

We could keep using the jdoc and jwin globals, and use JS_AddObjectRoot
to root them, but I think the opaque struct is the safest bet.  It also
eliminates void pointers.
The jcx global is ok, but I'd argue for getting rid of it as well, since
it just duplicates the JSContext * from the current window.


-- Chris

Re: [Edbrowse-dev] gc rooted

[Adam Thompson]

On Sat, Jan 25, 2014 at 11:47:36AM -0500, Karl Dahlke wrote:
> I finally read the gc rooted guide, and it makes my head spin,
> especially since I am still learning C++ on top of it.
>
> I wanted to start at the heart of the matter, jwin,
> which is a global pointer to a js object.

Yeah, I basically use JS_AddObjectRoot to root jwin when it's created in
createJavaContext.

This fixes the jwin rooting issue for the moment but as we've now got a c++
html parser (html.cpp) we can and should go for the opaque js struct.

In addition:
JS::RootedObject *jwin = new JS::RootedObjet

Is (I think) according to the gc rooting guide a really bad idea since it keeps
a rooted object on the heap which is illegal (it's only valid for the stack).

Cheers,
Adam.