Bug 184074 – We shouldn't recurse into the parser when gathering metadata about various function offsets

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

Fixing this will require quite a lot of plumbing, since throwing an exception needs both the ParserError and an ExecState, and there is a dozen levels to the call stack between the last place to get an ExecState and createExecutable (that currently crashes upon encountering a ParserError).
<rdar://problem/37165897>

[Robin Morisset]

<rdar://problem/37165897>

[Robin Morisset]

Created attachment 336656 [details]
WIP

[Robin Morisset]

Created attachment 336770 [details]
Patch

[EWS Watchlist]

This patch modifies the JS builtins code generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-builtins-generator-tests --reset-results`)

[EWS Watchlist]

Comment on attachment 336770 [details]
Patch

Attachment 336770 [details] did not pass bindings-ews (mac):
Output: http://webkit-queues.webkit.org/results/7136916

New failing tests:
(JS) JSTestClassWithJSBuiltinConstructor.cpp
(JS) JSTestJSBuiltinConstructor.cpp

[Robin Morisset]

Created attachment 336771 [details]
Patch

[Keith Miller]

Comment on attachment 336771 [details]
Patch

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

r- because I think we should limit where the scope where it's "acceptable" to have a null ExecState since that's not true anywhere else.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:105
> +    return Expected<JSC::FunctionExecutable*, JSC::ParserError>(expectedUnlinked.value()->link(vm, vm.builtinExecutables()->codeName##Source(), std::nullopt, s_##codeName##Intrinsic));\\

Nit: I don't think you need the Expected constructor explicitly here.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:117
> +    return Expected<JSC::FunctionExecutable*, JSC::ParserError>(clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Executable().value()->link(vm, clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Source(), std::nullopt, s_##codeName##Intrinsic)); \\

ditto.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:129
> +    return Expected<JSC::FunctionExecutable*, JSC::ParserError>(clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Executable().value()->link(vm, clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Source(), std::nullopt, s_##codeName##Intrinsic)); \\

ditto.

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:179
> +    return Expected<JSC::UnlinkedFunctionExecutable*, JSC::ParserError>(m_##name##Executable.get());\\

ditto.

> Source/JavaScriptCore/builtins/BuiltinExecutables.h:41
> +typedef Expected<UnlinkedFunctionExecutable*, ParserError> ExpectedUnlinkedFunctionExecutable;

Nit: We try to use "using" these days.

> Source/JavaScriptCore/parser/ParserError.cpp:60
> +    VM& vm = exec->vm();

I think this will just crash if exec is nullptr. That sees not intended?

> Source/JavaScriptCore/runtime/Lookup.h:324
> +inline void reifyStaticProperty(VM& vm, const ClassInfo* classInfo, const PropertyName& propertyName, const HashTableValue& value, JSObject& thisObj, ExecState* exec = nullptr)

Nit: We don't usually set up our methods this way. I would move the ExecState to just after vm and change the call site below to pass nullptr.

I would also change the name to execForThrowing or something.

> Source/JavaScriptCore/runtime/Lookup.h:326
> +    ASSERT(&vm == &(exec->vm()));

Won't this crash if exec is nullptr?

> Source/JavaScriptCore/runtime/Lookup.h:335
> +            if (f.has_value())
> +                thisObj.putDirectBuiltinFunction(vm, thisObj.globalObject(), propertyName, f.value(), attributesForStructure(value.attributes()));
> +            else
> +                f.error().throwStackOverflowOrOutOfMemory(exec);

It seems like you should handle nullptr exec here rather than in the throwStackOverflowOrOutOfMemory function.

[Robin Morisset]

Created attachment 336775 [details]
Patch

[Robin Morisset]

Thanks a lot for the review, and for catching the null pointer dereferences.
I have applied all of your suggestions, except for naming the variable execIfAvailable instead of execForThrowing (making it clearer to the caller that passing nullptr is ok).

(In reply to Keith Miller from comment #7)
> Comment on attachment 336771 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=336771&action=review
> 
> r- because I think we should limit where the scope where it's "acceptable"
> to have a null ExecState since that's not true anywhere else.
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:105
> > +    return Expected<JSC::FunctionExecutable*, JSC::ParserError>(expectedUnlinked.value()->link(vm, vm.builtinExecutables()->codeName##Source(), std::nullopt, s_##codeName##Intrinsic));\\
> 
> Nit: I don't think you need the Expected constructor explicitly here.
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:117
> > +    return Expected<JSC::FunctionExecutable*, JSC::ParserError>(clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Executable().value()->link(vm, clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Source(), std::nullopt, s_##codeName##Intrinsic)); \\
> 
> ditto.
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:129
> > +    return Expected<JSC::FunctionExecutable*, JSC::ParserError>(clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Executable().value()->link(vm, clientData->builtinFunctions().${objectNameLC}Builtins().codeName##Source(), std::nullopt, s_##codeName##Intrinsic)); \\
> 
> ditto.
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:179
> > +    return Expected<JSC::UnlinkedFunctionExecutable*, JSC::ParserError>(m_##name##Executable.get());\\
> 
> ditto.
> 
> > Source/JavaScriptCore/builtins/BuiltinExecutables.h:41
> > +typedef Expected<UnlinkedFunctionExecutable*, ParserError> ExpectedUnlinkedFunctionExecutable;
> 
> Nit: We try to use "using" these days.
> 
> > Source/JavaScriptCore/parser/ParserError.cpp:60
> > +    VM& vm = exec->vm();
> 
> I think this will just crash if exec is nullptr. That sees not intended?
> 
> > Source/JavaScriptCore/runtime/Lookup.h:324
> > +inline void reifyStaticProperty(VM& vm, const ClassInfo* classInfo, const PropertyName& propertyName, const HashTableValue& value, JSObject& thisObj, ExecState* exec = nullptr)
> 
> Nit: We don't usually set up our methods this way. I would move the
> ExecState to just after vm and change the call site below to pass nullptr.
> 
> I would also change the name to execForThrowing or something.
> 
> > Source/JavaScriptCore/runtime/Lookup.h:326
> > +    ASSERT(&vm == &(exec->vm()));
> 
> Won't this crash if exec is nullptr?
> 
> > Source/JavaScriptCore/runtime/Lookup.h:335
> > +            if (f.has_value())
> > +                thisObj.putDirectBuiltinFunction(vm, thisObj.globalObject(), propertyName, f.value(), attributesForStructure(value.attributes()));
> > +            else
> > +                f.error().throwStackOverflowOrOutOfMemory(exec);
> 
> It seems like you should handle nullptr exec here rather than in the
> throwStackOverflowOrOutOfMemory function.

[Keith Miller]

Comment on attachment 336775 [details]
Patch

r=me.

[Robin Morisset]

Michael: There appears to be a GCC crash (Internal Compiler Error) on both the gtk-wk2 and on the wpe bot. I do not see how to access the stack trace of the crashing GCC, or anything else required for reporting the bug.
Can you do it or at least point me in the right direction?
Thank you,
Robin

[Michael Catanzaro]

My guess it that the compiler (actually its subprocess, cc1plus) was killed by the OOM killer: that's the usual reason for these errors. What's more worrying is that this happened on both the GTK and WPE bots at the same time. Also surprising is that it occurred on two different files (UnifiedSource20.cpp on the GTK bot, UnifiedSource215.cpp on the WPE bot).

The first thing I would try is uploading the same patch again and see if it passes EWS on a second round, since it might just be bad luck. Let's hope for that. If it fails a second time... well, these issues are the worst to debug. We'll have to try to reproduce it locally. The good news is that attachment #336771 [details] passed EWS, and it's not much different from attachment #336775 [details]....

Carlos, what do you think?

[Robin Morisset]

Created attachment 336848 [details]
Trying again to check if EWS failure is deterministic

[Robin Morisset]

Created attachment 336849 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 336849 [details]
Patch for landing

Clearing flags on attachment: 336849

Committed r230102: <https://trac.webkit.org/changeset/230102>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

(In reply to Robin Morisset from comment #13)
> Created attachment 336848 [details]
> Trying again to check if EWS failure is deterministic

*Whew!*

[Robin Morisset]

(In reply to Michael Catanzaro from comment #17)
> (In reply to Robin Morisset from comment #13)
> > Created attachment 336848 [details]
> > Trying again to check if EWS failure is deterministic
> 
> *Whew!*

Thank you for your advice of trying again. I would not have guessed that the problem could be the OOM killer, and I was quite relieved when the bots turned green the second time.

[Michael Catanzaro]

The clue is here:

c++: internal compiler error: Killed (program cc1plus)
Please submit a full bug report,
with preprocessed source if appropriate.
See <file:///usr/share/doc/gcc-6/README.Bugs> for instructions.

That "Killed" means actual SIGKILL, and there are not many reasons that could ever happen since a crash would normally be SIGSEGV or SIGABRT or something like that... in practice, when building WebKit, experience says Killed means OOM. :)

[Ryan Haddad]

(In reply to WebKit Commit Bot from comment #15)
> Comment on attachment 336849 [details]
> Patch for landing
> 
> Clearing flags on attachment: 336849
> 
> Committed r230102: <https://trac.webkit.org/changeset/230102>
This change caused assertion failures on JSC bots: https://build.webkit.org/builders/Apple%20High%20Sierra%2032-bit%20JSC%20%28BuildAndTest%29/builds/1539

slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit: ERROR: Unchecked JS exception:
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     This scope can throw a JS exception: setUpStaticFunctionSlot @ ./runtime/Lookup.cpp:51
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:         (ExceptionScope::m_recursionDepth was 3)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     But the exception was unchecked as of this scope: getOwnStaticPropertySlot @ ./runtime/JSObject.cpp:2040
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:         (ExceptionScope::m_recursionDepth was 2)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit: 
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit: Unchecked exception detected at:
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     1   0x139b60d JSC::VM::verifyExceptionCheckNeedIsSatisfied(unsigned int, JSC::ExceptionEventLocation&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     2   0x137596b JSC::ThrowScope::~ThrowScope()
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     3   0x1375cd7 JSC::ThrowScope::~ThrowScope()
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     4   0x12000fe JSC::JSObject::getOwnStaticPropertySlot(JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     5   0x382158 JSC::JSObject::getOwnNonIndexPropertySlot(JSC::ExecState*, JSC::Structure*, JSC::PropertyName, JSC::PropertySlot&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     6   0x372cb2 JSC::JSObject::getOwnPropertySlot(JSC::JSObject*, JSC::ExecState*, JSC::PropertyName, JSC::PropertySlot&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     7   0x11a81ca JSC::JSGlobalObject::init(JSC::VM&)::$_36::operator()(JSC::JSObject*, JSC::Identifier const&) const
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     8   0x1199963 JSC::JSGlobalObject::init(JSC::VM&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     9   0x11aadf2 JSC::JSGlobalObject::finishCreation(JSC::VM&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     10  0x3e6b4 GlobalObject::finishCreation(JSC::VM&, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     11  0x3d664 GlobalObject::create(JSC::VM&, JSC::Structure*, WTF::Vector<WTF::String, 0ul, WTF::CrashOnOverflow, 16ul, WTF::FastMalloc> const&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     12  0x22e96 int runJSC<jscmain(int, char**)::$_3>(CommandLine, bool, jscmain(int, char**)::$_3 const&)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     13  0x2169f jscmain(int, char**)
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     14  0x215b7 main
slowMicrobenchmarks.yaml/slowMicrobenchmarks/to-lower-case.js.no-cjit:     15  0xa759f6e1 start

[Ryan Haddad]

Reverted r230102 for reason:

Caused assertion failures on JSC bots.

Committed r230105: <https://trac.webkit.org/changeset/230105>

[Ryan Haddad]

This change also broke builtins-generator-tests:
https://build.webkit.org/builders/Apple%20High%20Sierra%20Release%20WK2%20%28Tests%29/builds/3749

They may simply need to be rebaselined as part of this patch.

[Robin Morisset]

Created attachment 338407 [details]
Patch

[Robin Morisset]

Comment on attachment 338407 [details]
Patch

I fixed the exception handling on the test-case that failed previously, hopefully it won't fail elsewhere.
I also reset the builtin-generator tests.

No significant change to the patch beyond that.

[JF Bastien]

Comment on attachment 338407 [details]
Patch

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

A few comments, otherwise r=me

> Source/JavaScriptCore/ChangeLog:14
> +        There are now bare calls to '.value()' on several paths that may crash. It is not a problem in my opinion, since we previously crashed in every case regardless of the path that took us to createExecutable when encountering a stack overflow.

Can you clarify here that calling foo.value() is critical compared to using *foo because the later is UB if the expected isn't "valued". That distinction has surprised folks before for e.g. std::optional.

> Source/JavaScriptCore/Scripts/builtins/builtins_generate_combined_header.py:70
> +#include <wtf/Expected.h>

🎉

> Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:89
> +    if (!expectedUnlinked.has_value())\\

The patter I prefer is:

if (auto expectedUnlinked = vm.foo.far())
  return expectedUnlinked->link(...);
else
  return makeUnexpected(expectedUnlinked.error()); // or if the E types are compatible, just return expectedUnlinked;

I'd do the here and other places in this patch.

> Source/JavaScriptCore/parser/ParserError.cpp:35
> +JSObject* ParserError::toErrorObject(JSGlobalObject* globalObject, const SourceCode& source, int overrideLineNumber)

Add a space between namespace and function.

> Source/JavaScriptCore/parser/ParserError.cpp:43
> +        auto line = overrideLineNumber == -1 ? m_line : overrideLineNumber;

I'd make overrideLineNumber a std::optional<int> instead, and default to nullopt in the header.

> Source/JavaScriptCore/parser/ParserError.cpp:92
> +        return;

My preference is to have a toString method for the enum, and then have print call that:

const char* toString(ErrorType e)
{
  switch (e) {
  case None: return "none";
  // ...
  }
}

void print(out, e) { return out.print(toString(e)); }

> Source/JavaScriptCore/parser/ParserError.h:85
> +    JS_EXPORT_PRIVATE JSObject* throwStackOverflowOrOutOfMemory(ExecState* = nullptr);

Why default to nullptr? It'll insta-crash if that's called.

> Source/JavaScriptCore/runtime/Lookup.h:416
> +        reifyStaticProperty(vm, nullptr, classInfo, key, value, thisObj);

Looks like reifyStaticProperties is called in CodeGeneratorJS.pm, which has access to exec through globalObject()->globalExec().
Otherwise there's bunch of tests in Source/WebCore/bindings/scripts/test/JS/ which we can probably assume don't throw.
You can probably pipe exec in this function too. Maybe in this patch or in a follow-up?

> JSTests/stress/stack-overflow-while-parsing-builtin.js:1
> +function f() {

You can add the following to control stack size and redone size and make the repro faster / trigger it:

//@ runDefault("--maxPerThreadStackUsage=1000000", "--reservedZoneSize=0")

[Robin Morisset]

Thanks for the review!

(In reply to JF Bastien from comment #25)
> Comment on attachment 338407 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338407&action=review
> 
> A few comments, otherwise r=me
> 
> > Source/JavaScriptCore/ChangeLog:14
> > +        There are now bare calls to '.value()' on several paths that may crash. It is not a problem in my opinion, since we previously crashed in every case regardless of the path that took us to createExecutable when encountering a stack overflow.
> 
> Can you clarify here that calling foo.value() is critical compared to using
> *foo because the later is UB if the expected isn't "valued". That
> distinction has surprised folks before for e.g. std::optional.

Done

> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_generate_combined_header.py:70
> > +#include <wtf/Expected.h>
> 
> 🎉
> 
> > Source/JavaScriptCore/Scripts/builtins/builtins_templates.py:89
> > +    if (!expectedUnlinked.has_value())\\
> 
> The patter I prefer is:
> 
> if (auto expectedUnlinked = vm.foo.far())
>   return expectedUnlinked->link(...);
> else
>   return makeUnexpected(expectedUnlinked.error()); // or if the E types are
> compatible, just return expectedUnlinked;
> 
> I'd do the here and other places in this patch.

That's a very nice pattern. I had not noticed that Expected<> converts to bool. I guess I was too used to strongly typed languages. I've now used it everywhere I reasonably could.
On the other hand, I kept the (apparently redundant?!) use of makeUnexpected when the type changes, even if E is the same, to make things explicit.

> 
> > Source/JavaScriptCore/parser/ParserError.cpp:35
> > +JSObject* ParserError::toErrorObject(JSGlobalObject* globalObject, const SourceCode& source, int overrideLineNumber)
> 
> Add a space between namespace and function.
> 
> > Source/JavaScriptCore/parser/ParserError.cpp:43
> > +        auto line = overrideLineNumber == -1 ? m_line : overrideLineNumber;
> 
> I'd make overrideLineNumber a std::optional<int> instead, and default to
> nullopt in the header.

I agree that it would be cleaner, but I would rather do it in a separate patch, this one is already pretty big (this use of -1 is from before my patch and the -1 appears to be propagated through quite a few places). Would you agree if I just open a new bug for that?

> 
> > Source/JavaScriptCore/parser/ParserError.cpp:92
> > +        return;
> 
> My preference is to have a toString method for the enum, and then have print
> call that:
> 
> const char* toString(ErrorType e)
> {
>   switch (e) {
>   case None: return "none";
>   // ...
>   }
> }
> 
> void print(out, e) { return out.print(toString(e)); }

Done

> 
> > Source/JavaScriptCore/parser/ParserError.h:85
> > +    JS_EXPORT_PRIVATE JSObject* throwStackOverflowOrOutOfMemory(ExecState* = nullptr);
> 
> Why default to nullptr? It'll insta-crash if that's called.

Good catch, this was a remnant from a previous version of this patch.

> 
> > Source/JavaScriptCore/runtime/Lookup.h:416
> > +        reifyStaticProperty(vm, nullptr, classInfo, key, value, thisObj);
> 
> Looks like reifyStaticProperties is called in CodeGeneratorJS.pm, which has
> access to exec through globalObject()->globalExec().
> Otherwise there's bunch of tests in Source/WebCore/bindings/scripts/test/JS/
> which we can probably assume don't throw.
> You can probably pipe exec in this function too. Maybe in this patch or in a
> follow-up?

In a follow-up please.

> 
> > JSTests/stress/stack-overflow-while-parsing-builtin.js:1
> > +function f() {
> 
> You can add the following to control stack size and redone size and make the
> repro faster / trigger it:
> 
> //@ runDefault("--maxPerThreadStackUsage=1000000", "--reservedZoneSize=0")

Done


I've also merged the patch for https://bugs.webkit.org/show_bug.cgi?id=184744, since it  is very closely linked, and cleans up some of this patch.

[Robin Morisset]

Created attachment 338584 [details]
Patch

[JF Bastien]

Comment on attachment 338584 [details]
Patch

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

If you want to fix https://bugs.webkit.org/show_bug.cgi?id=184744 you should also add the test from it. There's one comment that I think you need to address though, otherwise the fix is incorrect.

> Source/JavaScriptCore/Scripts/tests/builtins/expected/JavaScriptCore-Builtin.prototype-Combined.js-result:290
> +        return expectedUnlinked.value()->link(vm, vm.builtinExecutables()->codeName##Source(), std::nullopt, s_##codeName##Intrinsic);\

FWIW you  don't need ".value()" here: expected overload operator-> and forwards to the value type already. It's UB to use operator-> if it contains an error, but you've already checked above.

I'm not saying you should change it, it's just the style I prefer but either approach is fine with me.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:133
> +        if (!f.has_value())\

Yo can do the same pattern here.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3329
> +            emitThrowRangeError("Stack overflow in the parser");

I don't think this is correct because you'll emit an exception. It'll correctly throw when we first try to execute, but then it goes into the code cache and will always throw, even if the stack wouldn't overflow anymore.

[Filip Pizlo]

(In reply to JF Bastien from comment #28)
> Comment on attachment 338584 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=338584&action=review
> 
> If you want to fix https://bugs.webkit.org/show_bug.cgi?id=184744 you should
> also add the test from it. There's one comment that I think you need to
> address though, otherwise the fix is incorrect.
> 
> > Source/JavaScriptCore/Scripts/tests/builtins/expected/JavaScriptCore-Builtin.prototype-Combined.js-result:290
> > +        return expectedUnlinked.value()->link(vm, vm.builtinExecutables()->codeName##Source(), std::nullopt, s_##codeName##Intrinsic);\
> 
> FWIW you  don't need ".value()" here: expected overload operator-> and
> forwards to the value type already. It's UB to use operator-> if it contains
> an error, but you've already checked above.
> 
> I'm not saying you should change it, it's just the style I prefer but either
> approach is fine with me.
> 
> > Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:133
> > +        if (!f.has_value())\
> 
> Yo can do the same pattern here.
> 
> > Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp:3329
> > +            emitThrowRangeError("Stack overflow in the parser");
> 
> I don't think this is correct because you'll emit an exception. It'll
> correctly throw when we first try to execute, but then it goes into the code
> cache and will always throw, even if the stack wouldn't overflow anymore.

I think Robin's approach totally makes sense.  It's easy to do and it's better than crashing.

[JF Bastien]

> > I don't think this is correct because you'll emit an exception. It'll
> > correctly throw when we first try to execute, but then it goes into the code
> > cache and will always throw, even if the stack wouldn't overflow anymore.
> 
> I think Robin's approach totally makes sense.  It's easy to do and it's
> better than crashing.

As coded, the code cache will contain a function which always throws, even if there is enough stack space. The approach seems fine to me, as long as the entry doesn't stay in the cache.

[Saam Barati]

(In reply to JF Bastien from comment #30)
> > > I don't think this is correct because you'll emit an exception. It'll
> > > correctly throw when we first try to execute, but then it goes into the code
> > > cache and will always throw, even if the stack wouldn't overflow anymore.
> > 
> > I think Robin's approach totally makes sense.  It's easy to do and it's
> > better than crashing.
> 
> As coded, the code cache will contain a function which always throws, even
> if there is enough stack space. The approach seems fine to me, as long as
> the entry doesn't stay in the cache.

I don't think we should ever emit code for a stack overflow when parsing is what caused the stack overflow. I agree with JF here, this feels really nasty to do.

[Robin Morisset]

Created attachment 339406 [details]
Patch without the controversial part

[Robin Morisset]

(In reply to Robin Morisset from comment #32)
> Created attachment 339406 [details]
> Patch without the controversial part

I moved the controversial part out of this patch, since it was to fix the different (but related) https://bugs.webkit.org/show_bug.cgi?id=184744, and it is not entirely clear how to best fix it.

No other change to the rest of the patch. May I land this part since it has already been reviewed and there was no remaining issue? I am a bit afraid of the merge conflicts I will get if a patch that touches that many files remains in limbo for long.

[Robin Morisset]

Comment on attachment 339406 [details]
Patch without the controversial part

After a discussion with Saam, there might be a much simpler solution.

[Saam Barati]

Created attachment 343216 [details]
WIP

a WIP for writing the mini builtin offsets parser. This code is pretty terrible. I'm not sure if we should do it. But it probably works, at least for now.

[Saam Barati]

Will rebase the WIP. Still not sure if we want this approach for sure, but I'll get it into landing shape just in case.

[Saam Barati]

Created attachment 343842 [details]
WIP

Try EWS out

[EWS Watchlist]

Attachment 343842 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/parser/Nodes.cpp:283:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:284:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:284:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:285:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:285:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:286:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:286:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:287:  Extra space before )  [whitespace/parens] [2]
ERROR: Source/JavaScriptCore/parser/Nodes.cpp:292:  Missing space after ,  [whitespace/comma] [3]
ERROR: Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:258:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 10 in 14 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 343842 [details]
WIP

Attachment 343842 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/8373449

New failing tests:
microbenchmarks/try-get-by-id-polymorphic.js.ftl-no-cjit-no-put-stack-validate
microbenchmarks/try-get-by-id-polymorphic.js.ftl-no-cjit-validate-sampling-profiler
microbenchmarks/try-get-by-id-polymorphic.js.ftl-eager-no-cjit-b3o1
microbenchmarks/try-get-by-id-polymorphic.js.no-ftl
microbenchmarks/try-get-by-id-polymorphic.js.dfg-eager-no-cjit-validate
microbenchmarks/try-get-by-id-polymorphic.js.dfg-maximal-flush-validate-no-cjit
microbenchmarks/try-get-by-id-polymorphic.js.dfg-eager
microbenchmarks/try-get-by-id-polymorphic.js.default
microbenchmarks/try-get-by-id-polymorphic.js.ftl-no-cjit-no-inline-validate
microbenchmarks/try-get-by-id-polymorphic.js.ftl-no-cjit-small-pool
microbenchmarks/try-get-by-id-polymorphic.js.ftl-no-cjit-b3o1
microbenchmarks/try-get-by-id-polymorphic.js.ftl-eager
microbenchmarks/try-get-by-id-polymorphic.js.ftl-eager-no-cjit
microbenchmarks/try-get-by-id-polymorphic.js.no-cjit-collect-continuously
microbenchmarks/try-get-by-id-polymorphic.js.no-cjit-validate-phases
microbenchmarks/try-get-by-id-polymorphic.js.no-llint
apiTests

[Saam Barati]

Created attachment 343871 [details]
patch

[EWS Watchlist]

Comment on attachment 343871 [details]
patch

Attachment 343871 [details] did not pass jsc-ews (mac):
Output: https://webkit-queues.webkit.org/results/8376067

New failing tests:
stress/dont-crash-on-stack-overflow-when-parsing-builtin.js.default
stress/dont-crash-on-stack-overflow-when-parsing-default-constructor.js.default
apiTests

[Mark Lam]

Comment on attachment 343871 [details]
patch

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

r=me with fixes.  Please also investigate why the jsc EWS is unhappy.  Your newly added tests should not be failing.  Please fix.

> JSTests/stress/dont-crash-on-stack-overflow-when-parsing-builtin.js:1
> +//@ runDefault("--maxPerThreadStackUsage=10000", "--reservedZoneSize=0")

Did you really intend for this test to only run with the default configuration?  If not, you can use requireOptions to specify the required options and run this test with all test configurations:
//@ requireOptions("--maxPerThreadStackUsage=10000", " --reservedZoneSize=0")

> JSTests/stress/dont-crash-on-stack-overflow-when-parsing-default-constructor.js:1
> +//@ runDefault("--maxPerThreadStackUsage=10000", "--reservedZoneSize=0")

Ditto.

> Source/JavaScriptCore/ChangeLog:17
> +        sync with the full parser.) The result of this is that BuiltinExecutbles::createExecutable
> +        always succeeds.

Yay!  Can't argue with success.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:94
> +    RELEASE_ASSERT(view.length() >= 14); // strlen("(function(){})") == 14

Shouldn't this be 15 because you're requiring a space between "function" and "("?

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:143
> +            if (i + 2 < view.length() && characters[i] == '.' && characters[i + 1] == '.' && characters[i + 2] == '.')
> +                hasRestParam = true;

If you've processed the rest param already, you should add 2 to i because you're consuming 2 additional chars in addition to the one counted by ++i below.

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:177
> +        if (!isInStrictContext && (characters[i] == '"' || characters[i] == '\'')) {
> +            if (i + 1 + strlen("use strict") <= view.length()) {
> +                if (!memcmp(characters + i + 1, "use strict", strlen("use strict")))
> +                    isInStrictContext = true;
> +            }
> +        }

You should also require condition that characters[i + strlen("use strict") + 1] == characters[i] for the closing quote.

If you've successfully matched "use strict", you should also add 1 + strlen("use strict") to i so that we can skip ahead.

Technically, don't we also require a ';' after "use strict"?  Because "use strict" + " with other stuff" doesn't count as a strict mode declaration, no?  If a semi-colon is also required, let's add it to the verification and the count skipping above.

I would also suggest precaching strlen("use strict") into a variable (even if the compiler is smart enough to treat it as an intrinsic and replace it with a constant).

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:196
> +    unsigned positionBeforeLastNewlineLineStartOffset = 0; // This is the second to last newline + 1.
> +    for (int i = offsetOfLastNewline - 1; true; ) {
> +        if (i < 0)
> +            break;
> +        if (characters[i] == '\n') {
> +            positionBeforeLastNewlineLineStartOffset = i + 1;
> +            break;
> +        }
> +        --i;
> +    }

Why not just add track offsetOfSecondToLastNewLine in the first for loop above instead of iterating the characters again?

    unsigned offsetOfLastNewline = UINT_MAX;
    unsigned offsetOfSecondToLastNewline = UINT_MAX;
    for (unsigned i = 0; i < view.length(); ++i) {
            ...
            offsetOfSecondToLastNewline = offsetOfLastNewline;
            offsetOfLastNewline = i;

And just RELEASE_ASSERT that offsetOfLastNewline and offsetOfSecondToLastNewline != UINT_MAX after the loop.  I think it's safe to assume that our builtins won't be in a single line).  If not (i.e. if we minify our builtin code and remove newlines in the future), then we need a handle the UINT_MAX offsets in some way as well.

With that, positionBeforeLastNewlineLineStartOffset = offsetOfSecondToLastNewline + 1;

> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:216
> +    if (!ASSERT_DISABLED || Options::validateBytecode()) {

Don't you want this to be a && condition?  You're trying to not include this test code in a release build, right?

> Source/JavaScriptCore/parser/Nodes.cpp:234
> +        : Node(endLocation)
> +        , m_startColumn(startColumn)
> +        , m_endColumn(endColumn)
> +        , m_functionKeywordStart(functionKeywordStart)
> +        , m_functionNameStart(functionNameStart)
> +        , m_parametersStart(parametersStart)
> +        , m_startStartOffset(startLocation.startOffset)
> +        , m_parameterCount(parameterCount)
> +        , m_parseMode(mode)
> +        , m_isInStrictContext(isInStrictContext)
> +        , m_superBinding(static_cast<unsigned>(superBinding))
> +        , m_constructorKind(static_cast<unsigned>(constructorKind))
> +        , m_isArrowFunctionBodyExpression(isArrowFunctionBodyExpression)

I think WebKit style is to have these indented only by 4 spaces.

[Saam Barati]

Comment on attachment 343871 [details]
patch

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

>> JSTests/stress/dont-crash-on-stack-overflow-when-parsing-builtin.js:1
>> +//@ runDefault("--maxPerThreadStackUsage=10000", "--reservedZoneSize=0")
> 
> Did you really intend for this test to only run with the default configuration?  If not, you can use requireOptions to specify the required options and run this test with all test configurations:
> //@ requireOptions("--maxPerThreadStackUsage=10000", " --reservedZoneSize=0")

We can do this I suppose. I need to figure out why this fails on the bot but not locally for me

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:94
>> +    RELEASE_ASSERT(view.length() >= 14); // strlen("(function(){})") == 14
> 
> Shouldn't this be 15 because you're requiring a space between "function" and "("?

I just did 14 as bare minimum. I could make it 15 too, it doesn't matter

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:143
>> +                hasRestParam = true;
> 
> If you've processed the rest param already, you should add 2 to i because you're consuming 2 additional chars in addition to the one counted by ++i below.

ok

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:177
>> +        }
> 
> You should also require condition that characters[i + strlen("use strict") + 1] == characters[i] for the closing quote.
> 
> If you've successfully matched "use strict", you should also add 1 + strlen("use strict") to i so that we can skip ahead.
> 
> Technically, don't we also require a ';' after "use strict"?  Because "use strict" + " with other stuff" doesn't count as a strict mode declaration, no?  If a semi-colon is also required, let's add it to the verification and the count skipping above.
> 
> I would also suggest precaching strlen("use strict") into a variable (even if the compiler is smart enough to treat it as an intrinsic and replace it with a constant).

We don't require a semi colon

>> Source/JavaScriptCore/builtins/BuiltinExecutables.cpp:216
>> +    if (!ASSERT_DISABLED || Options::validateBytecode()) {
> 
> Don't you want this to be a && condition?  You're trying to not include this test code in a release build, right?

No, I want "or" here. I want this to run in our release tests

[Saam Barati]

Comment on attachment 343871 [details]
patch

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

>> Source/JavaScriptCore/parser/Nodes.cpp:234
>> +        , m_isArrowFunctionBodyExpression(isArrowFunctionBodyExpression)
> 
> I think WebKit style is to have these indented only by 4 spaces.

I agree, but I'm going to just keep this consistent with the above constructor

[Saam Barati]

Created attachment 343926 [details]
patch for landing

[Saam Barati]

Created attachment 343927 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 343927 [details]
patch for landing

Clearing flags on attachment: 343927

Committed r233377: <https://trac.webkit.org/changeset/233377>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

This change broke the Windows build:
https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/10393

[Saam Barati]

(In reply to Ryan Haddad from comment #49)
> This change broke the Windows build:
> https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> 10393

looking

[Saam Barati]

(In reply to Saam Barati from comment #50)
> (In reply to Ryan Haddad from comment #49)
> > This change broke the Windows build:
> > https://build.webkit.org/builders/Apple%20Win%20Release%20%28Build%29/builds/
> > 10393
> 
> looking

landed attempted fix:
https://trac.webkit.org/changeset/233383/webkit