220408 - DOM needs to call JS_SetThreadStackLimit with right address

Status: RESOLVED FIXED

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

Description

[Brendan Eich [:brendan]]

See bug 192414 and its patches, especially bug 192414 attachment 132172 [details] [diff] [review], but
note bug 192414 comment 77.  Also see bug 96128 attachment 132092 [details] [diff] [review] -- thanks to
Igor Bukanov for pointing that out.

Rather than file a bug per OS, I'm filing this bug.  I'll let others decide
whether to file separate bugs, or use only this bug to do coordinated patching,
even if different OS's system calls or library APIs are needed to divine the
current thread/process stack limit.

/be

Comment 1

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm not sure what is ment by "DOM" in this bug. I don't think we want to every
time we're calling an eventhandler or callback or executing a script-element to
call the JS_SetThreadStackLimit method. First of all that would be hundreds of
callsites. Second, a lot of the time we don't know weather an object is
implemented in js or in c++.

Shouldn't it be enough to call JS_SetThreadStackLimit once per thread? The stack
for a thread should never move in memory, right?

Comment 2

[Brendan Eich [:brendan]]

Yes, that's what this bug asks for: call JS_SetThreadStackLimit once when
creating a JSContext for DOM use and abuse.

What made you think otherwise?  In no case that I know of would one want to call
JS_SetThreadStackLimit more often.  Thread stacks don't move in C'ish runtimes
(pthreads, windows), nor do maximum/sanity depth limits change over thread
lifetimes.

/be

Comment 3

[Johnny Stenback (:jst)]

Attachment: Attempt set reasonable stack limits.

Maybe something like this? Did I get the Mac part right?

Comment 4

[Johnny Stenback (:jst)]

I really don't like this guessing game we're playing here tho, wouldn't be cool
if NSPR could do this right for us, on all platforms?

Comment 5

[Brendan Eich [:brendan]]

Does OS X provide a useful StackSize() result?  That seems like an OS <=9 API
that doesn't fit into a BSD-Unix/Mach/NextOS world.

I too would love to see some NSPR veneer here.  Wtc?

/be

Comment 6

[timeless]

Comment on attachment 138206 [details] [diff] [review]
Attempt set reasonable stack limits.

>+#ifdef XP_MAC
That's os9

>+#ifdef XP_MAC
That's os9
>+    // For the mac we make the limit 16k below the current stack adds
'adds' is that [(v) add]+s? or add[y']s
>+    // + the stack size.
>+    sThreadStackLimit = currentStackAddr + StackSize() - 16384;
Personally I'd rather you use consistent notation for both sides of the branch
(16384 v. 0x80000) [0x4000, 0x80000], and you probably should use named
constants instead of magic numbers.
>+#else
>+    // On non-mac platforms (where the stack grows down) we set the
>+    // stack limit to 512k below the current stack addr.
>+    sThreadStackLimit = currentStackAddr - 0x80000;
>+#endif

I'm also not sure that you covered all of the cases. But I guess this is an
invitation to each and every amusing port (hpux-parisc) to dump its own amusing
code into this file (you've been warned).

Comment 7

[Johnny Stenback (:jst)]

Cc:ing pinkerton in hope that he'd have clues on how to deal with this on OS X.

Comment 8

[Mike Pinkerton (not reading bugmail)]

just some quick digging turned up that the default stack is 512k on darwin. is
|StackSize()| one of our routines? I can't find any documentation for it on
apple's site.

Comment 9

[Brendan Eich [:brendan]]

How about we use 512K as a universal stack depth limit at the start of 1.8a, and
see how that flies?

/be

Comment 10

[Johnny Stenback (:jst)]

I'm all for that.

Comment 11

[Igor Bukanov]

For Windows the stack overflow happens after cosuming 100-128K. Pehaps setting
the limit to 100K would a better idea to defend against overflow on a wide range
of paltforms.

Comment 12

[Igor Bukanov]

I just filed bug 237006 against NSPR to provide API for the initial stack
address with the goal of passing that address + 100K to JS_SetThreadStackLimit .

Comment 13

[Johnny Stenback (:jst)]

Hmm, looks like a 512k limit works fine on Win32 (at least on XP). I'll attach
an updated patch shortly...

Comment 14

[Johnny Stenback (:jst)]

Attachment: Updated diff, no more Mac specific code.

Comment 15

[Johnny Stenback (:jst)]

Attachment: Testcase, recurse to death.

Comment 16

[Brendan Eich [:brendan]]

Re: comment 11: Igor, can you say more about how you provoked a stack overflow
just 100K away from a low (near bottom) stack address?  That seems way small for
modern Windows.

/be

Comment 17

[Igor Bukanov]

Comment on attachment 143986 [details] [diff] [review]
Updated diff, no more Mac specific code.

>Index: dom/src/base/nsJSEnvironment.cpp
>+  // Store the thread stack limit in a static local to ensure that all
>+  // contexts get the same stack limit (they're all on the same thread
>+  // anyways), and this also helps prevent returning a stack limit
>+  // that is beyond the end of the stack if this method is called way
>+  // deep on the stack.

Perhaps I missed something but I always thought that an application are allowed
to create new thread, do some DOM/JS activity there and shutdown the thread to
repeat the process later on a different thread. In this case the static limit
would be very wrong, right?

Comment 18

[Johnny Stenback (:jst)]

JS that accesses the DOM must always run on the same thread. Any time that's not
true, anything can happen. None of the code involved attempts to be thread safe.

Comment 19

[Igor Bukanov]

(In reply to comment #16)
> Re: comment 11: Igor, can you say more about how you provoked a stack overflow
> just 100K away from a low (near bottom) stack address?  That seems way small for
> modern Windows.
> 
> /be

I did not check it myself but rather this is my interprettion of 
http://bugzilla.mozilla.org/show_bug.cgi?id=192414#c31 . Of cause, my assumtion
that it was necessary to set the stack to 128 and not, say 256 or 512 can be wrong.

Comment 20

[Igor Bukanov]

(In reply to comment #18)
> JS that accesses the DOM must always run on the same thread. Any time that's not
> true, anything can happen. None of the code involved attempts to be thread safe.

So it is not possible to embed DOM+JS into application that from time to time
create a special single thread to render some document, then destroy it and then
later create another one?

Comment 21

[Johnny Stenback (:jst)]

(In reply to comment #20)
> So it is not possible to embed DOM+JS into application that from time to time
> create a special single thread to render some document, then destroy it and then
> later create another one?

Well, anything is possible, but if you access Mozilla's DOM/Layout/most anything
on threads other than Mozilla's main thread, you're on your own.

Comment 22

[Igor Bukanov]

(In reply to comment #21)
> if you access Mozilla's DOM/Layout/most anything
> on threads other than Mozilla's main thread, you're on your own.

Ok, the static is ok then: when time of multithreading layout comes (which is
quite distant future as I understand it now), the static limit from the first
thread most likely would be too strict and jseng would report an error about too
much recursion and then it would be time to deal with the bug again and review
static assumtions. 

But for now Mozilla would be at least immune against stack overflow out of the box.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

would be nice if we could use the JS_STACK_GROWTH_DIRECTION define to detect the
direction. Either by making another define that isn't js-only, or by making
JS_SetThreadStackLimit take a stack-start and stack-size

Comment 24

[Brendan Eich [:brendan]]

Why do you need another define?  JS_STACK_GROWTH_DIRECTION is public (jsapi.h =>
jspubtd.h => jstypes.h => jscpucfg.h).

/be

Comment 25

[Johnny Stenback (:jst)]

We could make the patch do:

+    sThreadStackLimit = currentStackAddr +
+      (0x80000 * JS_STACK_GROWTH_DIRECTION);

(or something along those lines) in stead of assuming that the stack always
grows down (by simply relying on that assumption in the JS engine).

Let me know if you think it's worth doing.

Comment 26

[Brendan Eich [:brendan]]

*** Bug 234389 has been marked as a duplicate of this bug. ***

Comment 27

[Brendan Eich [:brendan]]

*** Bug 228442 has been marked as a duplicate of this bug. ***

Comment 28

[Brendan Eich [:brendan]]

Yeah, do that and I'll r= and we can try for 1.7.

/be

Comment 29

[Brendan Eich [:brendan]]

*** Bug 96128 has been marked as a duplicate of this bug. ***

Comment 30

[Johnny Stenback (:jst)]

Attachment: Use the JS engine's assumption about stack-growth direction.

Comment 31

[Brendan Eich [:brendan]]

Comment on attachment 145579 [details] [diff] [review]
Use the JS engine's assumption about stack-growth direction.

r+sr=me again.

/be

Comment 32

[Johnny Stenback (:jst)]

Comment on attachment 145579 [details] [diff] [review]
Use the JS engine's assumption about stack-growth direction.

This bug's gotten a bunch of dupes, and it'll fix many stack-overflow crashes
that are yet to be found. IMO worth it for 1.7 final.

Comment 33

[chris hofmann]

Comment on attachment 145579 [details] [diff] [review]
Use the JS engine's assumption about stack-growth direction.

a=chofmann for 1.7

Comment 34

[Johnny Stenback (:jst)]

Fix checked in!