[webkit-reviews] review granted: [Bug 184074] We shouldn't recurse into the parser when gathering metadata about various function offsets : [Attachment 343871] patch

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has granted Saam Barati <sbarati at apple.com>'s
request for review:
Bug 184074: We shouldn't recurse into the parser when gathering metadata about
various function offsets
https://bugs.webkit.org/show_bug.cgi?id=184074

Attachment 343871: patch

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




--- Comment #42 from Mark Lam <mark.lam at apple.com> ---
Comment on attachment 343871
  --> https://bugs.webkit.org/attachment.cgi?id=343871
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.