309337 - Embedding inside Java may result in stack < 512k

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[jhp (no longer active)]

http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=4689767
http://bugs.sun.com/bugdatabase/view_bug.do;jsessionid=5fdf676125dd7b3cc88af53331625:YfiG?bug_id=6316197

|GetThreadStackLimit()| in 'mozilla/dom/src/base/nsJSEnvironment.cpp' makes the
assumption that there is at least 512k left on the stack, and makes calculations
accordingly.  But when embedding Mozilla from Java, this is not necessarily the
case, as you can read about in the first link above.  Specifically on Win32, the
main Java thread will have a stack of only 256k (or 320k in later versions).

The specific problem is that |sThreadStackLimit| is only several hunder K above
0, such that subtracting 512k 'wraps' the |sThreadStackLimit| to a very large
value, and all JS calls immediately result in "too much recursion" errors.

The easy immediate fix is to put back the checks that were deleted by bug 192414
attachment 132216 [details] [diff] [review].  I'm not sure if we would want to add any Win32 calls to
check for the real end of the stack.

So as mentioned in the two links above, only Win32 is affected (and OS/2 I
guess).  A workaround would be for the Java embedder to spawn a thread that
actually did the embedding, since the thread stack size can be controlled by the
"-Xss" parameter to java.exe.  But I don't think we want require that.  What do
you guys think?

Comment 1

[jhp (no longer active)]

Attachment: prevent wrapping of unsigned value

At the very least, this patch prevents the wrapping that I'm seeing of
|sThreadStackLimit|.  However, I put printfs in the js code for the places that
call |JS_CHECK_STACK_SIZE| while running jst's recursions testcase at
attachment 143988 [details], but I never see the stack get any where near the stackLimit.
 Instead, it hits the other recursion check |if (inlineCallCount ==
MAX_INLINE_CALL_COUNT)| in jsinterp.c.	Is there a testcase that would actually
test out |JS_CHECK_STACK_SIZE| without hitting the other recursion checks
first?

Comment 2

[jhp (no longer active)]

It would be great to get this into the next XULrunner/libXUL release on beta5,
since there's a group that wants to use this with Javaconnect, and this patch
makes it useable on win32.  

Comment 3

[Asa Dotzler [:asa]]

we're not going to block the release on this. we'd condsider a fully reviewed,
near-zero risk patch. 

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 197050 [details] [diff] [review]
prevent wrapping of unsigned value

sr=jst

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 197050 [details] [diff] [review]
prevent wrapping of unsigned value

This should go in for rc1, sorry for the delay.

/be

Comment 6

[Brendan Eich [:brendan]]

This is safe, anyone getting near the null page is going to die on sane OSes. 
Java can't be changed, so we need to cope.

Javier, can you check into trunk and 1.8 branch?  Thanks,

/be

Comment 7

[jhp (no longer active)]

Checked into trunk and branch. -> FIXED