Closed Bug 678623 Opened 11 years ago Closed 11 years ago

"too much recursion" with physics engine in worker thread


(Core :: JavaScript Engine, defect)

Not set





(Reporter: azakai, Assigned: azakai)





(2 files, 1 obsolete file)

Attached file stack trace
The worker version of the physics engine demo in bug 653967,

fails with "too much recursion" happening in the worker thread. However the non-worker version of that demo, which loads the same physics engine (just not in a worker) works ok,

This problem is sensitive to something, not sure what: I get it on Nightly on linux and when I build with default settings, but I do not get it when I build with --disable-optimize for example. bent can't see it on os x.

Attaching a stack trace for this error. js_EmitTree occurs dozens of times calling itself recursively, eventually leading to js_ReportOverRecursed being run.
Attached patch patch (obsolete) — Splinter Review
This patch sets the stack quota to 128*sizeof(size_t)*1024, which is the same as elsewhere (TabChild, nsInProcessTabChildGlobal). With this the demo works fine.
Attachment #552785 - Flags: review?(bent.mozilla)
I don't know if this is safe... Does the mochitest dom/workers/test/test_recursion.html pass without crashing? I tuned this to 32 on OS X because that was the largest value I could use without that test crashing...
I sent to try half an hour ago. I guess I'll see it crash on OS X there...

Do you know if the code in the worker in that test crashes OS X when run normally (not in a worker)? If so, then the other places we currently set the stack size to the value in the patch are dangerous. If not, then I wonder what is different between the worker and non-worker case.
As far as I can see, we never pick a stack size - all our thread creations use the default platform values. And I believe those values are 8MB for Linux, 1MB for Windows, 512K for OS X. So I guess that can explain why OS X would be most sensitive to this problem - the JS stack size limit we set on a 32-bit platform is 512K.

If that is the situation, perhaps a solution is to set the stack size to 1MB on OS X? I worry about us doing such a change though.
Bug 664341 reduced the stack size for media threads on Linux.
Attached patch patch v2Splinter Review
This patch sets the stack size to 1MB and the JS stack limit to 512K (on 32-bit systems), and passes try.

The patch uses the same stack size on all OSes instead of using the defaults. This seems safer since we are now able to be sure that the JS stack limit will be smaller (the crashes before were apparently due to the JS stack limit being equal to the native stack size, and we overflowed).
Attachment #552785 - Attachment is obsolete: true
Attachment #552785 - Flags: review?(bent.mozilla)
Attachment #553309 - Flags: review?(bent.mozilla)
Comment on attachment 553309 [details] [diff] [review]
patch v2

Review of attachment 553309 [details] [diff] [review]:

Sounds good! Thanks.

::: dom/workers/RuntimeService.cpp
@@ +78,5 @@
>  #define WORKER_RUNTIME_HEAPSIZE 32 * 1024 * 1024
> +// The C stack size. We use the same stack size on all platforms for
> +// consistency.
> +#define WORKER_STACKSIZE 256 * sizeof(size_t) * 1024


@@ +804,5 @@
>    if (!thread) {
> +    if (NS_FAILED(NS_NewThread(getter_AddRefs(thread),
> +                               nsnull,
> +                               WORKER_STACKSIZE))) {

Nit: keep nsnull on the first line, only wrap what you have to.
Attachment #553309 - Flags: review?(bent.mozilla) → review+

Could we check in a test here?
Assignee: general → azakai
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
The test is "Does dom/workers/test/test_recursion.html crash the browser?"
Flags: in-testsuite?
We can also add a test for the other direction, that is, to test if we can run huge scripts in workers without hitting "too much recursion". I don't see how to do that though except to add a test with the demo in this bug (2MB), and I assumed people would object to that. Or not?
Ever confirmed: false
Resolution: FIXED → ---
Target Milestone: mozilla8 → ---
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.