Last Comment Bug 309337 - Embedding inside Java may result in stack < 512k
: Embedding inside Java may result in stack < 512k
: fixed1.8
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
-- normal (vote)
: ---
Assigned To: general
: Andrew Overholt [:overholt]
Depends on:
  Show dependency treegraph
Reported: 2005-09-20 09:20 PDT by jhp (no longer active)
Modified: 2008-07-31 02:43 PDT (History)
2 users (show)
asa: blocking1.8b5-
brendan: blocking1.8rc1+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

prevent wrapping of unsigned value (1.37 KB, patch)
2005-09-22 10:25 PDT, jhp (no longer active)
brendan: review+
jst: superreview+
brendan: approval1.8rc1+
Details | Diff | Splinter Review

Description User image jhp (no longer active) 2005-09-20 09:20:43 PDT;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 User image jhp (no longer active) 2005-09-22 10:25:18 PDT
Created attachment 197050 [details] [diff] [review]
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
Comment 2 User image jhp (no longer active) 2005-09-29 11:04:19 PDT
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 User image Asa Dotzler [:asa] 2005-09-30 12:44:40 PDT
we're not going to block the release on this. we'd condsider a fully reviewed,
near-zero risk patch. 
Comment 4 User image Johnny Stenback (:jst, 2005-10-04 16:57:40 PDT
Comment on attachment 197050 [details] [diff] [review]
prevent wrapping of unsigned value

Comment 5 User image Brendan Eich [:brendan] 2005-10-10 14:29:26 PDT
Comment on attachment 197050 [details] [diff] [review]
prevent wrapping of unsigned value

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

Comment 6 User image Brendan Eich [:brendan] 2005-10-10 14:31:22 PDT
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,

Comment 7 User image jhp (no longer active) 2005-10-11 11:30:41 PDT
Checked into trunk and branch. -> FIXED

Note You need to log in before you can comment on or make changes to this bug.