Bug 184744 – Propagate stack overflow and OOM when compiling builtins at runtime

• Status: RESOLVED
• Resolution: CONFIGURATION CHANGED
• Component: JavaScriptCore

[JF Bastien]

Some builtins, such as createDefaultConstructor, can get compiled at runtime and fail because of OOM or stack overflow. We should let that propagate out to JavaScript instead of crashing. Builtins which are generated at VM startup should remain hard errors.

[JF Bastien]

<rdar://problem/39260348>

[JF Bastien]

Created attachment 338233 [details]
patch

[Mark Lam]

Comment on attachment 338233 [details]
patch

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

As per our offline discussion, since this issue does not reproduce on Darwin, can you please hack the code to force a StackOverflow error after some N number of recursions, and verify that the fix does work as intended, and that it doesn't result in some unexpected failure condition?

r=me with fixes and testing.

> JSTests/ChangeLog:9
> +        * stress/create-default-constuctor-near-stack-limit.js: Added.

Please fix the name of this file: /constuctor/constructor/.

> Source/JavaScriptCore/ChangeLog:17
> +        done in two ways:

nit: I would say "handled" instead of "done".

> Source/JavaScriptCore/ChangeLog:20
> +        users the error type), which then puts a JS error on the execution

typo: /users/uses/?

[EWS Watchlist]

Comment on attachment 338233 [details]
patch

Attachment 338233 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7362415

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html

[EWS Watchlist]

Created attachment 338264 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Saam Barati]

(In reply to Mark Lam from comment #3)
> Comment on attachment 338233 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338233&action=review
> 
> As per our offline discussion, since this issue does not reproduce on
> Darwin, can you please hack the code to force a StackOverflow error after
> some N number of recursions, and verify that the fix does work as intended,
> and that it doesn't result in some unexpected failure condition?

I suggested a way to make a test to JF online. JF any luck?

> 
> r=me with fixes and testing.
> 
> > JSTests/ChangeLog:9
> > +        * stress/create-default-constuctor-near-stack-limit.js: Added.
> 
> Please fix the name of this file: /constuctor/constructor/.
> 
> > Source/JavaScriptCore/ChangeLog:17
> > +        done in two ways:
> 
> nit: I would say "handled" instead of "done".
> 
> > Source/JavaScriptCore/ChangeLog:20
> > +        users the error type), which then puts a JS error on the execution
> 
> typo: /users/uses/?

[Robin Morisset]

This appears to be an exact duplicate of https://bugs.webkit.org/show_bug.cgi?id=184074, in which I solved the exact same bug (but it was reverted due to a mistake in my handling of the exception throw scope).

I am not sure exactly why my patch had to be so much larger than yours, maybe because I tried to plumb a js exception all the way through in more cases.

[JF Bastien]

(In reply to Robin Morisset from comment #7)
> This appears to be an exact duplicate of
> https://bugs.webkit.org/show_bug.cgi?id=184074, in which I solved the exact
> same bug (but it was reverted due to a mistake in my handling of the
> exception throw scope).
> 
> I am not sure exactly why my patch had to be so much larger than yours,
> maybe because I tried to plumb a js exception all the way through in more
> cases.

Right now I've fixed some of the issues, but I think I'll end up doing almost what you did because of this:

    frame #1: 0x0000000100766420 JavaScriptCore`JSC::BytecodeGenerator::emitNewDefaultConstructor(this=0x0000000107850b40, dst=0x00000001078c2648, constructorKind=Base, name=0x0000000107cf9000, ecmaName=0x0000000107cf9000, classSource=0x0000000107be6078) at BytecodeGenerator.cpp:3334
    frame #2: 0x0000000100791fdb JavaScriptCore`JSC::ClassExprNode::emitBytecode(this=0x0000000107be6010, generator=0x0000000107850b40, dst=0x00000001078c2648) at NodesCodegen.cpp:3987
    frame #3: 0x000000010078717d JavaScriptCore`JSC::BytecodeGenerator::emitNodeInTailPosition(this=0x0000000107850b40, dst=0x00000001078c2648, n=0x0000000107be6010) at BytecodeGenerator.h:521
    frame #4: 0x000000010075a15e JavaScriptCore`JSC::BytecodeGenerator::emitNode(this=0x0000000107850b40, dst=0x00000001078c2648, n=0x0000000107be6010) at BytecodeGenerator.h:510
    frame #5: 0x0000000100791de9 JavaScriptCore`JSC::ClassExprNode::emitBytecode(this=0x0000000107be60b8, generator=0x0000000107850b40, dst=0x0000000000000000) at NodesCodegen.cpp:3973
    frame #6: 0x000000010078717d JavaScriptCore`JSC::BytecodeGenerator::emitNodeInTailPosition(this=0x0000000107850b40, dst=0x0000000000000000, n=0x0000000107be60b8) at BytecodeGenerator.h:521
    frame #7: 0x000000010075a15e JavaScriptCore`JSC::BytecodeGenerator::emitNode(this=0x0000000107850b40, dst=0x0000000000000000, n=0x0000000107be60b8) at BytecodeGenerator.h:510
    frame #8: 0x0000000100772644 JavaScriptCore`JSC::BytecodeGenerator::emitNode(this=0x0000000107850b40, n=0x0000000107be60b8) at BytecodeGenerator.h:526
    frame #9: 0x000000010077822a JavaScriptCore`JSC::NewExprNode::emitBytecode(this=0x0000000107be6168, generator=0x0000000107850b40, dst=0x00000001078c263c) at NodesCodegen.cpp:785
    frame #10: 0x000000010078717d JavaScriptCore`JSC::BytecodeGenerator::emitNodeInTailPosition(this=0x0000000107850b40, dst=0x00000001078c263c, n=0x0000000107be6168) at BytecodeGenerator.h:521
    frame #11: 0x000000010075a15e JavaScriptCore`JSC::BytecodeGenerator::emitNode(this=0x0000000107850b40, dst=0x00000001078c263c, n=0x0000000107be6168) at BytecodeGenerator.h:510
    frame #12: 0x000000010078a349 JavaScriptCore`JSC::ExprStatementNode::emitBytecode(this=0x0000000107be61b8, generator=0x0000000107850b40, dst=0x00000001078c263c) at NodesCodegen.cpp:2701
    frame #13: 0x000000010078b0ed JavaScriptCore`JSC::BytecodeGenerator::emitNodeInTailPosition(this=0x0000000107850b40, dst=0x00000001078c263c, n=0x0000000107be61b8) at BytecodeGenerator.h:494
    frame #14: 0x000000010078a223 JavaScriptCore`JSC::SourceElements::emitBytecode(this=0x0000000107be6000, generator=0x0000000107850b40, dst=0x00000001078c263c) at NodesCodegen.cpp:2667
    frame #15: 0x0000000100790277 JavaScriptCore`JSC::ScopeNode::emitStatementsBytecode(this=0x00000001078908d0, generator=0x0000000107850b40, dst=0x00000001078c263c) at NodesCodegen.cpp:3689
    frame #16: 0x000000010078ffb5 JavaScriptCore`JSC::emitProgramNodeBytecode(generator=0x0000000107850b40, scopeNode=0x00000001078908d0) at NodesCodegen.cpp:3699
    frame #17: 0x000000010078fec4 JavaScriptCore`JSC::ProgramNode::emitBytecode(this=0x00000001078908d0, generator=0x0000000107850b40, (null)=0x0000000000000000) at NodesCodegen.cpp:3709
    frame #18: 0x000000010074acbd JavaScriptCore`JSC::BytecodeGenerator::generate(this=0x0000000107850b40) at BytecodeGenerator.cpp:152
    frame #19: 0x0000000101362ea0 JavaScriptCore`JSC::ParserError JSC::BytecodeGenerator::generate<JSC::ProgramNode, JSC::UnlinkedProgramCodeBlock>(vm=0x0000000107900000, node=0x00000001078908d0, sourceCode=0x0000000107d88358, unlinkedCodeBlock=0x0000000107d84870, debuggerMode=DebuggerOff, environment=0x00007ffeefbfd050) at BytecodeGenerator.h:390
    frame #20: 0x0000000101362803 JavaScriptCore`JSC::UnlinkedProgramCodeBlock* JSC::generateUnlinkedCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(vm=0x0000000107900000, executable=0x0000000107d88300, source=0x0000000107d88358, strictMode=NotStrict, scriptMode=Classic, debuggerMode=DebuggerOff, error=0x00007ffeefbfd4d8, evalContextType=None, variablesUnderTDZ=0x00007ffeefbfd050) at CodeCache.h:251
    frame #21: 0x0000000101324230 JavaScriptCore`JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getUnlinkedGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(this=0x00000001078eb058, vm=0x0000000107900000, executable=0x0000000107d88300, source=0x0000000107d88358, strictMode=NotStrict, scriptMode=Classic, debuggerMode=DebuggerOff, error=0x00007ffeefbfd4d8, evalContextType=None) at CodeCache.cpp:75
    frame #22: 0x0000000101323e18 JavaScriptCore`JSC::CodeCache::getUnlinkedProgramCodeBlock(this=0x00000001078eb058, vm=0x0000000107900000, executable=0x0000000107d88300, source=0x0000000107d88358, strictMode=NotStrict, debuggerMode=DebuggerOff, error=0x00007ffeefbfd4d8) at CodeCache.cpp:85
    frame #23: 0x0000000101533753 JavaScriptCore`JSC::ProgramExecutable::initializeGlobalProperties(this=0x0000000107d88300, vm=0x0000000107900000, callFrame=0x0000000107de0048, scope=0x0000000107dcc000) at ProgramExecutable.cpp:98
    frame #24: 0x00000001010a355e JavaScriptCore`JSC::Interpreter::executeProgram(this=0x00000001078fe200, source=0x00007ffeefbfefd0, callFrame=0x0000000107de0048, thisObj=0x0000000107da8080) at Interpreter.cpp:938

Basically, emitNewDefaultConstructor can be taught about stack overflow and OOM, but then I'd need to create a JS exception at that point. We have access to VM there, but it seems pretty icky to just do that and let it just flow upwards tot he CodeCache.

Maybe it's better to fix your patch and re-land it?

[JF Bastien]

Created attachment 338336 [details]
hacky patch, not for landing

Here's an updated patch which starts propagating some of the errors, but I think we want to go with Robin's instead since he did the whole piping.

My patch also has a hacky thing to stack overflow after the VM initializes and we start interpreting. It's of course a hack for testing.

[Robin Morisset]

Created attachment 338412 [details]
Patch

[Robin Morisset]

(In reply to Robin Morisset from comment #10)
> Created attachment 338412 [details]
> Patch

This patch relies on the patch I just uploaded for https://bugs.webkit.org/show_bug.cgi?id=184074

It is also currently missing a testcase (since I have not managed to get a reliable testcase for this on Mac), and a Changelog. I only uploaded it in case JF wants to take  a look at it.
It is a straightforward extension of my patch for https://bugs.webkit.org/show_bug.cgi?id=184074 to also cover createDefaultConstructor.

[JF Bastien]

Comment on attachment 338412 [details]
Patch

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

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3332
> +            emitThrowOutOfMemoryError();

As we discussed offline this isn't quite right: you'll indeed throw the right exception at the right time, but then this code goes in the code cache and you'll throw it again if the user tries to recover! Say you go out of stack, the stack unwinds enough, and you try the same thing, you'll erroneously fail.

[Filip Pizlo]

(In reply to JF Bastien from comment #12)
> Comment on attachment 338412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338412&action=review
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3332
> > +            emitThrowOutOfMemoryError();
> 
> As we discussed offline this isn't quite right: you'll indeed throw the
> right exception at the right time, but then this code goes in the code cache
> and you'll throw it again if the user tries to recover! Say you go out of
> stack, the stack unwinds enough, and you try the same thing, you'll
> erroneously fail.

That's a progression over what we have now, so I don't think this is a reason to not land the patch.

[Saam Barati]

I'm taking this over

[Saam Barati]

I think this will be fixed in:
https://bugs.webkit.org/show_bug.cgi?id=184074

[Saam Barati]

This is fixed now that we don’t use the parser in BuiltinExecutables::createExecutable