Bug 151324 – REGRESSION(r192436): Inspector Hanging under tryParseDestructuringPatternExpression

• Component: JavaScriptCore
• Status: RESOLVED DUPLICATE

[Joseph Pecoraro]

* SUMMARY
Inspector Hanging under tryParseDestructuringPatternExpression.

* STEPS TO REPRODUCE
1. Inspect <http://bogojoker.com/shell/>
2. Show Timeline Tab
3. Reload
  => Timeline starts a bit, then hangs completely

* NOTES
- Sample shows stuck under tryParseDestructuringPatternExpression:
> 8033 void IPC::handleMessage<Messages::WebInspectorUI::SendMessageToFrontend, WebKit::WebInspectorUI, void (WebKit::WebInspectorUI::*)(WTF::String const&)>(IPC::MessageDecoder&, WebKit::WebInspectorUI*, void (WebKit::WebInspectorUI::*)(WTF::String const&))  (in WebKit) + 67  [0x10e37e1f7]  HandleMessage.h:16
>   8033 WebKit::WebInspectorFrontendAPIDispatcher::dispatchMessageAsync(WTF::String const&)  (in WebKit) + 72  [0x10e3761c0]  WebInspectorFrontendAPIDispatcher.cpp:76
>     8033 WebKit::WebInspectorFrontendAPIDispatcher::evaluateExpressionOnLoad(WTF::String const&)  (in WebKit) + 59  [0x10e375fd1]  WebInspectorFrontendAPIDispatcher.cpp:83
>       8033 WebCore::ScriptController::executeScript(WTF::String const&, bool, WebCore::ExceptionDetails*)  (in WebCore) + 343  [0x110a049a7]  ScriptController.cpp:180
>         8033 WebCore::ScriptController::evaluateInWorld(WebCore::ScriptSourceCode const&, WebCore::DOMWrapperWorld&, WebCore::ExceptionDetails*)  (in WebCore) + 304  [0x110a02ca0]  JSMainThreadExecState.h:62
>           8033 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)  (in JavaScriptCore) + 469  [0x10f11ca75]  Completion.cpp:104
>             8033 JSC::Interpreter::execute(JSC::ProgramExecutable*, JSC::ExecState*, JSC::JSObject*)  (in JavaScriptCore) + 10276  [0x10f441114]  Interpreter.cpp:938
>               8033 JSC::ProgramExecutable::initializeGlobalProperties(JSC::VM&, JSC::ExecState*, JSC::JSScope*)  (in JavaScriptCore) + 106  [0x10f32f47a]  Executable.cpp:574
>                 8033 JSC::JSGlobalObject::createProgramCodeBlock(JSC::ExecState*, JSC::ProgramExecutable*, JSC::JSObject**)  (in JavaScriptCore) + 205  [0x10f4dafbd]  JSGlobalObject.cpp:935
>                   8033 JSC::CodeCache::getProgramCodeBlock(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&)  (in JavaScriptCore) + 82  [0x10f0f6a52]  CodeCache.cpp:135
>                     8033 JSC::UnlinkedProgramCodeBlock* JSC::CodeCache::getGlobalCodeBlock<JSC::UnlinkedProgramCodeBlock, JSC::ProgramExecutable>(JSC::VM&, JSC::ProgramExecutable*, JSC::SourceCode const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::ThisTDZMode, JSC::DebuggerMode, JSC::ProfilerMode, JSC::ParserError&, JSC::VariableEnvironment const*)  (in JavaScriptCore) + 225  [0x10f0f71d1]  CodeCache.cpp:104
>                       8033 std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::parse<JSC::ProgramNode>(JSC::VM*, JSC::SourceCode const&, JSC::Identifier const&, JSC::JSParserBuiltinMode, JSC::JSParserStrictMode, JSC::SourceParseMode, JSC::ParserError&, JSC::JSTextPosition*, JSC::ConstructorKind, JSC::ThisTDZMode)  (in JavaScriptCore) + 306  [0x10f0a7ae2]  Parser.h:1428
>                         8033 std::__1::unique_ptr<JSC::ProgramNode, std::__1::default_delete<JSC::ProgramNode> > JSC::Parser<JSC::Lexer<unsigned char> >::parse<JSC::ProgramNode>(JSC::ParserError&, JSC::Identifier const&, JSC::SourceParseMode)  (in JavaScriptCore) + 96  [0x10f0a7d40]  Parser.h:1342
>                           8033 JSC::Parser<JSC::Lexer<unsigned char> >::parseInner(JSC::Identifier const&, JSC::SourceParseMode) Parser.cpp:275
>                             8033 JSC::ASTBuilder::SourceElements JSC::Parser<JSC::Lexer<unsigned char> >::parseSourceElements<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::SourceElementsMode) Parser.cpp:377
>                               8033 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatementListItem<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) Parser.cpp:499
>                                 8033 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseStatement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::Identifier const*&, unsigned int*) Parser.cpp:1532
>                                   8033 JSC::ASTBuilder::Statement JSC::Parser<JSC::Lexer<unsigned char> >::parseExpressionOrLabelStatement<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2150
>                                     8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2691
>                                       8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700
>                                         8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3486
>                                           8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700
>                                             8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128
>                                               8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948
>                                                 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2727
>                                                   8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::tryParseDestructuringPatternExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::AssignmentContext) Parser.cpp:771
>                                                     8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDestructuringPattern<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:913
>                                                       8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentElement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:793
>                                                         8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128
>                                                           8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948
>                                                             8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700
>                                                               8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3255
>                                                                 8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700
>                                                                   8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128
>                                                                     8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948
>                                                                       8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2727
>                                                                         8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::tryParseDestructuringPatternExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::AssignmentContext) Parser.cpp:771
>                                                                           8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDestructuringPattern<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:845
>                                                                             8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentElement<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:793
>                                                                               8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128
>                                                                                 8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948
>                                                                                   8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3700
>                                                                                     8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseMemberExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:3128
>                                                                                       8033 JSC::ASTBuilder::Property JSC::Parser<JSC::Lexer<unsigned char> >::parseProperty<JSC::ASTBuilder>(JSC::ASTBuilder&, bool) Parser.cpp:2948
>                                                                                         8033 JSC::ASTBuilder::Expression JSC::Parser<JSC::Lexer<unsigned char> >::parseAssignmentExpression<JSC::ASTBuilder>(JSC::ASTBuilder&) Parser.cpp:2727
>                                                                                           8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::tryParseDestructuringPatternExpression<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::AssignmentContext) Parser.cpp:771
>                                                                                             8033 JSC::ASTBuilder::DestructuringPattern JSC::Parser<JSC::Lexer<unsigned char> >::parseDestructuringPattern<JSC::ASTBuilder>(JSC::ASTBuilder&, JSC::DestructuringKind, JSC::Parser<JSC::Lexer<unsigned char> >::ExportType, JSC::Identifier const**, bool*, JSC::AssignmentContext, int) Parser.cpp:913
> 
> ...

[Joseph Pecoraro]

Created attachment 265629 [details]
[TEST] Test Case

Attached a test case. This test used to take 8ms to load on my machine, it now takes 13seconds.

The test case just does:

    <script>
    Does.not.exist({...huge object literal...})
    </script>

So it is valid syntax, but immediate runtime error. Tons of time is spent parsing the literal.

[Joseph Pecoraro]

We may want to roll out until the performance issue is addressed. I didn't see anything obviously wrong in the original change.

[Caitlin Potter (:caitp)]

The cause is pretty straight forward, when parsing the destructuring pattern, there's a SavePoint for each property in the object literal, when parsed in an expression context, and this happens for every ObjectLiteral/ArrayLiteral parsed in an AssignmentExpression context.

A fix that may work is re-organizing it to parse as an ObjectLiteral/ArrayLiteral first, and switch to destructuring mode only if followed by a "=".

[Caitlin Potter (:caitp)]

Created attachment 265679 [details]
Speed up AssignmentPattern parsing

I haven't run the full test suite, this isn't ready yet, but it does produce a significant speedup for loading this page. Most real-world destructuring patterns are probably not that big, so while those would still be slow, they shouldn't be as noticeable.

I'll add further fixes to the test suite as needed.

PS, webkit-patch is failing to upload my patches today (https://gist.github.com/caitp/27af5c48cd1758c65784 for a log), so this is just the output from `git format-patch`, hope that's cool.

[Blaze Burg]

(In reply to comment #4)
> Created attachment 265679 [details]
> Speed up AssignmentPattern parsing
> 
> I haven't run the full test suite, this isn't ready yet, but it does produce
> a significant speedup for loading this page. Most real-world destructuring
> patterns are probably not that big, so while those would still be slow, they
> shouldn't be as noticeable.

This seems like a good choice. You should add a similar test case to Joe's as a regression test. 

> PS, webkit-patch is failing to upload my patches today
> (https://gist.github.com/caitp/27af5c48cd1758c65784 for a log), so this is
> just the output from `git format-patch`, hope that's cool.

We've been having server issues. As long as the patch applies, it doesn't matter how it's generated. Without webkit-patch, you will need to manually set flags for r?, cq?, and submitting it to EWS by clicking the button next to the attachment.

[Caitlin Potter (:caitp)]

Created attachment 265692 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 265692 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:3
> +Speed up Destructuring Assignment parsing, by only performing it if parsing

Seems this ChangeLog formatting broke. Format like the other ChangeLogs below (bug title + reviewer at top, description beneath it, and indented).

Also, can we get a test for this.

[Build Bot]

Comment on attachment 265692 [details]
Patch

Attachment 265692 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/442423

New failing tests:
inspector/runtime/parse.html

[Build Bot]

Created attachment 265698 [details]
Archive of layout-test-results from ews104 for mac-mavericks-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-mavericks-wk2  Platform: Mac OS X 10.9.5

[Caitlin Potter (:caitp)]

Created attachment 265699 [details]
Patch

Add a regression test and fix ChangeLog

[Joseph Pecoraro]

These seem like legit minor issues with the patch caught by inspector/runtime/parse.html:

> -PASS: Should be SyntaxErrorType Recoverable.
> +FAIL: Should be SyntaxErrorType Recoverable.
>  Source: var x = {prop:123,
>                           ^

> -PASS: Should be SyntaxErrorType Recoverable.
> +FAIL: Should be SyntaxErrorType Recoverable.
>  Source: var x = [1,
>                    ^

We expect the parser to be reporting that these are Recoverable syntax errors.

[Build Bot]

Comment on attachment 265699 [details]
Patch

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

New failing tests:
inspector/runtime/parse.html

[Build Bot]

Created attachment 265701 [details]
Archive of layout-test-results from ews100 for mac-mavericks

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mavericks  Platform: Mac OS X 10.9.5

[Caitlin Potter (:caitp)]

Created attachment 265703 [details]
Patch

Hack to fix the inspector test failure

[Geoffrey Garen]

Comment on attachment 265703 [details]
Patch

r=me

[Geoffrey Garen]

Comment on attachment 265703 [details]
Patch

Oops! Still failing parse.html.

[Caitlin Potter (:caitp)]

Created attachment 265722 [details]
Patch

Really fix the failing inspector test

[Caitlin Potter (:caitp)]

Created attachment 265765 [details]
Proposal for further reducing re-parsing

For consideration, this followup provides a way to reduce re-parsing even more

[Geoffrey Garen]

Comment on attachment 265722 [details]
Patch

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

> Source/JavaScriptCore/tests/stress/regress-151324.js:1698
> +})`, `new ReferenceError("Can't find variable: InspectorFrontendAPI")`);

I think you need a newline at the end of this test to make diffing tools happy.

[Geoffrey Garen]

Comment on attachment 265765 [details]
Proposal for further reducing re-parsing

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

This seems like a more efficient approach. Can you merge it into your previous patch and rebase to trunk?

> Source/JavaScriptCore/parser/Parser.cpp:2727
> +    ExpressionErrorClassifier classification(this);

Let's call this classifier to match its class name.

> Source/JavaScriptCore/parser/Parser.h:740
> +    enum ErrorClass {

Let's call this ExpressionErrorClass.

> Source/JavaScriptCore/parser/Parser.h:748
> +            , m_outerClassifier(parser->m_errorClassifier)

Let's call this m_previous.

> Source/JavaScriptCore/parser/Parser.h:759
> +        void classifyError(ErrorClass classification)

Let's call this classifyExpressionError.

[Michael Catanzaro]

Comment on attachment 265722 [details]
Patch

Clearing this from request queue. Hey Caitlin, got time to follow up with Geoff's feedback?

[Caitlin Potter (:caitp)]

This was landed in a separate bug (https://bugs.webkit.org/show_bug.cgi?id=151026)

I don't have editbugs on webkit, but feel free to mark this one as resolved

[Yusuke Suzuki]

(In reply to comment #22)
> This was landed in a separate bug
> (https://bugs.webkit.org/show_bug.cgi?id=151026)
> 
> I don't have editbugs on webkit, but feel free to mark this one as resolved

Nice :)

*** This bug has been marked as a duplicate of bug 151026 ***

[Blaze Burg]

(In reply to comment #22)
> This was landed in a separate bug
> (https://bugs.webkit.org/show_bug.cgi?id=151026)
> 
> I don't have editbugs on webkit, but feel free to mark this one as resolved

Hi caitp, we should be able to get this bit set for you tomorrow.