Bug 137839 - Don't create cached functions that access lexicalGlobalObject()
Summary: Don't create cached functions that access lexicalGlobalObject()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Michael Saboff
URL:
Keywords:
Depends on:
Blocks: 136724 136901 137907
  Show dependency treegraph
 
Reported: 2014-10-17 15:05 PDT by Michael Saboff
Modified: 2014-10-22 14:14 PDT (History)
0 users

See Also:


Attachments
Patch (22.16 KB, patch)
2014-10-17 15:40 PDT, Michael Saboff
ggaren: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Saboff 2014-10-17 15:05:19 PDT
The current implementation of lexicalGlobalObject() uses the JSScope stored in the current call frame.  For native functions, the JSScope is copied from the caller's frame.  These two behaviors together mean that a native function that calls lexicalGlobalObject() will not get their true lexicalGlobalObject, but their caller's.  As part of eliminating the JSScope slot from the call frame, we need to clean up code that depends on this behavior.  Therefore native functions that access their lexicalGlobalObject need to always be created for each instance of their prototype object, so that their instance has their lexicalGlobalObject.
Comment 1 Michael Saboff 2014-10-17 15:40:44 PDT
Created attachment 240045 [details]
Patch
Comment 2 Geoffrey Garen 2014-10-17 15:45:56 PDT
Comment on attachment 240045 [details]
Patch

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

r=me

> LayoutTests/ChangeLog:17
> +        a parents and child windows instances are the same.  Now they are always different.

"parent's"
"window's"
Comment 3 Michael Saboff 2014-10-17 15:58:30 PDT
Committed r174847: <http://trac.webkit.org/changeset/174847>
Comment 4 Darin Adler 2014-10-17 23:20:49 PDT
Comment on attachment 240045 [details]
Patch

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

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:65
> +    if (propertyName == Identifier(exec, "open")) {

I believe this is less efficient than:

    if (propertyName == "open") {

> Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:76
> +    const HashTableValue* entry = JSHTMLDocument::info()->staticPropHashTable->entry(propertyName);
> +    if (entry) {

Would be nice to put this definition inside the if.
Comment 5 Michael Saboff 2014-10-20 10:55:39 PDT
(In reply to comment #4)
> Comment on attachment 240045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=240045&action=review
> 
> > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:65
> > +    if (propertyName == Identifier(exec, "open")) {
> 
> I believe this is less efficient than:
> 
>     if (propertyName == "open") {
> 
> > Source/WebCore/bindings/js/JSHTMLDocumentCustom.cpp:76
> > +    const HashTableValue* entry = JSHTMLDocument::info()->staticPropHashTable->entry(propertyName);
> > +    if (entry) {
> 
> Would be nice to put this definition inside the if.

Tracked in <https://bugs.webkit.org/show_bug.cgi?id=137885>