Bug 151324

Summary: REGRESSION(r192436): Inspector Hanging under tryParseDestructuringPatternExpression
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: JavaScriptCoreAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: bburg, buildbot, caitp, commit-queue, ggaren, joepeck, keith_miller, mark.lam, msaboff, rniwa, saam, ysuzuki
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 151026    
Bug Blocks:    
Attachments:
Description Flags
[TEST] Test Case
none
Speed up AssignmentPattern parsing
none
Patch
none
Archive of layout-test-results from ews104 for mac-mavericks-wk2
none
Patch
none
Archive of layout-test-results from ews100 for mac-mavericks
none
Patch
none
Patch
mcatanzaro: review-
Proposal for further reducing re-parsing none

Description Joseph Pecoraro 2015-11-16 14:27:19 PST
* 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
> 
> ...
Comment 1 Joseph Pecoraro 2015-11-16 15:29:13 PST
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.
Comment 2 Joseph Pecoraro 2015-11-16 16:44:43 PST
We may want to roll out until the performance issue is addressed. I didn't see anything obviously wrong in the original change.
Comment 3 Caitlin Potter (:caitp) 2015-11-17 09:10:48 PST
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 "=".
Comment 4 Caitlin Potter (:caitp) 2015-11-17 10:19:38 PST
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.
Comment 5 BJ Burg 2015-11-17 10:25:09 PST
(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.
Comment 6 Caitlin Potter (:caitp) 2015-11-17 12:06:49 PST
Created attachment 265692 [details]
Patch
Comment 7 Joseph Pecoraro 2015-11-17 12:20:28 PST
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.
Comment 8 Build Bot 2015-11-17 12:38:06 PST
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
Comment 9 Build Bot 2015-11-17 12:38:09 PST
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
Comment 10 Caitlin Potter (:caitp) 2015-11-17 12:38:50 PST
Created attachment 265699 [details]
Patch

Add a regression test and fix ChangeLog
Comment 11 Joseph Pecoraro 2015-11-17 13:05:47 PST
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.
Comment 12 Build Bot 2015-11-17 13:09:47 PST
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
Comment 13 Build Bot 2015-11-17 13:09:50 PST
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
Comment 14 Caitlin Potter (:caitp) 2015-11-17 13:21:39 PST
Created attachment 265703 [details]
Patch

Hack to fix the inspector test failure
Comment 15 Geoffrey Garen 2015-11-17 13:54:27 PST
Comment on attachment 265703 [details]
Patch

r=me
Comment 16 Geoffrey Garen 2015-11-17 13:58:09 PST
Comment on attachment 265703 [details]
Patch

Oops! Still failing parse.html.
Comment 17 Caitlin Potter (:caitp) 2015-11-17 17:30:45 PST
Created attachment 265722 [details]
Patch

Really fix the failing inspector test
Comment 18 Caitlin Potter (:caitp) 2015-11-18 11:36:01 PST
Created attachment 265765 [details]
Proposal for further reducing re-parsing

For consideration, this followup provides a way to reduce re-parsing even more
Comment 19 Geoffrey Garen 2015-11-18 15:36:57 PST
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.
Comment 20 Geoffrey Garen 2015-11-18 15:39:09 PST
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.
Comment 21 Michael Catanzaro 2016-01-02 10:45:33 PST
Comment on attachment 265722 [details]
Patch

Clearing this from request queue. Hey Caitlin, got time to follow up with Geoff's feedback?
Comment 22 Caitlin Potter (:caitp) 2016-01-03 06:15:04 PST
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
Comment 23 Yusuke Suzuki 2016-01-03 09:21:23 PST
(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 ***
Comment 24 BJ Burg 2016-01-03 15:17:38 PST
(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.