Embedding inside Java may result in stack < 512k




DOM: Core & HTML
12 years ago
9 years ago


(Reporter: jhp (no longer active), Unassigned)



Bug Flags:
blocking1.8b5 -
blocking1.8rc1 +

Firefox Tracking Flags

(Not tracked)



(1 attachment)



12 years ago

|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

12 years ago
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
Attachment #197050 - Flags: review?(brendan)


12 years ago
Attachment #197050 - Flags: superreview?(jst)

Comment 2

12 years ago
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.  
Flags: blocking1.8b5?

Comment 3

12 years ago
we're not going to block the release on this. we'd condsider a fully reviewed,
near-zero risk patch. 
Flags: blocking1.8b5? → blocking1.8b5-
Comment on attachment 197050 [details] [diff] [review]
prevent wrapping of unsigned value

Attachment #197050 - Flags: superreview?(jst) → superreview+
Comment on attachment 197050 [details] [diff] [review]
prevent wrapping of unsigned value

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

Attachment #197050 - Flags: review?(brendan)
Attachment #197050 - Flags: review+
Attachment #197050 - Flags: approval1.8rc1+
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,

Flags: blocking1.8rc1+


12 years ago
Blocks: 307451

Comment 7

12 years ago
Checked into trunk and branch. -> FIXED
No longer blocks: 307451
Last Resolved: 12 years ago
Keywords: fixed1.8
Resolution: --- → FIXED


9 years ago
Component: DOM: Core → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.