[webkit-changes] [192693] trunk

Fri, 20 Nov 2015 12:46:02 -0800

Title: [192693] trunk
Revision
192693
Author
cdu...@apple.com
Date
2015-11-20 12:45:12 -0800 (Fri, 20 Nov 2015)

Log Message

Caching of properties on objects that have named property getters is sometimes incorrect
https://bugs.webkit.org/show_bug.cgi?id=151453
<rdar://problem/23049343>

Reviewed by Gavin Barraclough.

Source/_javascript_Core:

Add new GetOwnPropertySlotIsImpureForPropertyAbsence TypeInfo flag to be
used by objects that have a non-'OverrideBuiltins' named property getter.
This flag prevents caching of properties that are missing as a named
property with this name may later become available.

Objects with an 'OverrideBuiltins' named property getter will keep using
the GetOwnPropertySlotIsImpure TypeInfo flag, which prevents all property
caching since named properties can override own properties or properties
on the prototype.

* bytecode/ComplexGetStatus.cpp:
(JSC::ComplexGetStatus::computeFor):
* bytecode/PropertyCondition.cpp:
(JSC::PropertyCondition::isStillValid):
* jit/Repatch.cpp:
(JSC::tryCacheGetByID):
(JSC::tryRepatchIn):
* jsc.cpp:
* runtime/JSTypeInfo.h:
(JSC::TypeInfo::getOwnPropertySlotIsImpure):
(JSC::TypeInfo::getOwnPropertySlotIsImpureForPropertyAbsence):
(JSC::TypeInfo::prohibitsPropertyCaching): Deleted.
* runtime/Structure.h:

Source/WebCore:

In r188590, we dropped the JSC::GetOwnPropertySlotIsImpure TypeInfo flag for
interfaces that have a non-'OverrideBuiltins' named property getter in order
to allow caching of properties returns by GetOwnPropertySlot(). We assumed
this was safe as it was no longer possible for named properties to override
own properties (or properties on the prototype).

However, there is an issue when we cache the non-existence of a property.
Even though at one point the property did not exist, a named property with
this name may later become available. In such case, caching would cause us
to wrongly report a property as missing.

To address the problem, this patch introduces a new
GetOwnPropertySlotIsImpureForPropertyAbsence TypeInfo flag and uses it for
interfaces that have a non-'OverrideBuiltins' named property getter. This
will cause us to not cache the fact that a property is missing on such
objects, while maintaining the performance win from r188590 in the common
case.

Test: fast/dom/NamedNodeMap-named-getter-caching.html

* bindings/scripts/CodeGeneratorJS.pm:
(GenerateHeader):
* bindings/scripts/test/JS/JSTestCustomNamedGetter.h:
* bindings/scripts/test/JS/JSTestEventTarget.h:
* bindings/scripts/test/JS/JSTestOverrideBuiltins.h:

LayoutTests:

Add layout test to make sure caching does not cause NamedNodeMap's
named property getter to sometimes wrongly report a property as
missing.

* fast/dom/NamedNodeMap-named-getter-caching-expected.txt: Added.
* fast/dom/NamedNodeMap-named-getter-caching.html: Added.

Modified Paths

Added Paths

Diff

Modified: trunk/LayoutTests/ChangeLog (192692 => 192693)


--- trunk/LayoutTests/ChangeLog	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/LayoutTests/ChangeLog	2015-11-20 20:45:12 UTC (rev 192693)
@@ -1,3 +1,18 @@
+2015-11-20  Chris Dumez  <cdu...@apple.com>
+
+        Caching of properties on objects that have named property getters is sometimes incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=151453
+        <rdar://problem/23049343>
+
+        Reviewed by Gavin Barraclough.
+
+        Add layout test to make sure caching does not cause NamedNodeMap's
+        named property getter to sometimes wrongly report a property as
+        missing.
+
+        * fast/dom/NamedNodeMap-named-getter-caching-expected.txt: Added.
+        * fast/dom/NamedNodeMap-named-getter-caching.html: Added.
+
 2015-11-20  Zalan Bujtas  <za...@apple.com>
 
         Simple line layout: Add text-indent support.

Added: trunk/LayoutTests/fast/dom/NamedNodeMap-named-getter-caching-expected.txt (0 => 192693)


--- trunk/LayoutTests/fast/dom/NamedNodeMap-named-getter-caching-expected.txt	                        (rev 0)
+++ trunk/LayoutTests/fast/dom/NamedNodeMap-named-getter-caching-expected.txt	2015-11-20 20:45:12 UTC (rev 192693)
@@ -0,0 +1,11 @@
+Tests caching of result of NamedNodeMap named property getter
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+PASS bodyAttributes.class is undefined
+PASS lastIterationHasRightValue is true
+PASS successfullyParsed is true
+
+TEST COMPLETE
+

Added: trunk/LayoutTests/fast/dom/NamedNodeMap-named-getter-caching.html (0 => 192693)


--- trunk/LayoutTests/fast/dom/NamedNodeMap-named-getter-caching.html	                        (rev 0)
+++ trunk/LayoutTests/fast/dom/NamedNodeMap-named-getter-caching.html	2015-11-20 20:45:12 UTC (rev 192693)
@@ -0,0 +1,28 @@
+<!DOCTYPE html>
+<html>
+<body>
+<script src=""
+<script>
+description("Tests caching of result of NamedNodeMap named property getter");
+
+var bodyAttributes = document.body.attributes;
+shouldBe("bodyAttributes.class", "undefined");
+
+var lastIterationHasRightValue = false;
+for (var i = 0; i < 1000; i++) {
+    if (i == 999)
+        document.body.setAttribute('class', 'test');
+    if (bodyAttributes.class != undefined) {
+        if (i == 999)
+            lastIterationHasRightValue = true;
+        else
+            testFailed("The body element should NOT have a 'class' attribute set");
+    }
+}
+
+shouldBeTrue("lastIterationHasRightValue");
+
+</script>
+<script src=""
+</body>
+</html>

Modified: trunk/Source/_javascript_Core/ChangeLog (192692 => 192693)


--- trunk/Source/_javascript_Core/ChangeLog	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/ChangeLog	2015-11-20 20:45:12 UTC (rev 192693)
@@ -1,3 +1,35 @@
+2015-11-20  Chris Dumez  <cdu...@apple.com>
+
+        Caching of properties on objects that have named property getters is sometimes incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=151453
+        <rdar://problem/23049343>
+
+        Reviewed by Gavin Barraclough.
+
+        Add new GetOwnPropertySlotIsImpureForPropertyAbsence TypeInfo flag to be
+        used by objects that have a non-'OverrideBuiltins' named property getter.
+        This flag prevents caching of properties that are missing as a named
+        property with this name may later become available.
+
+        Objects with an 'OverrideBuiltins' named property getter will keep using
+        the GetOwnPropertySlotIsImpure TypeInfo flag, which prevents all property
+        caching since named properties can override own properties or properties
+        on the prototype.
+
+        * bytecode/ComplexGetStatus.cpp:
+        (JSC::ComplexGetStatus::computeFor):
+        * bytecode/PropertyCondition.cpp:
+        (JSC::PropertyCondition::isStillValid):
+        * jit/Repatch.cpp:
+        (JSC::tryCacheGetByID):
+        (JSC::tryRepatchIn):
+        * jsc.cpp:
+        * runtime/JSTypeInfo.h:
+        (JSC::TypeInfo::getOwnPropertySlotIsImpure):
+        (JSC::TypeInfo::getOwnPropertySlotIsImpureForPropertyAbsence):
+        (JSC::TypeInfo::prohibitsPropertyCaching): Deleted.
+        * runtime/Structure.h:
+
 2015-11-19  Joseph Pecoraro  <pecor...@apple.com>
 
         REGRESSION(r189433) Web Inspector: JSContext inspection exceptions should include native call frames by default

Modified: trunk/Source/_javascript_Core/bytecode/ComplexGetStatus.cpp (192692 => 192693)


--- trunk/Source/_javascript_Core/bytecode/ComplexGetStatus.cpp	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/bytecode/ComplexGetStatus.cpp	2015-11-20 20:45:12 UTC (rev 192693)
@@ -34,7 +34,7 @@
     Structure* headStructure, const ObjectPropertyConditionSet& conditionSet, UniquedStringImpl* uid)
 {
     // FIXME: We should assert that we never see a structure that
-    // hasImpureGetOwnPropertySlot() but for which we don't
+    // getOwnPropertySlotIsImpure() but for which we don't
     // newImpurePropertyFiresWatchpoints(). We're not at a point where we can do
     // that, yet.
     // https://bugs.webkit.org/show_bug.cgi?id=131810

Modified: trunk/Source/_javascript_Core/bytecode/PropertyCondition.cpp (192692 => 192693)


--- trunk/Source/_javascript_Core/bytecode/PropertyCondition.cpp	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/bytecode/PropertyCondition.cpp	2015-11-20 20:45:12 UTC (rev 192693)
@@ -213,10 +213,13 @@
     // "shadow" an existing JS property on the same object. Hence it affects both presence and
     // absence. It doesn't affect AbsenceOfSetter because impure properties aren't ever setters.
     switch (m_kind) {
+    case Absence:
+        if (structure->typeInfo().getOwnPropertySlotIsImpure() || structure->typeInfo().getOwnPropertySlotIsImpureForPropertyAbsence())
+            return false;
+        break;
     case Presence:
-    case Absence:
     case Equivalence:
-        if (structure->typeInfo().hasImpureGetOwnPropertySlot())
+        if (structure->typeInfo().getOwnPropertySlotIsImpure())
             return false;
         break;
     default:

Modified: trunk/Source/_javascript_Core/jit/Repatch.cpp (192692 => 192693)


--- trunk/Source/_javascript_Core/jit/Repatch.cpp	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/jit/Repatch.cpp	2015-11-20 20:45:12 UTC (rev 192693)
@@ -283,6 +283,9 @@
             if (structure->typeInfo().prohibitsPropertyCaching() || structure->isDictionary())
                 return GiveUpOnCache;
             
+            if (slot.isUnset() && structure->typeInfo().getOwnPropertySlotIsImpureForPropertyAbsence())
+                return GiveUpOnCache;
+
             if (slot.isUnset()) {
                 conditionSet = generateConditionsForPropertyMiss(
                     vm, codeBlock, exec, structure, propertyName.impl());
@@ -485,7 +488,7 @@
     if (forceICFailure(exec))
         return GiveUpOnCache;
     
-    if (!base->structure()->propertyAccessesAreCacheable())
+    if (!base->structure()->propertyAccessesAreCacheable() || (!wasFound && !base->structure()->propertyAccessesAreCacheableForAbsence()))
         return GiveUpOnCache;
     
     if (wasFound) {

Modified: trunk/Source/_javascript_Core/jsc.cpp (192692 => 192693)


--- trunk/Source/_javascript_Core/jsc.cpp	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/jsc.cpp	2015-11-20 20:45:12 UTC (rev 192693)
@@ -267,7 +267,7 @@
 
     DECLARE_INFO;
     typedef JSNonFinalObject Base;
-    static const unsigned StructureFlags = Base::StructureFlags | JSC::HasImpureGetOwnPropertySlot | JSC::OverridesGetOwnPropertySlot;
+    static const unsigned StructureFlags = Base::StructureFlags | JSC::GetOwnPropertySlotIsImpure | JSC::OverridesGetOwnPropertySlot;
 
     static Structure* createStructure(VM& vm, JSGlobalObject* globalObject, JSValue prototype)
     {

Modified: trunk/Source/_javascript_Core/runtime/JSTypeInfo.h (192692 => 192693)


--- trunk/Source/_javascript_Core/runtime/JSTypeInfo.h	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/runtime/JSTypeInfo.h	2015-11-20 20:45:12 UTC (rev 192693)
@@ -47,9 +47,10 @@
 
 static const unsigned OverridesGetPropertyNames = 1 << 8;
 static const unsigned ProhibitsPropertyCaching = 1 << 9;
-static const unsigned HasImpureGetOwnPropertySlot = 1 << 10;
+static const unsigned GetOwnPropertySlotIsImpure = 1 << 10;
 static const unsigned NewImpurePropertyFiresWatchpoints = 1 << 11;
 static const unsigned IsEnvironmentRecord = 1 << 12;
+static const unsigned GetOwnPropertySlotIsImpureForPropertyAbsence = 1 << 13;
 
 class TypeInfo {
 public:
@@ -91,7 +92,8 @@
     bool structureIsImmortal() const { return isSetOnFlags1(StructureIsImmortal); }
     bool overridesGetPropertyNames() const { return isSetOnFlags2(OverridesGetPropertyNames); }
     bool prohibitsPropertyCaching() const { return isSetOnFlags2(ProhibitsPropertyCaching); }
-    bool hasImpureGetOwnPropertySlot() const { return isSetOnFlags2(HasImpureGetOwnPropertySlot); }
+    bool getOwnPropertySlotIsImpure() const { return isSetOnFlags2(GetOwnPropertySlotIsImpure); }
+    bool getOwnPropertySlotIsImpureForPropertyAbsence() const { return isSetOnFlags2(GetOwnPropertySlotIsImpureForPropertyAbsence); }
     bool newImpurePropertyFiresWatchpoints() const { return isSetOnFlags2(NewImpurePropertyFiresWatchpoints); }
     bool isEnvironmentRecord() const { return isSetOnFlags2(IsEnvironmentRecord); }

Modified: trunk/Source/_javascript_Core/runtime/Structure.h (192692 => 192693)


--- trunk/Source/_javascript_Core/runtime/Structure.h	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/_javascript_Core/runtime/Structure.h	2015-11-20 20:45:12 UTC (rev 192693)
@@ -204,13 +204,18 @@
     {
         return dictionaryKind() != UncachedDictionaryKind
             && !typeInfo().prohibitsPropertyCaching()
-            && !(typeInfo().hasImpureGetOwnPropertySlot() && !typeInfo().newImpurePropertyFiresWatchpoints());
+            && !(typeInfo().getOwnPropertySlotIsImpure() && !typeInfo().newImpurePropertyFiresWatchpoints());
     }
+
+    bool propertyAccessesAreCacheableForAbsence()
+    {
+        return !typeInfo().getOwnPropertySlotIsImpureForPropertyAbsence();
+    }
     
     bool needImpurePropertyWatchpoint()
     {
         return propertyAccessesAreCacheable()
-            && typeInfo().hasImpureGetOwnPropertySlot()
+            && typeInfo().getOwnPropertySlotIsImpure()
             && typeInfo().newImpurePropertyFiresWatchpoints();
     }
 
@@ -218,7 +223,7 @@
     // DFG from inlining property accesses since structures don't transition when a new impure property appears.
     bool takesSlowPathInDFGForImpureProperty()
     {
-        return typeInfo().hasImpureGetOwnPropertySlot();
+        return typeInfo().getOwnPropertySlotIsImpure();
     }
     
     // Type accessors.

Modified: trunk/Source/WebCore/ChangeLog (192692 => 192693)


--- trunk/Source/WebCore/ChangeLog	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/WebCore/ChangeLog	2015-11-20 20:45:12 UTC (rev 192693)
@@ -1,3 +1,37 @@
+2015-11-20  Chris Dumez  <cdu...@apple.com>
+
+        Caching of properties on objects that have named property getters is sometimes incorrect
+        https://bugs.webkit.org/show_bug.cgi?id=151453
+        <rdar://problem/23049343>
+
+        Reviewed by Gavin Barraclough.
+
+        In r188590, we dropped the JSC::GetOwnPropertySlotIsImpure TypeInfo flag for
+        interfaces that have a non-'OverrideBuiltins' named property getter in order
+        to allow caching of properties returns by GetOwnPropertySlot(). We assumed
+        this was safe as it was no longer possible for named properties to override
+        own properties (or properties on the prototype).
+
+        However, there is an issue when we cache the non-existence of a property.
+        Even though at one point the property did not exist, a named property with
+        this name may later become available. In such case, caching would cause us
+        to wrongly report a property as missing.
+
+        To address the problem, this patch introduces a new
+        GetOwnPropertySlotIsImpureForPropertyAbsence TypeInfo flag and uses it for
+        interfaces that have a non-'OverrideBuiltins' named property getter. This
+        will cause us to not cache the fact that a property is missing on such
+        objects, while maintaining the performance win from r188590 in the common
+        case.
+
+        Test: fast/dom/NamedNodeMap-named-getter-caching.html
+
+        * bindings/scripts/CodeGeneratorJS.pm:
+        (GenerateHeader):
+        * bindings/scripts/test/JS/JSTestCustomNamedGetter.h:
+        * bindings/scripts/test/JS/JSTestEventTarget.h:
+        * bindings/scripts/test/JS/JSTestOverrideBuiltins.h:
+
 2015-11-19  Simon Fraser  <simon.fra...@apple.com>
 
         Back-buffer to front-buffer copy fails for some buffer formats

Modified: trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm (192692 => 192693)


--- trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/WebCore/bindings/scripts/CodeGeneratorJS.pm	2015-11-20 20:45:12 UTC (rev 192693)
@@ -982,20 +982,23 @@
     my $namedGetterFunction = GetNamedGetterFunction($interface);
     my $indexedGetterFunction = GetIndexedGetterFunction($interface);
 
-    my $hasImpureNamedGetter = $interface->extendedAttributes->{"OverrideBuiltins"}
-        && ($namedGetterFunction || $interface->extendedAttributes->{"CustomNamedGetter"});
+    my $hasNamedGetter = $namedGetterFunction
+        || $interface->extendedAttributes->{"CustomNamedGetter"};
 
     my $hasComplexGetter =
         $indexedGetterFunction
         || $interface->extendedAttributes->{"JSCustomGetOwnPropertySlotAndDescriptor"}
         || $interface->extendedAttributes->{"CustomGetOwnPropertySlot"}
-        || $namedGetterFunction
-        || $interface->extendedAttributes->{"CustomNamedGetter"};
+        || $hasNamedGetter;
     
     my $hasGetter = InstanceOverridesGetOwnPropertySlot($interface);
 
-    if ($hasImpureNamedGetter) {
-        $structureFlags{"JSC::HasImpureGetOwnPropertySlot"} = 1;
+    if ($hasNamedGetter) {
+        if ($interface->extendedAttributes->{"OverrideBuiltins"}) {
+            $structureFlags{"JSC::GetOwnPropertySlotIsImpure"} = 1;
+        } else {
+            $structureFlags{"JSC::GetOwnPropertySlotIsImpureForPropertyAbsence"} = 1;
+        }
     }
     if ($interface->extendedAttributes->{"NewImpurePropertyFiresWatchpoints"}) {
         $structureFlags{"JSC::NewImpurePropertyFiresWatchpoints"} = 1;

Modified: trunk/Source/WebCore/bindings/scripts/test/JS/JSTestCustomNamedGetter.h (192692 => 192693)


--- trunk/Source/WebCore/bindings/scripts/test/JS/JSTestCustomNamedGetter.h	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/WebCore/bindings/scripts/test/JS/JSTestCustomNamedGetter.h	2015-11-20 20:45:12 UTC (rev 192693)
@@ -53,7 +53,7 @@
 
     static JSC::JSValue getConstructor(JSC::VM&, JSC::JSGlobalObject*);
 public:
-    static const unsigned StructureFlags = JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::OverridesGetOwnPropertySlot | Base::StructureFlags;
+    static const unsigned StructureFlags = JSC::GetOwnPropertySlotIsImpureForPropertyAbsence | JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::OverridesGetOwnPropertySlot | Base::StructureFlags;
 protected:
     JSTestCustomNamedGetter(JSC::Structure*, JSDOMGlobalObject&, Ref<TestCustomNamedGetter>&&);

Modified: trunk/Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h (192692 => 192693)


--- trunk/Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/WebCore/bindings/scripts/test/JS/JSTestEventTarget.h	2015-11-20 20:45:12 UTC (rev 192693)
@@ -57,7 +57,7 @@
     static void visitChildren(JSCell*, JSC::SlotVisitor&);
 
 public:
-    static const unsigned StructureFlags = JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::MasqueradesAsUndefined | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetPropertyNames | Base::StructureFlags;
+    static const unsigned StructureFlags = JSC::GetOwnPropertySlotIsImpureForPropertyAbsence | JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::MasqueradesAsUndefined | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetPropertyNames | Base::StructureFlags;
 protected:
     JSTestEventTarget(JSC::Structure*, JSDOMGlobalObject&, Ref<TestEventTarget>&&);

Modified: trunk/Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.h (192692 => 192693)


--- trunk/Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.h	2015-11-20 20:40:26 UTC (rev 192692)
+++ trunk/Source/WebCore/bindings/scripts/test/JS/JSTestOverrideBuiltins.h	2015-11-20 20:45:12 UTC (rev 192693)
@@ -54,7 +54,7 @@
     static void getOwnPropertyNames(JSC::JSObject*, JSC::ExecState*, JSC::PropertyNameArray&, JSC::EnumerationMode = JSC::EnumerationMode());
     static JSC::JSValue getConstructor(JSC::VM&, JSC::JSGlobalObject*);
 public:
-    static const unsigned StructureFlags = JSC::HasImpureGetOwnPropertySlot | JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetPropertyNames | Base::StructureFlags;
+    static const unsigned StructureFlags = JSC::GetOwnPropertySlotIsImpure | JSC::InterceptsGetOwnPropertySlotByIndexEvenWhenLengthIsNotZero | JSC::OverridesGetOwnPropertySlot | JSC::OverridesGetPropertyNames | Base::StructureFlags;
 protected:
     JSTestOverrideBuiltins(JSC::Structure*, JSDOMGlobalObject&, Ref<TestOverrideBuiltins>&&);
 

_______________________________________________
webkit-changes mailing list
webkit-changes@lists.webkit.org
https://lists.webkit.org/mailman/listinfo/webkit-changes

Reply via email to