Bug 206972

Summary: Parser needs to restore unary stack state when backtracking
Product: WebKit Reporter: Keith Miller <keith_miller>
Component: New BugsAssignee: Keith Miller <keith_miller>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews-watchlist, ljharb, mark.lam, msaboff, ross.kirsling, saam, tzagallo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch for landing
none
Patch for landing
none
Patch for landing none

Description Keith Miller 2020-01-29 15:37:52 PST 
Parser needs to restore unary stack state when backtracking
Comment 1 Keith Miller 2020-01-29 16:36:10 PST 
Created attachment 389200 [details]
Patch
Comment 2 Saam Barati 2020-01-29 16:50:20 PST 
Comment on attachment 389200 [details]
Patch

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

nice

Maybe add some more tests. like test both strict and non strict. And maybe some of the other tests we were dealing with yesterday. Might also be good to try different unary ops, etc.

> Source/JavaScriptCore/ChangeLog:8
> +        Previously we would try to parse possibly stale stack entries

"stack entries" => "unary operator. stack entries"

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:177
> +    static constexpr unsigned thisLength = sizeof("this") - 1; // don't count \0

why?

why not strlen?

> Source/JavaScriptCore/parser/SyntaxChecker.h:340
> +    void assignmentStackAppend(int& hasAssignements, int, int, int, int, Operator) { hasAssignements = 1; }

hasAssignments => assignmentStackDepth

> Source/JavaScriptCore/parser/SyntaxChecker.h:341
> +    int createAssignment(const JSTokenLocation&, int& hasAssignements, int, int, int, int) { hasAssignements = 0; return AssignmentExpr; }

hasAssignments => assignmentStackDepth
Comment 3 Saam Barati 2020-01-29 16:51:23 PST 
Comment on attachment 389200 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:11
> +        reparsing. Additionally, this patch fixes an issue where the

"reparsing" => "reparsing after backtracking"
Comment 4 Keith Miller 2020-01-29 19:43:08 PST 
Comment on attachment 389200 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:8
>> +        Previously we would try to parse possibly stale stack entries
> 
> "stack entries" => "unary operator. stack entries"

Fixed, I'm assuming the period is a typo?

>> Source/JavaScriptCore/ChangeLog:11
>> +        reparsing. Additionally, this patch fixes an issue where the
> 
> "reparsing" => "reparsing after backtracking"

Fixed.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:177
>> +    static constexpr unsigned thisLength = sizeof("this") - 1; // don't count \0
> 
> why?
> 
> why not strlen?

This way it can be constexpr thus it has more big brain-ness. :P

Fixed.

>> Source/JavaScriptCore/parser/SyntaxChecker.h:340
>> +    void assignmentStackAppend(int& hasAssignements, int, int, int, int, Operator) { hasAssignements = 1; }
> 
> hasAssignments => assignmentStackDepth

That seems inaccurate since it's not incremented. I think I'm going to leave it as is.

>> Source/JavaScriptCore/parser/SyntaxChecker.h:341
>> +    int createAssignment(const JSTokenLocation&, int& hasAssignements, int, int, int, int) { hasAssignements = 0; return AssignmentExpr; }
> 
> hasAssignments => assignmentStackDepth

Ditto.
Comment 5 Keith Miller 2020-01-29 19:45:16 PST 
The function-toString-parentheses.js failures are just incorrect. The crash in the bytecode-cache test is because it asserts there's a codeBlock but since it exception'd the first time there wasn't one. I changed that test to only run in default mode.
Comment 6 Keith Miller 2020-01-29 19:47:37 PST 
rdar://problem/58862199
Comment 7 Keith Miller 2020-01-29 23:11:03 PST 
Created attachment 389230 [details]
Patch for landing
Comment 8 WebKit Commit Bot 2020-01-29 23:42:13 PST 
Comment on attachment 389230 [details]
Patch for landing

Rejecting attachment 389230 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
RGET -DENABLE_XSLT -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.14 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore -I. -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources-normal/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders --system-header-prefix=unicode/ -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/JavaScriptCorePrefix-focfrejsjmfkulfpzqniyatstmxf/JavaScriptCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource95.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource95.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource95.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource95.o

Failed to run "['Tools/Scripts/build-webkit', '--release']" exit_code: 65
/JavaScriptCore-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore -I. -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources-normal/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders --system-header-prefix=unicode/ -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/JavaScriptCorePrefix-focfrejsjmfkulfpzqniyatstmxf/JavaScriptCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource95.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource95.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource95.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource95.o

Full output: https://webkit-queues.webkit.org/results/13313829
Comment 9 Keith Miller 2020-01-30 07:54:31 PST 
Created attachment 389248 [details]
Patch for landing
Comment 10 WebKit Commit Bot 2020-01-30 08:25:46 PST 
Comment on attachment 389248 [details]
Patch for landing

Rejecting attachment 389248 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-03', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 5000 characters of output:
TITIONING -DENABLE_CHANNEL_MESSAGING -DENABLE_CONTENT_FILTERING -DENABLE_CSS_BOX_DECORATION_BREAK -DENABLE_CSS_COMPOSITING -DENABLE_CSS_CONIC_GRADIENTS -DENABLE_CSS_PAINTING_API -DENABLE_CSS_SCROLL_SNAP -DENABLE_CSS_SELECTORS_LEVEL4 -DENABLE_CSS_TRAILING_WORD -DENABLE_CSS_TYPED_OM -DENABLE_CURSOR_VISIBILITY -DENABLE_DARK_MODE_CSS -DENABLE_DATACUE_VALUE -DENABLE_DATALIST_ELEMENT -DENABLE_ENCRYPTED_MEDIA -DENABLE_EXPERIMENTAL_FEATURES -DENABLE_FILTERS_LEVEL_2 -DENABLE_FTL_JIT -DENABLE_FULLSCREEN_API -DENABLE_PICTURE_IN_PICTURE_API -DENABLE_GAMEPAD -DENABLE_GEOLOCATION -DENABLE_GPU_PROCESS -DENABLE_INDEXED_DATABASE -DENABLE_INDEXED_DATABASE_IN_WORKERS -DENABLE_INPUT_TYPE_COLOR -DENABLE_INSPECTOR_ALTERNATE_DISPATCHERS -DENABLE_INSPECTOR_TELEMETRY -DENABLE_INTERSECTION_OBSERVER -DENABLE_INTL -DENABLE_LAYOUT_FORMATTING_CONTEXT -DENABLE_LEGACY_CSS_VENDOR_PREFIXES -DENABLE_LEGACY_CUSTOM_PROTOCOL_MANAGER -DENABLE_LEGACY_ENCRYPTED_MEDIA -DENABLE_MATHML -DENABLE_MEDIA_CONTROLS_SCRIPT -DENABLE_MEDIA_SOURCE -DENABLE_MEDIA_STREAM -DENABLE_MEMORY_SAMPLER -DENABLE_METER_ELEMENT -DENABLE_MOUSE_CURSOR_SCALE -DENABLE_NETWORK_CACHE_SPECULATIVE_REVALIDATION -DENABLE_NETWORK_CACHE_STALE_WHILE_REVALIDATE -DENABLE_NOTIFICATIONS -DENABLE_PAYMENT_REQUEST -DENABLE_PDFKIT_PLUGIN -DENABLE_POINTER_EVENTS -DENABLE_POINTER_LOCK -DENABLE_PUBLIC_SUFFIX_LIST -DENABLE_REMOTE_INSPECTOR -DENABLE_RESIZE_OBSERVER -DENABLE_RESOURCE_LOAD_STATISTICS -DENABLE_RESOURCE_USAGE -DENABLE_RUBBER_BANDING -DENABLE_SANDBOX_EXTENSIONS -DENABLE_SERVER_PRECONNECT -DENABLE_SERVICE_CONTROLS -DENABLE_SERVICE_WORKER -DENABLE_SHAREABLE_RESOURCE -DENABLE_SPEECH_SYNTHESIS -DENABLE_STREAMS_API -DENABLE_SVG_FONTS -DENABLE_TELEPHONE_NUMBER_DETECTION -DENABLE_TEXT_AUTOSIZING -DENABLE_USERSELECT_ALL -DENABLE_USER_MESSAGE_HANDLERS -DENABLE_VARIATION_FONTS -DENABLE_VIDEO -DENABLE_VIDEO_PRESENTATION_MODE -DENABLE_VIDEO_TRACK -DENABLE_VIDEO_USES_ELEMENT_FULLSCREEN -DENABLE_WEBDRIVER_MOUSE_INTERACTIONS -DENABLE_WEBDRIVER_KEYBOARD_INTERACTIONS -DENABLE_WEBGL -DENABLE_WEBGL2 -DENABLE_WEBGPU -DENABLE_WEB_AUDIO -DENABLE_WEB_AUTHN -DENABLE_WEB_CRYPTO -DENABLE_WEB_RTC -DENABLE_WIRELESS_PLAYBACK_TARGET -DENABLE_XSLT -DU_HIDE_DEPRECATED_API -DU_DISABLE_RENAMING=1 -DU_SHOW_CPLUSPLUS_API=0 -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk -fasm-blocks -fstrict-aliasing -Wdeprecated-declarations -Winvalid-offsetof -mmacosx-version-min=10.14 -g -fvisibility=hidden -fvisibility-inlines-hidden -fno-threadsafe-statics -Wno-sign-conversion -Winfinite-recursion -Wmove -Wcomma -Wblock-capture-autoreleasing -Wstrict-prototypes -Wrange-loop-analysis -Wno-semicolon-before-method-body -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-generated-files.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-own-target-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-all-target-headers.hmap -iquote /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/JavaScriptCore-project-headers.hmap -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore -I. -I/Volumes/Data/EWS/WebKit/WebKitBuild/Release/usr/local/include -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources-normal/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources/x86_64 -I/Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/DerivedSources -Wall -Wextra -Wcast-qual -Wchar-subscripts -Wconditional-uninitialized -Wextra-tokens -Wformat=2 -Winit-self -Wmissing-format-attribute -Wmissing-noreturn -Wpacked -Wpointer-arith -Wredundant-decls -Wundef -Wwrite-strings -Wexit-time-destructors -Wglobal-constructors -Wtautological-compare -Wimplicit-fallthrough -F/Volumes/Data/EWS/WebKit/WebKitBuild/Release -isystem /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.14.sdk/System/Library/Frameworks/System.framework/PrivateHeaders --system-header-prefix=unicode/ -include /Volumes/Data/EWS/WebKit/WebKitBuild/PrecompiledHeaders/JavaScriptCorePrefix-focfrejsjmfkulfpzqniyatstmxf/JavaScriptCorePrefix.h -MMD -MT dependencies -MF /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource94.d --serialize-diagnostics /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource94.dia -c /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/JavaScriptCore/unified-sources/UnifiedSource94.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/JavaScriptCore.build/Release/JavaScriptCore.build/Objects-normal/x86_64/UnifiedSource94.o

Full output: https://webkit-queues.webkit.org/results/13313979
Comment 11 Keith Miller 2020-01-30 09:35:45 PST 
Created attachment 389256 [details]
Patch for landing
Comment 12 WebKit Commit Bot 2020-01-30 11:04:19 PST 
Comment on attachment 389256 [details]
Patch for landing

Clearing flags on attachment: 389256

Committed r255440: <https://trac.webkit.org/changeset/255440>
Comment 13 WebKit Commit Bot 2020-01-30 11:04:21 PST 
All reviewed patches have been landed.  Closing bug.
Comment 14 Ross Kirsling 2020-01-30 11:25:34 PST 
Comment on attachment 389256 [details]
Patch for landing

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

> Source/JavaScriptCore/ChangeLog:13
> +        expressions. Intrestingly, this was not tested in test262.

Can we make a test262 issue?
Comment 15 Keith Miller 2020-01-30 12:39:48 PST 
Comment on attachment 389256 [details]
Patch for landing

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

>> Source/JavaScriptCore/ChangeLog:13
>> +        expressions. Intrestingly, this was not tested in test262.
> 
> Can we make a test262 issue?

I’ll file an issue, but I doubt I’ll have the bandwidth to make the test. It seems like there’s a general problem that test262 doesn’t test early errors inside functions. This means it doesn’t stress out SyntaxChecker...