[Edbrowse-dev] segfault with small pool

[Edbrowse-dev] segfault with small pool

[Karl Dahlke]

> As for the heap-rooted objects, maybe javaSessionFail could traverse the

No need.
I don't call the heap destructor unless js is alive.
See html.cpp line 319.

But the concern about stacks unwinding the rooted objects,
in a context that is destroyed,  is real,
and we need to think about that logic.

Karl Dahlke

[Edbrowse-dev] segfault with small pool

[Karl Dahlke]

I sometimes run with jspool = 2 just to test,
just to see what happens with a real world memory error.
While doing this, I was looking at my edbrowse wiki article,
putting in more feature content as Adam suggested,
and I was verifying some of the internal wiki links,
and got a segfault.
Set jspool = 2 and type or read in these two commands.

b http://en.wikipedia.org/wiki/http
b http://en.wikipedia.org/wiki/https

I get something like this.

edbrowse ready
 .
180413
 .
34837
 .
135113
 .
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
could not allocate memory for javascript operations.
out of memory
Unrecoverable JavaScript error in this session
*** glibc detected *** /home/eklhad/bin/edbrowse: free(): corrupted unsorted chunks: 0x0a4ca310 ***
======= Backtrace: =========

Karl Dahlke

Re: [Edbrowse-dev] segfault with small pool

[Adam Thompson]

On Wed, Feb 19, 2014 at 04:21:37AM -0500, Karl Dahlke wrote:
> I sometimes run with jspool = 2 just to test,
> just to see what happens with a real world memory error.

Good idea.

[snip]
> Set jspool = 2 and type or read in these two commands.
> 
> b http://en.wikipedia.org/wiki/http
> b http://en.wikipedia.org/wiki/https
> 
> I get something like this.
> 
> edbrowse ready
>  .
> 180413
>  .
> 34837

Wow, mine explodes on this one.

>  .
> 135113
>  .
> could not allocate memory for javascript operations.
> out of memory
Why haven't we stopped here?
Anything from this point on is probably going to go wrong.

> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> could not allocate memory for javascript operations.
> out of memory
> Unrecoverable JavaScript error in this session

And it does.

> *** glibc detected *** /home/eklhad/bin/edbrowse: free(): corrupted unsorted chunks: 0x0a4ca310 ***
> ======= Backtrace: =========

Ugh, double free? Not sure, have switched javaSessionFail to use
freeJavaContext in case.

Thanks for the testing, I'm currently going thrpough jsloc.cpp trying to fix things.

Cheers,
Adam.

Re: [Edbrowse-dev] segfault with small pool

[Adam Thompson]

On Wed, Feb 19, 2014 at 04:21:37AM -0500, Karl Dahlke wrote:
> While doing this, I was looking at my edbrowse wiki article,
> putting in more feature content as Adam suggested,
> and I was verifying some of the internal wiki links,
> and got a segfault.
Oops, should have debugged fully first.
Looks like it's the currently being addressed lack of error checking in
jsloc.cpp which is to blame here.
This is currently being fixed.

Cheers,
Adam.

Re: [Edbrowse-dev] segfault with small pool

[Adam Thompson]

On Wed, Feb 19, 2014 at 11:38:07AM +0000, Adam Thompson wrote:
> On Wed, Feb 19, 2014 at 04:21:37AM -0500, Karl Dahlke wrote:
> > While doing this, I was looking at my edbrowse wiki article,
> > putting in more feature content as Adam suggested,
> > and I was verifying some of the internal wiki links,
> > and got a segfault.
> Oops, should have debugged fully first.
> Looks like it's the currently being addressed lack of error checking in
> jsloc.cpp which is to blame here.
> This is currently being fixed.

Ok, fixed (and pushed fixes for) much of this,
as well as fixing freeJavaContext so it correctly handles compartments (where
correct = not failing asserts in my debug build of mozjs 24).
I'm now seeing a segfault in encodeTags relating to a stringAndChar call.
I'm having a go at debugging this, but it's a realloc segfault so I think I'll
have to look at it in valgrind to work out why it's exploding.

Cheers,
Adam.

Re: [Edbrowse-dev] segfault with small pool

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> I'm now seeing a segfault in encodeTags relating to a stringAndChar call.
> I'm having a go at debugging this, but it's a realloc segfault

What you're seeing is heap corruption.  I'm seeing a similar segfault in
malloc, called by allocZeroMem, called by newTag.
I have no idea what's causing it either, but I do have a theory.  After
the call to javaSessionFail(), we have lots of objects: JS::Rooted,
JS::Handle, JS::HeapRooted, and so forth, and they're still on the stack
or heap.  And they now reference a destroyed JSContext *.  The heap
corruption comes about when their destructors are called.
There's no easy fix for the stack-allocated stuff, I think.  We have to
insure that we don't have any dangling JS::Handle or JS::Rooted when we
call javaSessionFail.
As for the heap-rooted objects, maybe javaSessionFail could traverse the
tag list before the context is destroyed, eliminating all of our
references.  I'd put this logic into a function in html.cpp, since the
tag list is private to that file.  For example, it could look like this.

void freeHeapRooted(void) {
for tag in tag_list {
tag->ev = NULL;
}
}

Of course, this is all just a theory.  Maybe the heap corruption is
coming from elsewhere.  But this seems like a plausible candidate.

-- Chris

Re: [Edbrowse-dev] segfault with small pool

[Adam Thompson]

On Wed, Feb 19, 2014 at 08:07:28AM -0800, Chris Brannon wrote:
> Adam Thompson <arthompson1990@gmail.com> writes:
> 
> > I'm now seeing a segfault in encodeTags relating to a stringAndChar call.
> > I'm having a go at debugging this, but it's a realloc segfault
> 
> What you're seeing is heap corruption.  I'm seeing a similar segfault in
> malloc, called by allocZeroMem, called by newTag.
> I have no idea what's causing it either, but I do have a theory.  After
> the call to javaSessionFail(), we have lots of objects: JS::Rooted,
> JS::Handle, JS::HeapRooted, and so forth, and they're still on the stack
> or heap.  And they now reference a destroyed JSContext *.  The heap
> corruption comes about when their destructors are called.

I agree, after further debugging using valgrind,
gdb and also the electric fence library [1],
it looks like a case of heap corruption.

> There's no easy fix for the stack-allocated stuff, I think.  We have to
> insure that we don't have any dangling JS::Handle or JS::Rooted when we
> call javaSessionFail.

I'm not sure if that's even possible at the moment given the way some of the
functions work. I'd hope that what happens when we destroy the context is that
the context-linked roots also go away (which'd make sense).
Otherwise, I don't know how we can fix this.

> As for the heap-rooted objects, maybe javaSessionFail could traverse the
> tag list before the context is destroyed, eliminating all of our
> references.  I'd put this logic into a function in html.cpp, since the
> tag list is private to that file.  For example, it could look like this.
> 
> void freeHeapRooted(void) {
> for tag in tag_list {
> tag->ev = NULL;
> }
> }

As Karl says, this should be ok as far as not destroying heap rooted things goes.

> 
> Of course, this is all just a theory.  Maybe the heap corruption is
> coming from elsewhere.  But this seems like a plausible candidate.

I know when running with electric fence it aborts at JS_NewContext (same as the
debian segfault, I wonder), so I've got a suspician smjs has a few more hidden 
issues.
Whether these are causing ours I'm not sure.

I'll have a go at working out a way round the stack issue though.

Cheers,
Adam.
[1] http://en.wikipedia.org/wiki/Electric_Fence

Re: [Edbrowse-dev] segfault with small pool

[Chris Brannon]

Adam Thompson <arthompson1990@gmail.com> writes:

> I'm not sure if that's even possible at the moment given the way some of the
> functions work. I'd hope that what happens when we destroy the context is that
> the context-linked roots also go away (which'd make sense).

They don't go away.  I just verified that with gdb.
I'm starting to see a possible fix, I think.
In javaSessionFail, do not call freeJavaContext at all.
Add a new variable to cw: cw->jss_saved.
Now javaSessionFail looks like this:
cw->jss_saved = cw->jss;
cw->jss = NULL;

Now we have two cases to consider.
1. Spidermonkey ran out of memory while parsing the page, or
2. Spidermonkey ran out of memory when handling a JS event like onclick.

For case 1, when we've finished parsing the page,
call freeJavaContext(cw->jss_saved) if cw->jss_saved is not NULL.
Set cw->jss_saved to NULL so we don't double-free.
I think we can do this at the end of parsePage.  There will be no
stack-allocated JS roots at this point.

It's going to be a lot messier for case 2, I think.  Can we just do the
"free-if-saved" check after each call to handlerGo?

BTW, freeJavaContext also has a bug.

void freeJavaContext(struct ebWindowJSState *state)
{
	if (state == NULL)
return;
if (state->jcx != NULL)
{
if (js::GetContextCompartment((const JSContext *) state->jcx) != NULL)
JS_LeaveCompartment(state->jcx, NULL);
		JS_DestroyContext(state->jcx);
}
		delete state;
}				/* freeJavaContext */

But state has HeapRooted objects after the context is destroyed.  Really
needs to be:

void freeJavaContext(struct ebWindowJSState *state)
{
	if (state == NULL)
		return;
	JSContext *cx = state->jcx;
	delete state;
	if (cx != NULL) {
		if (js::GetContextCompartment((const JSContext *) cx) != NULL)
			JS_LeaveCompartment(cx, NULL);
		JS_DestroyContext(cx);
}
}				/* freeJavaContext */

Re: [Edbrowse-dev] segfault with small pool

[Adam Thompson]

On Wed, Feb 19, 2014 at 09:34:22AM -0800, Chris Brannon wrote:
> Adam Thompson <arthompson1990@gmail.com> writes:
> 
> > I'm not sure if that's even possible at the moment given the way some of the
> > functions work. I'd hope that what happens when we destroy the context is that
> > the context-linked roots also go away (which'd make sense).
> 
> They don't go away.  I just verified that with gdb.

Ugh, that's actually really annoying.
> I'm starting to see a possible fix, I think.
> In javaSessionFail, do not call freeJavaContext at all.
> Add a new variable to cw: cw->jss_saved.
> Now javaSessionFail looks like this:
> cw->jss_saved = cw->jss;
> cw->jss = NULL;
> 
> Now we have two cases to consider.
> 1. Spidermonkey ran out of memory while parsing the page, or
> 2. Spidermonkey ran out of memory when handling a JS event like onclick.
> 
> For case 1, when we've finished parsing the page,
> call freeJavaContext(cw->jss_saved) if cw->jss_saved is not NULL.
> Set cw->jss_saved to NULL so we don't double-free.
> I think we can do this at the end of parsePage.  There will be no
> stack-allocated JS roots at this point.

Yep, or set a flag in cw like js_failed and leave jss as non-null,
adding the relevant checks.
> 
> It's going to be a lot messier for case 2, I think.  Can we just do the
> "free-if-saved" check after each call to handlerGo?

Probably.

> 
> BTW, freeJavaContext also has a bug.

Yep, I pushed a fix for this one earlier today.

Cheers,
Adam.