Bug 117862 - REGRESSION(r151808): fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html fails
Summary: REGRESSION(r151808): fast/xmlhttprequest/xmlhttprequest-recursive-sync-event....
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: LayoutTestFailure, Regression
Depends on: 117801
Blocks:
  Show dependency tree / graph
 
Reported: 2013-06-20 20:58 PDT by Ryosuke Niwa
Modified: 2013-06-21 16:59 PDT (History)
2 users (show)

See Also:

Attachments
the fix (19.15 KB, patch)
2013-06-21 13:22 PDT, Mark Lam 		no flags Details | Formatted Diff | Diff
patch #2 (20.08 KB, patch)
2013-06-21 13:32 PDT, Mark Lam 		ggaren: review+
Details | Formatted Diff | Diff
Show Obsolete (1) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-06-20 20:58:38 PDT 
fast/xmlhttprequest/xmlhttprequest-recursive-sync-event.html has been failing since http://trac.webkit.org/changeset/151808
Comment 1 Ryosuke Niwa 2013-06-20 21:01:08 PDT 
Committed r151823: <http://trac.webkit.org/changeset/151823>
Comment 2 Mark Lam 2013-06-21 13:22:48 PDT 
Created attachment 205210 [details]
the fix
Comment 3 Mark Lam 2013-06-21 13:32:07 PDT 
Created attachment 205212 [details]
patch #2
Comment 4 Geoffrey Garen 2013-06-21 15:57:37 PDT 
Comment on attachment 205212 [details]
patch #2

View in context: https://bugs.webkit.org/attachment.cgi?id=205212&action=review

r=me

> Source/JavaScriptCore/interpreter/Interpreter.cpp:-130
> -    const size_t requiredStack = 32 * KB;
> -    const size_t errorModeRequiredStack = 16 * KB;

I didn't realize that these values had just shrunk in r151808. Can we make them a little bigger? How about 128 required / 64 errorModeRequired? Would that work on Windows? 256 => 32 shrank by 8X, which seems like a risky move.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:794
> +    const VMStackBounds nativeStack(vm, wtfThreadData().stack());

Since the class is named VMStackBounds, the variable should be named vmStackBounds.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:959
> +    const VMStackBounds nativeStack(vm, wtfThreadData().stack());

Ditto.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1036
> +    const VMStackBounds nativeStack(vm, wtfThreadData().stack());

Ditto.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1116
> +    const VMStackBounds nativeStack(vm, wtfThreadData().stack());

Ditto.

> Source/JavaScriptCore/interpreter/Interpreter.cpp:1218
> +    const VMStackBounds nativeStack(vm, wtfThreadData().stack());

Ditto.
Comment 5 Mark Lam 2013-06-21 16:15:29 PDT 
(In reply to comment #4)
> I didn't realize that these values had just shrunk in r151808. Can we make them a little bigger? How about 128 required / 64 errorModeRequired? Would that work on Windows? 256 => 32 shrank by 8X, which seems like a risky move.

128/64 checks out fine for run-javascript-tests on Windows, where the256/64 values failed previously.  I will adopt these values and rerun the full tests on Mac.
Comment 6 Mark Lam 2013-06-21 16:59:59 PDT 
Landed in r151869: <http://trac.webkit.org/changeset/151869>.