JSDOMWindow should have a WatchpointSet to fire on window close
authormhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 May 2014 20:26:17 +0000 (20:26 +0000)
committermhahnenberg@apple.com <mhahnenberg@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 9 May 2014 20:26:17 +0000 (20:26 +0000)
https://bugs.webkit.org/show_bug.cgi?id=132721

Reviewed by Filip Pizlo.

Source/JavaScriptCore:
This patch allows us to reset the inline caches that assumed they could skip
the first part of JSDOMWindow::getOwnPropertySlot that checks if the window has
been closed. This is part of getting rid of HasImpureGetOwnPropertySlot on JSDOMWindow.

PropertySlot now accepts a WatchpointSet which the inline cache code can look for
to see if it should create a new Watchpoint for that particular inline cache site.

* bytecode/Watchpoint.h:
* jit/Repatch.cpp:
(JSC::generateByIdStub):
(JSC::tryBuildGetByIDList):
(JSC::tryCachePutByID):
(JSC::tryBuildPutByIdList):
* runtime/PropertySlot.h:
(JSC::PropertySlot::PropertySlot):
(JSC::PropertySlot::watchpointSet):
(JSC::PropertySlot::setWatchpointSet):

Source/WebCore:
No new tests.

This patch allows us to reset the inline caches that assumed they could skip
the first part of JSDOMWindow::getOwnPropertySlot that checks if the window has
been closed. This is part of getting rid of HasImpureGetOwnPropertySlot on JSDOMWindow.

JSDOMWindowBase now has a WatchpointSet that the underlying DOMWindow fires when its
frame is cleared. In getOwnPropertySlot, we now pass this WatchpointSet to PropertySlot
which will shepherd it back up to the code that generates the inline cache (and the
Watchpoint for clearing it).

* bindings/js/JSDOMWindowBase.cpp:
(WebCore::JSDOMWindowBase::JSDOMWindowBase):
(WebCore::JSDOMWindowBase::fireFrameClearedWatchpointsForWindow):
* bindings/js/JSDOMWindowBase.h:
* bindings/js/JSDOMWindowCustom.cpp:
(WebCore::JSDOMWindow::getOwnPropertySlot):
* page/DOMWindow.cpp:
(WebCore::DOMWindow::frameDestroyed):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@168548 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/GetByIdStatus.cpp
Source/JavaScriptCore/bytecode/PolymorphicGetByIdList.h
Source/JavaScriptCore/bytecode/Watchpoint.h
Source/JavaScriptCore/jit/Repatch.cpp
Source/JavaScriptCore/runtime/PropertySlot.h
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMWindowBase.cpp
Source/WebCore/bindings/js/JSDOMWindowBase.h
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp
Source/WebCore/page/DOMWindow.cpp

index 1a96bbd..656b35e 100644 (file)
@@ -1,3 +1,28 @@
+2014-05-08  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        JSDOMWindow should have a WatchpointSet to fire on window close
+        https://bugs.webkit.org/show_bug.cgi?id=132721
+
+        Reviewed by Filip Pizlo.
+
+        This patch allows us to reset the inline caches that assumed they could skip 
+        the first part of JSDOMWindow::getOwnPropertySlot that checks if the window has 
+        been closed. This is part of getting rid of HasImpureGetOwnPropertySlot on JSDOMWindow.
+
+        PropertySlot now accepts a WatchpointSet which the inline cache code can look for
+        to see if it should create a new Watchpoint for that particular inline cache site.
+
+        * bytecode/Watchpoint.h:
+        * jit/Repatch.cpp:
+        (JSC::generateByIdStub):
+        (JSC::tryBuildGetByIDList):
+        (JSC::tryCachePutByID):
+        (JSC::tryBuildPutByIdList):
+        * runtime/PropertySlot.h:
+        (JSC::PropertySlot::PropertySlot):
+        (JSC::PropertySlot::watchpointSet):
+        (JSC::PropertySlot::setWatchpointSet):
+
 2014-05-09  Tanay C  <tanay.c@samsung.com>
 
         Fix build warning (uninitialized variable) in DFGFixupPhase.cpp 
index da2cf59..04e2360 100644 (file)
@@ -178,10 +178,23 @@ GetByIdStatus GetByIdStatus::computeForStubInfo(
     PolymorphicGetByIdList* list = 0;
     if (stubInfo->accessType == access_get_by_id_list) {
         list = stubInfo->u.getByIdList.list;
+        bool makesCalls = false;
+        bool isWatched = false;
         for (unsigned i = 0; i < list->size(); ++i) {
-            if (list->at(i).doesCalls())
-                return GetByIdStatus(MakesCalls, true);
+            const GetByIdAccess& access = list->at(i);
+            if (access.doesCalls()) {
+                makesCalls = true;
+                break;
+            }
+            if (access.isWatched()) {
+                isWatched = true;
+                continue;
+            }
         }
+        if (makesCalls)
+            return GetByIdStatus(MakesCalls, true);
+        if (isWatched)
+            return GetByIdStatus(TakesSlowPath, true);
     }
     
     // Finally figure out if we can derive an access strategy.
@@ -215,7 +228,7 @@ GetByIdStatus GetByIdStatus::computeForStubInfo(
         
     case access_get_by_id_list: {
         for (unsigned listIndex = 0; listIndex < list->size(); ++listIndex) {
-            ASSERT(!list->at(listIndex).doesCalls());
+            ASSERT(list->at(listIndex).isSimple());
             
             Structure* structure = list->at(listIndex).structure();
             
index 945ba4d..265b0ad 100644 (file)
@@ -45,6 +45,7 @@ public:
         Invalid,
         SimpleInline, // This is the patched inline access.
         SimpleStub, // This is a stub.
+        WatchedStub,
         Getter,
         CustomGetter
     };
@@ -80,6 +81,8 @@ public:
     }
     
     bool doesCalls() const { return type() == Getter || type() == CustomGetter; }
+    bool isWatched() const { return type() == WatchedStub; }
+    bool isSimple() const { return !doesCalls() && !isWatched(); }
     
     bool visitWeak(RepatchBuffer&) const;
 
index 8790f4e..7659c0d 100644 (file)
@@ -57,8 +57,8 @@ class InlineWatchpointSet;
 class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     friend class LLIntOffsetsExtractor;
 public:
-    WatchpointSet(WatchpointState);
-    ~WatchpointSet(); // Note that this will not fire any of the watchpoints; if you need to know when a WatchpointSet dies then you need a separate mechanism for this.
+    JS_EXPORT_PRIVATE WatchpointSet(WatchpointState);
+    JS_EXPORT_PRIVATE ~WatchpointSet(); // Note that this will not fire any of the watchpoints; if you need to know when a WatchpointSet dies then you need a separate mechanism for this.
     
     // It is safe to call this from another thread. It may return an old
     // state. Guarantees that if *first* read the state() of the thing being
index 75a18be..973c261 100644 (file)
@@ -285,8 +285,8 @@ static FunctionPtr customFor(const PutPropertySlot& slot)
 static void generateByIdStub(
     ExecState* exec, ByIdStubKind kind, const Identifier& propertyName,
     FunctionPtr custom, StructureStubInfo& stubInfo, StructureChain* chain, size_t count,
-    PropertyOffset offset, Structure* structure, bool loadTargetFromProxy, CodeLocationLabel successLabel,
-    CodeLocationLabel slowCaseLabel, RefPtr<JITStubRoutine>& stubRoutine)
+    PropertyOffset offset, Structure* structure, bool loadTargetFromProxy, WatchpointSet* watchpointSet,
+    CodeLocationLabel successLabel, CodeLocationLabel slowCaseLabel, RefPtr<JITStubRoutine>& stubRoutine)
 {
     VM* vm = &exec->vm();
     GPRReg baseGPR = static_cast<GPRReg>(stubInfo.patch.baseGPR);
@@ -329,6 +329,9 @@ static void generateByIdStub(
     if (structure->typeInfo().newImpurePropertyFiresWatchpoints())
         vm->registerWatchpointForImpureProperty(propertyName, stubInfo.addWatchpoint(codeBlock));
 
+    if (watchpointSet)
+        watchpointSet->add(stubInfo.addWatchpoint(codeBlock));
+
     Structure* currStructure = structure;
     JSObject* protoObject = 0;
     if (chain) {
@@ -692,6 +695,7 @@ static bool tryCacheGetByID(ExecState* exec, JSValue baseValue, const Identifier
     // Optimize self access.
     if (slot.slotBase() == baseValue
         && slot.isCacheableValue()
+        && !slot.watchpointSet()
         && MacroAssembler::isCompactPtrAlignedAddressOffset(maxOffsetRelativeToPatchedStorage(slot.cachedOffset()))) {
             repatchByIdSelfAccess(*vm, codeBlock, stubInfo, structure, propertyName, slot.cachedOffset(), operationGetByIdBuildList, true);
             stubInfo.initGetByIdSelf(*vm, codeBlock->ownerExecutable(), structure);
@@ -783,12 +787,13 @@ static bool tryBuildGetByIDList(ExecState* exec, JSValue baseValue, const Identi
     RefPtr<JITStubRoutine> stubRoutine;
     generateByIdStub(
         exec, kindFor(slot), ident, customFor(slot), stubInfo, prototypeChain, count, offset, 
-        structure, loadTargetFromProxy, stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToDone),
+        structure, loadTargetFromProxy, slot.watchpointSet(), 
+        stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToDone),
         CodeLocationLabel(list->currentSlowPathTarget(stubInfo)), stubRoutine);
     
     GetByIdAccess::AccessType accessType;
     if (slot.isCacheableValue())
-        accessType = GetByIdAccess::SimpleStub;
+        accessType = slot.watchpointSet() ? GetByIdAccess::WatchedStub : GetByIdAccess::SimpleStub;
     else if (slot.isCacheableGetter())
         accessType = GetByIdAccess::Getter;
     else
@@ -1201,7 +1206,7 @@ static bool tryCachePutByID(ExecState* exec, JSValue baseValue, const Identifier
 
         generateByIdStub(
             exec, kindFor(slot), ident, customFor(slot), stubInfo, prototypeChain, count,
-            offset, structure, false,
+            offset, structure, false, nullptr,
             stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToDone),
             stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToSlowCase),
             stubRoutine);
@@ -1333,7 +1338,7 @@ static bool tryBuildPutByIdList(ExecState* exec, JSValue baseValue, const Identi
 
         generateByIdStub(
             exec, kindFor(slot), propertyName, customFor(slot), stubInfo, prototypeChain, count,
-            offset, structure, false,
+            offset, structure, false, nullptr,
             stubInfo.callReturnLocation.labelAtOffset(stubInfo.patch.deltaCallToDone),
             CodeLocationLabel(list->currentSlowPathTarget()),
             stubRoutine);
index a954cc0..e633ec5 100644 (file)
@@ -61,6 +61,7 @@ public:
         : m_propertyType(TypeUnset)
         , m_offset(invalidOffset)
         , m_thisValue(thisValue)
+        , m_watchpointSet(nullptr)
     {
     }
 
@@ -104,6 +105,11 @@ public:
         return m_slotBase;
     }
 
+    WatchpointSet* watchpointSet() const
+    {
+        return m_watchpointSet;
+    }
+
     void setValue(JSObject* slotBase, unsigned attributes, JSValue value)
     {
         ASSERT(value);
@@ -210,6 +216,11 @@ public:
         m_offset = invalidOffset;
     }
 
+    void setWatchpointSet(WatchpointSet& set)
+    {
+        m_watchpointSet = &set;
+    }
+
 private:
     JS_EXPORT_PRIVATE JSValue functionGetter(ExecState*) const;
 
@@ -232,6 +243,7 @@ private:
     PropertyOffset m_offset;
     const JSValue m_thisValue;
     JSObject* m_slotBase;
+    WatchpointSet* m_watchpointSet;
 };
 
 } // namespace JSC
index 2e22cd7..f366843 100644 (file)
@@ -1,3 +1,30 @@
+2014-05-08  Mark Hahnenberg  <mhahnenberg@apple.com>
+
+        JSDOMWindow should have a WatchpointSet to fire on window close
+        https://bugs.webkit.org/show_bug.cgi?id=132721
+
+        Reviewed by Filip Pizlo.
+
+        No new tests.
+
+        This patch allows us to reset the inline caches that assumed they could skip 
+        the first part of JSDOMWindow::getOwnPropertySlot that checks if the window has 
+        been closed. This is part of getting rid of HasImpureGetOwnPropertySlot on JSDOMWindow.
+
+        JSDOMWindowBase now has a WatchpointSet that the underlying DOMWindow fires when its
+        frame is cleared. In getOwnPropertySlot, we now pass this WatchpointSet to PropertySlot
+        which will shepherd it back up to the code that generates the inline cache (and the 
+        Watchpoint for clearing it).
+
+        * bindings/js/JSDOMWindowBase.cpp:
+        (WebCore::JSDOMWindowBase::JSDOMWindowBase):
+        (WebCore::JSDOMWindowBase::fireFrameClearedWatchpointsForWindow):
+        * bindings/js/JSDOMWindowBase.h:
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::JSDOMWindow::getOwnPropertySlot):
+        * page/DOMWindow.cpp:
+        (WebCore::DOMWindow::frameDestroyed):
+
 2014-05-09  Zsolt Borbely  <zsborbely.u-szeged@partner.samsung.com>
 
         ASSERTION FAILED: object->style()->overflowX() == object->style()->overflowY()
index c6feb1f..4a8b713 100644 (file)
@@ -60,6 +60,7 @@ const GlobalObjectMethodTable JSDOMWindowBase::s_globalObjectMethodTable = { &sh
 
 JSDOMWindowBase::JSDOMWindowBase(VM& vm, Structure* structure, PassRefPtr<DOMWindow> window, JSDOMWindowShell* shell)
     : JSDOMGlobalObject(vm, structure, &shell->world(), &s_globalObjectMethodTable)
+    , m_windowCloseWatchpoints((window && window->frame()) ? IsWatched : IsInvalidated)
     , m_impl(window)
     , m_shell(shell)
 {
@@ -266,4 +267,23 @@ JSDOMWindow* toJSDOMWindow(JSValue value)
     return 0;
 }
 
+void JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(DOMWindow* window)
+{
+    JSC::VM& vm = JSDOMWindowBase::commonVM();
+    WebCoreJSClientData* clientData = static_cast<WebCoreJSClientData*>(vm.clientData);
+    Vector<Ref<DOMWrapperWorld>> wrapperWorlds;
+    clientData->getAllWorlds(wrapperWorlds);
+    for (unsigned i = 0; i < wrapperWorlds.size(); ++i) {
+        DOMObjectWrapperMap& wrappers = wrapperWorlds[i]->m_wrappers;
+        auto result = wrappers.find(window);
+        if (result == wrappers.end())
+            continue;
+        JSC::JSObject* wrapper = result->value.get();
+        if (!wrapper)
+            continue;
+        JSDOMWindowBase* jsWindow = JSC::jsCast<JSDOMWindowBase*>(wrapper);
+        jsWindow->m_windowCloseWatchpoints.fireAll();
+    }
+}
+
 } // namespace WebCore
index 55c248d..65002cb 100644 (file)
@@ -72,6 +72,10 @@ namespace WebCore {
         JSDOMWindowShell* shell() const;
 
         static JSC::VM& commonVM();
+        static void fireFrameClearedWatchpointsForWindow(DOMWindow*);
+
+    protected:
+        JSC::WatchpointSet m_windowCloseWatchpoints;
 
     private:
         RefPtr<DOMWindow> m_impl;
index 6feda6f..6e4087a 100644 (file)
@@ -132,7 +132,8 @@ bool JSDOMWindow::getOwnPropertySlot(JSObject* object, ExecState* exec, Property
         // not allowed. 
         slot.setUndefined();
         return true;
-    }
+    } else
+        slot.setWatchpointSet(thisObject->m_windowCloseWatchpoints);
 
     // We need to check for cross-domain access here without printing the generic warning message
     // because we always allow access to some function, just different ones depending whether access
index f206dd3..52facb6 100644 (file)
@@ -457,6 +457,7 @@ void DOMWindow::frameDestroyed()
     willDestroyDocumentInFrame();
     FrameDestructionObserver::frameDestroyed();
     resetDOMWindowProperties();
+    JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(this);
 }
 
 void DOMWindow::willDetachPage()