JSDOMWindowBase m_windowCloseWatchpoints must be Ref<>
authorysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 May 2020 02:15:33 +0000 (02:15 +0000)
committerysuzuki@apple.com <ysuzuki@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 14 May 2020 02:15:33 +0000 (02:15 +0000)
https://bugs.webkit.org/show_bug.cgi?id=211844

Reviewed by Mark Lam.

Source/JavaScriptCore:

* bytecode/Watchpoint.cpp:
(JSC::InlineWatchpointSet::inflateSlow):
* bytecode/Watchpoint.h:
* runtime/JSGlobalObject.cpp:
(JSC::JSGlobalObject::JSGlobalObject):
* runtime/Structure.cpp:
(JSC::Structure::ensurePropertyReplacementWatchpointSet):
* runtime/SymbolTable.cpp:
(JSC::SymbolTableEntry::prepareToWatch):
* runtime/VM.cpp:
(JSC::VM::ensureWatchpointSetForImpureProperty):

Source/WebCore:

JSDOMWindowBase::m_windowCloseWatchpoints is WatchpointSet, not Ref<WatchpointSet>. And it is passed to JSC IC layer via PropertySlot::setWatchpoint(...).
And ProxyableAccessCase can hold it as `RefPtr<WatchpointSet> m_additionalSet;`, this is wrong.

This patch hides WatchpointSet constructor behind `protected` to disallow non Ref<> WatchpointSet construction. We made it `protected` since InferredValueWatchpointSet
inherits WatchpointSet and uses this constructor.

Possibly, this does not matter: ProxyableAccessCase keeps Structure, which points to JSDOMWindowBase. So, it would not happen that ProxyableAccessCase has a wrong pointer
to WatchpointSet since JSDOMWindowBase is kept alive anyway. But avoid using RefCounted objects without RefCount allocation is better since this can cause bugs easily.

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

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

Source/JavaScriptCore/ChangeLog
Source/JavaScriptCore/bytecode/Watchpoint.cpp
Source/JavaScriptCore/bytecode/Watchpoint.h
Source/JavaScriptCore/runtime/JSGlobalObject.cpp
Source/JavaScriptCore/runtime/Structure.cpp
Source/JavaScriptCore/runtime/SymbolTable.cpp
Source/JavaScriptCore/runtime/VM.cpp
Source/WebCore/ChangeLog
Source/WebCore/bindings/js/JSDOMWindowBase.cpp
Source/WebCore/bindings/js/JSDOMWindowBase.h
Source/WebCore/bindings/js/JSDOMWindowCustom.cpp

index 955cf4b..73ba3b7 100644 (file)
@@ -1,3 +1,22 @@
+2020-05-13  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        JSDOMWindowBase m_windowCloseWatchpoints must be Ref<>
+        https://bugs.webkit.org/show_bug.cgi?id=211844
+
+        Reviewed by Mark Lam.
+
+        * bytecode/Watchpoint.cpp:
+        (JSC::InlineWatchpointSet::inflateSlow):
+        * bytecode/Watchpoint.h:
+        * runtime/JSGlobalObject.cpp:
+        (JSC::JSGlobalObject::JSGlobalObject):
+        * runtime/Structure.cpp:
+        (JSC::Structure::ensurePropertyReplacementWatchpointSet):
+        * runtime/SymbolTable.cpp:
+        (JSC::SymbolTableEntry::prepareToWatch):
+        * runtime/VM.cpp:
+        (JSC::VM::ensureWatchpointSetForImpureProperty):
+
 2020-05-13  Caio Lima  <ticaiolima@gmail.com>
 
         Making 32-bits JIT build without Unified Build system
index c20de9b..613212f 100644 (file)
@@ -183,7 +183,7 @@ WatchpointSet* InlineWatchpointSet::inflateSlow()
 {
     ASSERT(isThin());
     ASSERT(!isCompilationThread());
-    WatchpointSet* fat = adoptRef(new WatchpointSet(decodeState(m_data))).leakRef();
+    WatchpointSet* fat = &WatchpointSet::create(decodeState(m_data)).leakRef();
     WTF::storeStoreFence();
     m_data = bitwise_cast<uintptr_t>(fat);
     return fat;
index 77c6dfc..6c057fd 100644 (file)
@@ -186,8 +186,6 @@ class WatchpointSet : public ThreadSafeRefCounted<WatchpointSet> {
     friend class LLIntOffsetsExtractor;
     friend class DeferredWatchpointFire;
 public:
-    JS_EXPORT_PRIVATE WatchpointSet(WatchpointState);
-    
     // FIXME: In many cases, it would be amazing if this *did* fire the watchpoints. I suspect that
     // this might be hard to get right, but still, it might be awesome.
     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.
@@ -295,6 +293,9 @@ public:
     JS_EXPORT_PRIVATE void fireAllSlow(VM&, DeferredWatchpointFire* deferredWatchpoints); // Ditto.
     JS_EXPORT_PRIVATE void fireAllSlow(VM&, const char* reason); // Ditto.
     
+protected:
+    JS_EXPORT_PRIVATE WatchpointSet(WatchpointState);
+
 private:
     void fireAllWatchpoints(VM&, const FireDetail&);
     void take(WatchpointSet* other);
index a34698f..e7a8a9e 100644 (file)
@@ -430,9 +430,9 @@ JSGlobalObject::JSGlobalObject(VM& vm, Structure* structure, const GlobalObjectM
     : Base(vm, structure, nullptr)
     , m_vm(&vm)
     , m_linkTimeConstants(numberOfLinkTimeConstants)
-    , m_masqueradesAsUndefinedWatchpoint(adoptRef(new WatchpointSet(IsWatched)))
-    , m_havingABadTimeWatchpoint(adoptRef(new WatchpointSet(IsWatched)))
-    , m_varInjectionWatchpoint(adoptRef(new WatchpointSet(IsWatched)))
+    , m_masqueradesAsUndefinedWatchpoint(WatchpointSet::create(IsWatched))
+    , m_havingABadTimeWatchpoint(WatchpointSet::create(IsWatched))
+    , m_varInjectionWatchpoint(WatchpointSet::create(IsWatched))
     , m_weakRandom(Options::forceWeakRandomSeed() ? Options::forcedWeakRandomSeed() : static_cast<unsigned>(randomNumber() * (std::numeric_limits<unsigned>::max() + 1.0)))
     , m_arrayIteratorProtocolWatchpointSet(IsWatched)
     , m_mapIteratorProtocolWatchpointSet(IsWatched)
index c62afce..4fc25b5 100644 (file)
@@ -953,7 +953,7 @@ WatchpointSet* Structure::ensurePropertyReplacementWatchpointSet(VM& vm, Propert
     }
     auto result = rareData->m_replacementWatchpointSets->add(offset, nullptr);
     if (result.isNewEntry)
-        result.iterator->value = adoptRef(new WatchpointSet(IsWatched));
+        result.iterator->value = WatchpointSet::create(IsWatched);
     return result.iterator->value.get();
 }
 
index bc49dc9..f0f9456 100644 (file)
@@ -69,7 +69,7 @@ void SymbolTableEntry::prepareToWatch()
     FatEntry* entry = inflate();
     if (entry->m_watchpoints)
         return;
-    entry->m_watchpoints = adoptRef(new WatchpointSet(ClearWatchpoint));
+    entry->m_watchpoints = WatchpointSet::create(ClearWatchpoint);
 }
 
 SymbolTableEntry::FatEntry* SymbolTableEntry::inflateSlow()
index 980b781..b7be032 100644 (file)
@@ -1163,7 +1163,7 @@ WatchpointSet* VM::ensureWatchpointSetForImpureProperty(UniquedStringImpl* prope
 {
     auto result = m_impurePropertyWatchpointSets.add(propertyName, nullptr);
     if (result.isNewEntry)
-        result.iterator->value = adoptRef(new WatchpointSet(IsWatched));
+        result.iterator->value = WatchpointSet::create(IsWatched);
     return result.iterator->value.get();
 }
 
index 54961ff..9b0ed23 100644 (file)
@@ -1,3 +1,26 @@
+2020-05-13  Yusuke Suzuki  <ysuzuki@apple.com>
+
+        JSDOMWindowBase m_windowCloseWatchpoints must be Ref<>
+        https://bugs.webkit.org/show_bug.cgi?id=211844
+
+        Reviewed by Mark Lam.
+
+        JSDOMWindowBase::m_windowCloseWatchpoints is WatchpointSet, not Ref<WatchpointSet>. And it is passed to JSC IC layer via PropertySlot::setWatchpoint(...).
+        And ProxyableAccessCase can hold it as `RefPtr<WatchpointSet> m_additionalSet;`, this is wrong.
+
+        This patch hides WatchpointSet constructor behind `protected` to disallow non Ref<> WatchpointSet construction. We made it `protected` since InferredValueWatchpointSet
+        inherits WatchpointSet and uses this constructor.
+
+        Possibly, this does not matter: ProxyableAccessCase keeps Structure, which points to JSDOMWindowBase. So, it would not happen that ProxyableAccessCase has a wrong pointer
+        to WatchpointSet since JSDOMWindowBase is kept alive anyway. But avoid using RefCounted objects without RefCount allocation is better since this can cause bugs easily.
+
+        * bindings/js/JSDOMWindowBase.cpp:
+        (WebCore::JSDOMWindowBase::JSDOMWindowBase):
+        (WebCore::JSDOMWindowBase::fireFrameClearedWatchpointsForWindow):
+        * bindings/js/JSDOMWindowBase.h:
+        * bindings/js/JSDOMWindowCustom.cpp:
+        (WebCore::JSDOMWindow::getOwnPropertySlot):
+
 2020-05-13  Jack Lee  <shihchieh_lee@apple.com>
 
         Nullptr crash in InsertParagraphSeparatorCommand::doApply when the canonical position is uneditable
index 053d131..48fd006 100644 (file)
@@ -90,7 +90,7 @@ const GlobalObjectMethodTable JSDOMWindowBase::s_globalObjectMethodTable = {
 
 JSDOMWindowBase::JSDOMWindowBase(VM& vm, Structure* structure, RefPtr<DOMWindow>&& window, JSWindowProxy* proxy)
     : JSDOMGlobalObject(vm, structure, proxy->world(), &s_globalObjectMethodTable)
-    , m_windowCloseWatchpoints((window && window->frame()) ? IsWatched : IsInvalidated)
+    , m_windowCloseWatchpoints(WatchpointSet::create((window && window->frame()) ? IsWatched : IsInvalidated))
     , m_wrapped(WTFMove(window))
     , m_proxy(proxy)
 {
@@ -296,7 +296,7 @@ void JSDOMWindowBase::fireFrameClearedWatchpointsForWindow(DOMWindow* window)
         if (!wrapper)
             continue;
         JSDOMWindowBase* jsWindow = JSC::jsCast<JSDOMWindowBase*>(wrapper);
-        jsWindow->m_windowCloseWatchpoints.fireAll(vm, "Frame cleared");
+        jsWindow->m_windowCloseWatchpoints->fireAll(vm, "Frame cleared");
     }
 }
 
index 1d7762c..ec6ddc1 100644 (file)
@@ -89,7 +89,7 @@ protected:
     JSDOMWindowBase(JSC::VM&, JSC::Structure*, RefPtr<DOMWindow>&&, JSWindowProxy*);
     void finishCreation(JSC::VM&, JSWindowProxy*);
 
-    JSC::WatchpointSet m_windowCloseWatchpoints;
+    Ref<JSC::WatchpointSet> m_windowCloseWatchpoints;
 
 private:
     using ResponseCallback = WTF::Function<void(const char*, size_t)>;
index 4665bce..01dcb6c 100644 (file)
@@ -194,7 +194,7 @@ bool JSDOMWindow::getOwnPropertySlot(JSObject* object, JSGlobalObject* lexicalGl
 
     // FIXME: this needs more explanation.
     // (Particularly, is it correct that this exists here but not in getOwnPropertySlotByIndex?)
-    slot.setWatchpointSet(thisObject->m_windowCloseWatchpoints);
+    slot.setWatchpointSet(thisObject->m_windowCloseWatchpoints.get());
 
     // (2) Regular own properties.
     PropertySlot slotCopy = slot;