[Edbrowse-dev] my very large patch

[Edbrowse-dev] my very large patch

[Chris Brannon]

Ok, it is done.
I think it is too big to send to the list, so here's a link:
http://the-brannons.com/js.patch
The commit message should be a pretty good description of what I've
changed, so I won't rehash it here unless someone has questions.
I may have made mistakes along the way as well.  It's quite possible
that I forgot to use JSAutoCompartment in some places where it is needed.

html.cpp is now compilable with g++, but it requires the -fpermissive
switch.  Turns out that g++ doesn't like Karl's list macros from eb.h,
because of the use of void pointers.  Since html.cpp is now in C++, it
might be a good idea to rewrite the list manipulations to use the list
type from C++'s standard template library.  But not today.

So after doing all that work, I fired up my shiny new edbrowse, and I
eventually managed to make it crash.  Turns out that in some cases, it
has nothing to do with GC at all.
The problem is that in a lot of instances, we do not check the return
value of JS_NewObject, and it will return NULL on an error (such as heap
exaustion).  JS_NewObject returned NULL somewhere, and a bit later on in
the program, a crash ensued because that NULL got passed to
a function that expected a non-NULL.
The JS heap is too small.

The patch is still necessary, though.

-- Chris

Re: [Edbrowse-dev] my very large patch

[Adam Thompson]

On Sat, Jan 25, 2014 at 02:46:37PM -0800, Chris Brannon wrote:
> Ok, it is done.
> I think it is too big to send to the list, so here's a link:
> http://the-brannons.com/js.patch
> The commit message should be a pretty good description of what I've
> changed, so I won't rehash it here unless someone has questions.

Ok, thanks. This is basically what I had in mind when I started doing this
rewrite, but I didn't have enough time to completely remove the globals, hence the...
interesting... rooting with JS_AddObjectRoot and friends.

Will go through the code in more detail as I have more time.
In addition, I think there's probably lots more cases where we're not correctly
checking return values etc so I'll try and pick these up.

Cheers,
Adam.

Re: [Edbrowse-dev] my very large patch

[Adam Thompson]

On Sun, Jan 26, 2014 at 11:52:28PM +0000, Adam Thompson wrote:
> Ok, thanks. This is basically what I had in mind when I started doing this
> rewrite, but I didn't have enough time to completely remove the globals, hence the...
> interesting... rooting with JS_AddObjectRoot and friends.
> 
> Will go through the code in more detail as I have more time.
> In addition, I think there's probably lots more cases where we're not correctly
> checking return values etc so I'll try and pick these up.

I'm also removing the jcx global and the jrt global.  I'm going to remove the
two explicit gc calls as well, as gc is now done almost everywhere anyway and this
allows the runtime var to be contained withing jsdom.cpp.

Re: [Edbrowse-dev] my very large patch

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> I'm also removing the jcx global and the jrt global.  I'm going to remove the
> two explicit gc calls as well,

Sounds good to me!  I was just keeping jcx around for convenience, since
it's a little easier to type and to read with a screen reader than
cw->jss->jcx.  But that's not really a good reason to keep it, IMHO.

-- Chris

Re: [Edbrowse-dev] my very large patch1

[Adam Thompson]

On Sun, Jan 26, 2014 at 05:22:16PM -0800, Chris Brannon wrote:
> Adam Thompson <arthompson1990@gmail.com> writes:
> 
> > I'm also removing the jcx global and the jrt global.  I'm going to remove the
> > two explicit gc calls as well,
> 
> Sounds good to me!  I was just keeping jcx around for convenience, since
> it's a little easier to type and to read with a screen reader than
> cw->jss->jcx.  But that's not really a good reason to keep it, IMHO.

Yeah, and as Karl said, it also means we can remove the jMyContext function,
which is useful I think.

I'm also thinking of removing a couple of wrapper functions,
like the ones which previously attempted to handle utf8 stuff with js as the
new api gives no easy way of working out what mode the js engine is running in
anyway, so we kind of need to hope everything's ok.
Basically I think the rule with new SpiderMonkey is assume that the engine
takes 1 byte per character strings unless using the jschar unicode functions
(which is going to require much rewriting).

Cheers,
Adam.

Re: [Edbrowse-dev] my very large patch

[Adam Thompson]

On Mon, Jan 27, 2014 at 12:49:10AM +0000, Adam Thompson wrote:
> I'm also removing the jcx global and the jrt global.  I'm going to remove the
> two explicit gc calls as well, as gc is now done almost everywhere anyway and this
> allows the runtime var to be contained withing jsdom.cpp.

Done and *hopefully* pushed, with reformatting.
I've also removed the jMyContext function.

Cheers,
Adam.