[Edbrowse-dev] garbage collection

[Edbrowse-dev] garbage collection

[Chris Brannon]

I'm more and more convinced that the JavaScript crashes are related to
GC and rooting, as Adam suspected.  The crashes I'm seeing are occurring
in jsdom.c, and they seem to involve corruption of the JS heap, not the
heap used for edbrowse strings and other data.
Example: this one at line 1185 of jsdom.c from master:
	    v = JS_NewObject(jcx, cp, NULL, owner);
One of the pointers passed in is apparently pointing to something that
was freed long ago.  I don't think it's jcx or cp, so it must be owner.

So let's switch gears.  I've been working with Adam's code, and it still
has GC / rooting issues.  From what I can tell, JS_DefineProperty can
trigger a GC.  We should not be passing unrooted jsval as the fourth
argument to JS_DefineProperty as this can also lead to a crash.
Also there seems to be a problem in jsloc.cpp, caused by uo, which is a
statically allocated pointer to a JS object that is never rooted.

-- Chris

Re: [Edbrowse-dev] garbage collection1

[Adam Thompson]

On Thu, Jan 23, 2014 at 12:27:10PM -0800, Chris Brannon wrote:
> I'm more and more convinced that the JavaScript crashes are related to
> GC and rooting, as Adam suspected.  The crashes I'm seeing are occurring
> in jsdom.c, and they seem to involve corruption of the JS heap, not the
> heap used for edbrowse strings and other data.
> Example: this one at line 1185 of jsdom.c from master:
> 	    v = JS_NewObject(jcx, cp, NULL, owner);
> One of the pointers passed in is apparently pointing to something that
> was freed long ago.  I don't think it's jcx or cp, so it must be owner.

If I remember correctly this is in domLink,
which I had to fix to get edbrowse passed linking the body tag.

I've *hopefully* fixed the uo rooting (thanks for noticing that,
I'd removed the rooting when I ran into the NULL pointer problem but never put it back).

> So let's switch gears.  I've been working with Adam's code, and it still
> has GC / rooting issues.  From what I can tell, JS_DefineProperty can
> trigger a GC.  We should not be passing unrooted jsval as the fourth
> argument to JS_DefineProperty as this can also lead to a crash.

Yeah, I'm not sure how to work around this.

If you could test the latest code that'd be useful (corrected jwin and uo rooting).
I'm not sure it fixes the event handler bug,
but it's hopefully a step closer to stability.

Cheers,
Adam.

Re: [Edbrowse-dev] garbage collection1

[Chris Brannon]

[-- Attachment #1: Type: text/plain, Size: 615 bytes --]

Adam Thompson <arthompson1990@gmail.com> writes:

>> We should not be passing unrooted jsval as the fourth
>> argument to JS_DefineProperty as this can also lead to a crash.
>
> Yeah, I'm not sure how to work around this.

I am, and a patch is attached.  There may be other cases.  I can look through
the code for them.  I fixed the one I noticed.

> If you could test the latest code that'd be useful (corrected jwin and uo rooting).

I'm not seeing it.  All I see is that you merged the master branch back
into your repo.  Did you forget a push?

PS.  Thank you very much for all of your work on this!

-- Chris


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: rootedval.diff --]
[-- Type: text/x-diff, Size: 1171 bytes --]

diff --git a/src/jsdom.cpp b/src/jsdom.cpp
index 7f3eef3..9c12bd7 100644
--- a/src/jsdom.cpp
+++ b/src/jsdom.cpp
@@ -1243,13 +1243,13 @@ Yeah, it makes my head spin too.
 	} else {
 	    v = JS_NewObject(jcx, cp, NULL, owner_root);
 	}
-	vv = OBJECT_TO_JSVAL(v);
+	JS::RootedValue rvv(jcx, OBJECT_TO_JSVAL(v));
 
 /* if no name, then use id as name */
 	if(!symname && idname) {
-	    JS_DefineProperty(jcx, owner_root, idname, vv, NULL, NULL, attr);
+	    JS_DefineProperty(jcx, owner_root, idname, rvv, NULL, NULL, attr);
 	} else if(symname && !dupname) {
-	    JS_DefineProperty(jcx, owner_root, symname, vv, NULL, NULL, attr);
+	    JS_DefineProperty(jcx, owner_root, symname, rvv, NULL, NULL, attr);
 	    if(stringEqual(symname, "action"))
 		establish_property_bool(v, "actioncrash", eb_true, eb_true);
 
@@ -1259,7 +1259,7 @@ Yeah, it makes my head spin too.
 	    establish_property_object(master, symname, v);
 	} else {
 /* tie this to something, to protect it from gc */
-	    JS_DefineProperty(jcx, owner_root, fakePropName(), vv,
+	    JS_DefineProperty(jcx, owner_root, fakePropName(), rvv,
 	       NULL, NULL, JSPROP_READONLY | JSPROP_PERMANENT);
 	}
 

Re: [Edbrowse-dev] garbage collection1

[Adam Thompson]

On Fri, Jan 24, 2014 at 06:21:21AM -0800, Chris Brannon wrote:
> Adam Thompson <arthompson1990@gmail.com> writes:
> 
> >> We should not be passing unrooted jsval as the fourth
> >> argument to JS_DefineProperty as this can also lead to a crash.
> >
> > Yeah, I'm not sure how to work around this.
> 
> I am, and a patch is attached.  There may be other cases.  I can look through
> the code for them.  I fixed the one I noticed.

Ah, ok, I see what you mean. I've looked through and *hopefully* fixed a bunch of them.
I've definitely pushed the set of commits now (git push origin master)
so hoefully you can see them.

In addition I think we need to change the establish_property_* functions to take
JS::Handle and js::MutableHandle arguments.

Cheers,
Adam.

Re: [Edbrowse-dev] garbage collection1

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> In addition I think we need to change the establish_property_* functions to take
> JS::Handle and js::MutableHandle arguments.

Yes, I think so, but this is going to cause problems, since other
functions in html.c call establish_property_*, and those don't know
about C++ types.

But you're on to something...
The rooting guide [1] says that we should basically never use raw pointers.
Why?  Because Spidermonkey doesn't know about them, and it will happily
invalidate them during a garbage collection.
But we're using them a lot.  Here's a stretch of code that could crash
the program.  It starts at line 544 of html.c and ends at line 564.
e is a raw pointer to something in the JS heap, and each one of those
establish_property_* calls could move it.

So I think it might be time to make html.c compilable with g++ and move
over to the appropriate SpiderMonkey types.  I'm
willing to do this work if you need a break.

-- Chris