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 treegraph
 
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

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>.