Bug 182549 - put_to_scope/get_from_scope should not cache lexical scopes when expecting a global object
Summary: put_to_scope/get_from_scope should not cache lexical scopes when expecting a ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Keith Miller
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-02-06 13:41 PST by Keith Miller
Modified: 2018-03-25 22:35 PDT (History)
6 users (show)

See Also:

Attachments
Patch (9.97 KB, patch)
2018-02-06 13:54 PST, Keith Miller 		sbarati: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Keith Miller 2018-02-06 13:41:37 PST 
put_to_scope/get_from_scope should not cache lexical scopes when expecting a global object
Comment 1 Keith Miller 2018-02-06 13:54:02 PST 
Created attachment 333214 [details]
Patch
Comment 2 Saam Barati 2018-02-06 14:03:43 PST 
Comment on attachment 333214 [details]
Patch

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

> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:861
> +            emitLoadWithStructureCheck(scope, structureSlot); // Structure check covers var injection since we don't cache structures for anything but the GlobalObject.

Also: this should say that resolve_scope does the check for var injection firing.
Comment 3 Mark Lam 2018-02-06 14:04:06 PST 
<rdar://problem/36189995>
Comment 4 Saam Barati 2018-02-06 14:05:38 PST 
Comment on attachment 333214 [details]
Patch

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

> Source/JavaScriptCore/ChangeLog:12
> +        in the DFG, does not follow this same assumption so we could

no comma needed

> Source/JavaScriptCore/ChangeLog:17
> +        GlobalPropertyWithVarInjectionChecks.

or GlobalProperty
Comment 5 Keith Miller 2018-02-06 14:14:50 PST 
Comment on attachment 333214 [details]
Patch

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

>> Source/JavaScriptCore/ChangeLog:12
>> +        in the DFG, does not follow this same assumption so we could
> 
> no comma needed

Fixed.

>> Source/JavaScriptCore/ChangeLog:17
>> +        GlobalPropertyWithVarInjectionChecks.
> 
> or GlobalProperty

I don't think that was possible before? I can update the sentence though.

>> Source/JavaScriptCore/jit/JITPropertyAccess.cpp:861
>> +            emitLoadWithStructureCheck(scope, structureSlot); // Structure check covers var injection since we don't cache structures for anything but the GlobalObject.
> 
> Also: this should say that resolve_scope does the check for var injection firing.

Fixed.
Comment 6 Keith Miller 2018-02-06 14:42:31 PST 
Committed r228193: <https://trac.webkit.org/changeset/228193>
Comment 7 JF Bastien 2018-02-23 16:59:43 PST 
Comment on attachment 333214 [details]
Patch

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

> Source/JavaScriptCore/runtime/Options.h:253
> +    v(bool, enableJITDebugAssetions, !ASSERT_DISABLED, Normal, nullptr) \

Typo enableJITDebugAssetions
Comment 8 Saam Barati 2018-03-25 22:35:47 PDT 
(In reply to JF Bastien from comment #7)
> Comment on attachment 333214 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=333214&action=review
> 
> > Source/JavaScriptCore/runtime/Options.h:253
> > +    v(bool, enableJITDebugAssetions, !ASSERT_DISABLED, Normal, nullptr) \
> 
> Typo enableJITDebugAssetions

https://bugs.webkit.org/show_bug.cgi?id=184001