REGRESSION (r180248): Repro Crash: com.apple.WebKit.WebContent at com.apple.JavaScriptCore: JSC::createRangeError + 20

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Saboff]

We are crashing trying to execute a JavaScript main program.

<rdar://problem/21659085>

[Michael Saboff]

Created attachment 256484 [details]
Patch

This also fixes LayoutTests/js/function-apply-aliased.html when run with the C Loop.

[Mark Lam]

Comment on attachment 256484 [details]
Patch

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

> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-478
> -    // This stack check is done in the prologue for a function call, and the
> -    // CallFrame is not completely set up yet. For example, if the frame needs
> -    // a lexical environment object, the lexical environment object will only be
> -    // set up after we start executing the function. If we need to throw a
> -    // StackOverflowError here, then we need to tell the prologue to start the
> -    // stack unwinding from the caller frame (which is fully set up) instead.
> -    // To do that, we return the caller's CallFrame in the second return value.
> -    //

The original patch that added this comment is http://trac.webkit.org/changeset/161084 (see https://bugs.webkit.org/show_bug.cgi?id=126174).  It was added to fix a test failure: jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit.  However, the key change for that patch was to return a non-zero exec value (previously returning a null exec even when an exception is thrown).  The code that accesses the uninitialized (not fully initialized) frame is in the caller of stack_check().  The use of exec = exec->callerFrame() existed since the introduction of the LLINT (see http://trac.webkit.org/changeset/108444).

If all the tests passes with this patch, we have high confidence that it is good.  However, to be safe, I'd look into what frame values are left to be initialized in the function prologue (i.e. the frame fields that remain uninitialized at this point), and either initialized them to some sane value, or at least make sure that they won't ever be used in the exception handling / unwinding code.  I'm nervous that we are setting loose a partially initialized frame on the VM.

[Geoffrey Garen]

Comment on attachment 256484 [details]
Patch

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

r=me

It seems like

>> Source/JavaScriptCore/llint/LLIntSlowPaths.cpp:-478
>> -    //
> 
> The original patch that added this comment is http://trac.webkit.org/changeset/161084 (see https://bugs.webkit.org/show_bug.cgi?id=126174).  It was added to fix a test failure: jsc-layout-tests.yaml/js/script-tests/function-apply-aliased.js.layout-no-cjit.  However, the key change for that patch was to return a non-zero exec value (previously returning a null exec even when an exception is thrown).  The code that accesses the uninitialized (not fully initialized) frame is in the caller of stack_check().  The use of exec = exec->callerFrame() existed since the introduction of the LLINT (see http://trac.webkit.org/changeset/108444).
> 
> If all the tests passes with this patch, we have high confidence that it is good.  However, to be safe, I'd look into what frame values are left to be initialized in the function prologue (i.e. the frame fields that remain uninitialized at this point), and either initialized them to some sane value, or at least make sure that they won't ever be used in the exception handling / unwinding code.  I'm nervous that we are setting loose a partially initialized frame on the VM.

It looks like the patch that added this code was about activation tear-off, which doesn't exist anymore. Perhaps we need to remove the two other uses of "exec = exec->callerFrame" preceding CommonSlowPaths::interpreterThrowInCaller as well.

> LayoutTests/http/tests/misc/large-js-program.php:33
> +for ($i = 0; $i < 1000000; $i++) {
> +    if ($i != 0)
> +        echo ",\n";
> +    echo "[0, $i]";

This could be our awesomest test yet!

[Mark Lam]

Michael has convinced me that the call frame is indeed properly set up by the time we get to the prologue where we do the stack check.  So, there is no cause for concern.

r=me too.

[Michael Saboff]

(In reply to comment #3)

> > If all the tests passes with this patch, we have high confidence that it is good.  However, to be safe, I'd look into what frame values are left to be initialized in the function prologue (i.e. the frame fields that remain uninitialized at this point), and either initialized them to some sane value, or at least make sure that they won't ever be used in the exception handling / unwinding code.  I'm nervous that we are setting loose a partially initialized frame on the VM.

As Mark replied, we looked at the code together and believe that the call frame is properly initialized for exception handling and unwinding before we make this check.

> It looks like the patch that added this code was about activation tear-off,
> which doesn't exist anymore. Perhaps we need to remove the two other uses of
> "exec = exec->callerFrame" preceding
> CommonSlowPaths::interpreterThrowInCaller as well.

Those are for the case that we have a stack overflow during arity check.  It actually makes sense that the exception is thrown from the caller and not the callee, as a call with proper arity could not be made due to stack overflow.

[Michael Saboff]

Committed r186606: <http://trac.webkit.org/changeset/186606>

[Brent Fulgham]

This new test fails repeatably on Windows. Can you please take a look?

[Michael Saboff]

(In reply to comment #7)
> This new test fails repeatably on Windows. Can you please take a look?

Looking at it now.

[Mark Lam]

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