Bug 29797 - [V8] Runaway JS recursion crashes Chromium workers on OS X
Summary: [V8] Runaway JS recursion crashes Chromium workers on OS X
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac (Intel) OS X 10.6
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-28 00:24 PDT by Dominic Cooney
Modified: 2009-10-11 15:17 PDT (History)
5 users (show)

See Also:


Attachments
Sets a stack limit. Do not land before proposed fix lands in V8--see description. (3.36 KB, patch)
2009-09-28 00:24 PDT, Dominic Cooney
dglazkov: review-
Details | Formatted Diff | Diff
Proposed fix. Uses the fixed V8 resource constraints API to set a stack limit for Chromium JavaScript workers. (3.32 KB, patch)
2009-10-10 22:54 PDT, Dominic Cooney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dominic Cooney 2009-09-28 00:24:51 PDT
Created attachment 40218 [details]
Sets a stack limit. Do not land before proposed fix lands in V8--see description.

Repro: Open LayoutTests/fast/workers/use-machine-stack.html.

Expected behavior: it prints PASS (RangeError: Maximum call stack size exceeded.)

Actual behavior:
Safari 4.0.3 (6531.9): OK.
Google Chrome 4.0.212.1 (Official Build 26852) WebKit 532.1 V8 1.3.11.1: Worker process crashes.

This is related to <http://crbug.com/21653>.

I've attached a patch. The patch depends on a proposed fix to V8 bug 442, see <http://codereview.chromium.org/242014>.
Comment 1 Dimitri Glazkov (Google) 2009-09-28 07:27:31 PDT
We typically don't modify tests to fix them. Can you explain why we have to change the test? Is it failing for JSC?
Comment 2 Dominic Cooney 2009-09-28 10:22:32 PDT
It isn't necessary to change the test. I added a stack overflow in a setTimeout callback to exercise stack overflow on another code path. I could revert the test change... either way.

The current test and the updated test pass with WebKit.
Comment 3 Dimitri Glazkov (Google) 2009-09-28 11:19:45 PDT
Comment on attachment 40218 [details]
Sets a stack limit. Do not land before proposed fix lands in V8--see description.

> +        Reviewed by NOBODY (OOPS!).
> +
> +        Sets a stack limit for V8 workers.
Need a better description of this test change and a bug URL.


>  
> +function h() {
> +    try {
> +        g();
> +    } catch (ex) {
> +        postMessage(ex);
> +    }
> +}
> +
>  try {
>      f();
>  } catch (ex) {
>      try {
>          g();
>      } catch (ex) {
> -        postMessage(ex);
> +        setTimeout(h, 0);
>      }

perhaps this could be a new test?

> +        Sets a stack limit for V8 workers.

Bug url is needed here. Also, make sure to prefix V8-related bug titles with [V8].
Comment 4 Dominic Cooney 2009-10-10 22:51:30 PDT
Runaway recursion in JavaScript workers crashes the Chromium worker process on OS X. This is because V8's default stack limit is 512K on ia32 or 1M on x64, but the worker process runs workers on a thread with the OS X default stack size--512K. Because there are already some C++ frames on the stack when V8 establishes its 512K default stack limit, and V8 doesn't precisely enforce the stack limit, runaway recursion in V8 workers overflows the OS stack and segfaults, killing the worker process. This is described in Chromium bug 21653 <http://crbug.com/21653>.
Comment 5 Dominic Cooney 2009-10-10 22:54:49 PDT
Created attachment 40999 [details]
Proposed fix. Uses the fixed V8 resource constraints API to set a stack limit for Chromium JavaScript workers.
Comment 6 Adam Barth 2009-10-11 00:12:52 PDT
Comment on attachment 40999 [details]
Proposed fix. Uses the fixed V8 resource constraints API to set a stack limit for Chromium JavaScript workers.

Thanks!  If you'd like the commit-queue to land this patch, you can nominate it by setting commit-queue=?
Comment 7 WebKit Commit Bot 2009-10-11 15:17:24 PDT
Comment on attachment 40999 [details]
Proposed fix. Uses the fixed V8 resource constraints API to set a stack limit for Chromium JavaScript workers.

Clearing flags on attachment: 40999

Committed r49427: <http://trac.webkit.org/changeset/49427>
Comment 8 WebKit Commit Bot 2009-10-11 15:17:28 PDT
All reviewed patches have been landed.  Closing bug.