- Revision
- 258189
- Author
- [email protected]
- Date
- 2020-03-10 01:32:56 -0700 (Tue, 10 Mar 2020)
Log Message
REGRESSION: (r257905) [ Mac wk2 Debug ] ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction
https://bugs.webkit.org/show_bug.cgi?id=208642
Reviewed by Darin Adler.
This patch fixes stale assertions and comments in JSEventListener.h, which has various problems.
1. This assertion is saying, "If m_wrapper is dead, m_jsFunction must be dead". This is wrong. Given that we have conservative
GC, JSC never guarantees such a condition. Even if m_wrapper is dead, m_jsFunction can be alive by various reasons: conservative
GC finds it, user code stores this function somewhere reachable from the root, etc.
The reason why this wrong assertion exists here is because the JSEventListener code and assertion assume that m_jsFunction is nullptr
when it is not initialized, and once it is initialized, it should be non nullptr. This is wrong because Weak<> can collect it if it
is not retained. This `!m_jsFunction` check mixes "it is not initialized" and "it is already initialized but collected".
The correct assertion should be checking `m_wrapper` and `m_jsFunction` are alive (not checking deadness, which is not guaranteed) if
the event-listener is once initialized. This patch adds m_isInitialized member to track this status separately from `m_wrapper` and
`m_jsFunction`.
2. JSEventListener::jsFunction has `if (!m_jsFunction)` condition. But this is not correct. This can revive JSFunction if it is collected
because m_wrapper is gone or some way, but this is not expected behavior. The correct way is checking `m_isInitialized`. Once the event-listener
is initialized, keeping m_wrapper and m_jsFunction alive is the responsibility of JSEventListener's owner.
3. The comments about "zombie m_jsFunctions" is wrong. We are using JSC::Weak<>. So if the object gets collected, it returns
nullptr, not getting a zombie pointer.
4. We are emitting write-barrier in a wrong order. In the heavily stressed scenario, it is possible that concurrent marking
scans JSEventListener just after we emit the write-barrier, and this marking misses the assigned value. We must emit
a write-barrier after the assignment. If the write-barrier code is written after the assignment, it correctly offers memory
fence to ensure this ordering.
5. We also remove "world is not normal, anything is allowed" assertion. The assertion is allowing non-normal world to get dead m_wrapper.
But skipping event handlers only in non-normal world does not make sense. And it is originally added as a hack to avoid assertions
caused by non-normal world.
While we are not sure which test is causing, it seems that we found a real bug by fixing this assertion[1].
[1]: https://bugs.webkit.org/show_bug.cgi?id=208798
* bindings/js/JSEventListener.cpp:
(WebCore::JSEventListener::JSEventListener):
(WebCore::JSEventListener::visitJSFunction):
* bindings/js/JSEventListener.h:
(WebCore::JSEventListener::wrapper const):
(WebCore::JSEventListener::setWrapperWhenInitializingJSFunction const):
(WebCore::JSEventListener::jsFunction const):
(WebCore::JSEventListener::setWrapper const): Deleted.
* bindings/js/JSLazyEventListener.cpp:
(WebCore::JSLazyEventListener::initializeJSFunction const):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (258188 => 258189)
--- trunk/Source/WebCore/ChangeLog 2020-03-10 08:28:46 UTC (rev 258188)
+++ trunk/Source/WebCore/ChangeLog 2020-03-10 08:32:56 UTC (rev 258189)
@@ -1,3 +1,49 @@
+2020-03-09 Yusuke Suzuki <[email protected]>
+
+ REGRESSION: (r257905) [ Mac wk2 Debug ] ASSERTION FAILED: !m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction
+ https://bugs.webkit.org/show_bug.cgi?id=208642
+
+ Reviewed by Darin Adler.
+
+ This patch fixes stale assertions and comments in JSEventListener.h, which has various problems.
+
+ 1. This assertion is saying, "If m_wrapper is dead, m_jsFunction must be dead". This is wrong. Given that we have conservative
+ GC, JSC never guarantees such a condition. Even if m_wrapper is dead, m_jsFunction can be alive by various reasons: conservative
+ GC finds it, user code stores this function somewhere reachable from the root, etc.
+ The reason why this wrong assertion exists here is because the JSEventListener code and assertion assume that m_jsFunction is nullptr
+ when it is not initialized, and once it is initialized, it should be non nullptr. This is wrong because Weak<> can collect it if it
+ is not retained. This `!m_jsFunction` check mixes "it is not initialized" and "it is already initialized but collected".
+ The correct assertion should be checking `m_wrapper` and `m_jsFunction` are alive (not checking deadness, which is not guaranteed) if
+ the event-listener is once initialized. This patch adds m_isInitialized member to track this status separately from `m_wrapper` and
+ `m_jsFunction`.
+ 2. JSEventListener::jsFunction has `if (!m_jsFunction)` condition. But this is not correct. This can revive JSFunction if it is collected
+ because m_wrapper is gone or some way, but this is not expected behavior. The correct way is checking `m_isInitialized`. Once the event-listener
+ is initialized, keeping m_wrapper and m_jsFunction alive is the responsibility of JSEventListener's owner.
+ 3. The comments about "zombie m_jsFunctions" is wrong. We are using JSC::Weak<>. So if the object gets collected, it returns
+ nullptr, not getting a zombie pointer.
+ 4. We are emitting write-barrier in a wrong order. In the heavily stressed scenario, it is possible that concurrent marking
+ scans JSEventListener just after we emit the write-barrier, and this marking misses the assigned value. We must emit
+ a write-barrier after the assignment. If the write-barrier code is written after the assignment, it correctly offers memory
+ fence to ensure this ordering.
+ 5. We also remove "world is not normal, anything is allowed" assertion. The assertion is allowing non-normal world to get dead m_wrapper.
+ But skipping event handlers only in non-normal world does not make sense. And it is originally added as a hack to avoid assertions
+ caused by non-normal world.
+
+ While we are not sure which test is causing, it seems that we found a real bug by fixing this assertion[1].
+
+ [1]: https://bugs.webkit.org/show_bug.cgi?id=208798
+
+ * bindings/js/JSEventListener.cpp:
+ (WebCore::JSEventListener::JSEventListener):
+ (WebCore::JSEventListener::visitJSFunction):
+ * bindings/js/JSEventListener.h:
+ (WebCore::JSEventListener::wrapper const):
+ (WebCore::JSEventListener::setWrapperWhenInitializingJSFunction const):
+ (WebCore::JSEventListener::jsFunction const):
+ (WebCore::JSEventListener::setWrapper const): Deleted.
+ * bindings/js/JSLazyEventListener.cpp:
+ (WebCore::JSLazyEventListener::initializeJSFunction const):
+
2020-03-09 Zalan Bujtas <[email protected]>
[LayoutTests] Do not expose didAddHorizontal/VerticalScrollbar and willRemoveHorizontal/VerticalScrollbar
Modified: trunk/Source/WebCore/bindings/js/JSEventListener.cpp (258188 => 258189)
--- trunk/Source/WebCore/bindings/js/JSEventListener.cpp 2020-03-10 08:28:46 UTC (rev 258188)
+++ trunk/Source/WebCore/bindings/js/JSEventListener.cpp 2020-03-10 08:32:56 UTC (rev 258189)
@@ -51,11 +51,13 @@
, m_isAttribute(isAttribute)
, m_isolatedWorld(isolatedWorld)
{
- if (wrapper) {
- JSC::Heap::heap(wrapper)->writeBarrier(wrapper, function);
+ if (function) {
+ ASSERT(wrapper);
+ JSC::VM& vm = m_isolatedWorld->vm();
m_jsFunction = JSC::Weak<JSC::JSObject>(function);
- } else
- ASSERT(!function);
+ vm.heap.writeBarrier(wrapper, function);
+ m_isInitialized = true;
+ }
}
JSEventListener::~JSEventListener() = default;
@@ -80,7 +82,7 @@
void JSEventListener::visitJSFunction(SlotVisitor& visitor)
{
- // If m_wrapper is null, then m_jsFunction is zombied, and should never be accessed.
+ // If m_wrapper is null, we are not keeping m_jsFunction alive.
if (!m_wrapper)
return;
Modified: trunk/Source/WebCore/bindings/js/JSEventListener.h (258188 => 258189)
--- trunk/Source/WebCore/bindings/js/JSEventListener.h 2020-03-10 08:28:46 UTC (rev 258188)
+++ trunk/Source/WebCore/bindings/js/JSEventListener.h 2020-03-10 08:32:56 UTC (rev 258189)
@@ -52,7 +52,6 @@
DOMWrapperWorld& isolatedWorld() const { return m_isolatedWorld; }
JSC::JSObject* wrapper() const { return m_wrapper.get(); }
- void setWrapper(JSC::VM&, JSC::JSObject* wrapper) const { m_wrapper = JSC::Weak<JSC::JSObject>(wrapper); }
virtual String sourceURL() const { return String(); }
virtual TextPosition sourcePosition() const { return TextPosition(); }
@@ -66,10 +65,12 @@
protected:
JSEventListener(JSC::JSObject* function, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld&);
void handleEvent(ScriptExecutionContext&, Event&) override;
+ void setWrapperWhenInitializingJSFunction(JSC::VM&, JSC::JSObject* wrapper) const { m_wrapper = JSC::Weak<JSC::JSObject>(wrapper); }
private:
mutable JSC::Weak<JSC::JSObject> m_jsFunction;
mutable JSC::Weak<JSC::JSObject> m_wrapper;
+ mutable bool m_isInitialized { false };
bool m_isAttribute;
Ref<DOMWrapperWorld> m_isolatedWorld;
@@ -95,28 +96,34 @@
{
// initializeJSFunction can trigger code that deletes this event listener
// before we're done. It should always return null in this case.
+ JSC::VM& vm = m_isolatedWorld->vm();
auto protect = makeRef(const_cast<JSEventListener&>(*this));
- JSC::Strong<JSC::JSObject> wrapper(m_isolatedWorld->vm(), m_wrapper.get());
+ JSC::Strong<JSC::JSObject> wrapper(vm, m_wrapper.get());
- if (!m_jsFunction) {
+ if (!m_isInitialized) {
+ ASSERT(!m_jsFunction);
auto* function = initializeJSFunction(scriptExecutionContext);
- if (auto* wrapper = m_wrapper.get())
- JSC::Heap::heap(wrapper)->writeBarrier(wrapper, function);
- m_jsFunction = JSC::Weak<JSC::JSObject>(function);
+ if (function) {
+ m_jsFunction = JSC::Weak<JSC::JSObject>(function);
+ // When JSFunction is initialized, initializeJSFunction must ensure that m_wrapper should be initialized too.
+ ASSERT(m_wrapper);
+ vm.heap.writeBarrier(m_wrapper.get(), function);
+ m_isInitialized = true;
+ }
}
- // Verify that we have a valid wrapper protecting our function from
- // garbage collection. That is except for when we're not in the normal
- // world and can have zombie m_jsFunctions.
- ASSERT(!m_isolatedWorld->isNormal() || m_wrapper || !m_jsFunction);
-
- // If m_wrapper is null, then m_jsFunction is zombied, and should never be accessed.
- if (!m_wrapper)
+ // m_wrapper and m_jsFunction are Weak<>. nullptr of these fields do not mean that this event-listener is not initialized yet.
+ // If this is initialized once, m_isInitialized should be true, and then m_wrapper and m_jsFunction must be alive. m_wrapper's
+ // liveness should be kept correctly by using ActiveDOMObject, output-constraints, etc. And m_jsFunction must be alive if m_wrapper
+ // is alive since JSEventListener marks m_jsFunction in JSEventListener::visitJSFunction if m_wrapper is alive.
+ // If the event-listener is not initialized yet, we should skip invoking this event-listener.
+ if (!m_isInitialized)
return nullptr;
- // Try to verify that m_jsFunction wasn't recycled. (Not exact, since an
- // event listener can be almost anything, but this makes test-writing easier).
- ASSERT(!m_jsFunction || static_cast<JSC::JSCell*>(m_jsFunction.get())->isObject());
+ ASSERT(m_wrapper);
+ ASSERT(m_jsFunction);
+ // Ensure m_jsFunction is live JSObject as a quick sanity check (while it is already ensured by Weak<>). If this fails, this is possibly JSC GC side's bug.
+ ASSERT(static_cast<JSC::JSCell*>(m_jsFunction.get())->isObject());
return m_jsFunction.get();
}
Modified: trunk/Source/WebCore/bindings/js/JSLazyEventListener.cpp (258188 => 258189)
--- trunk/Source/WebCore/bindings/js/JSLazyEventListener.cpp 2020-03-10 08:28:46 UTC (rev 258188)
+++ trunk/Source/WebCore/bindings/js/JSLazyEventListener.cpp 2020-03-10 08:32:56 UTC (rev 258189)
@@ -173,7 +173,7 @@
if (!wrapper()) {
// Ensure that 'node' has a _javascript_ wrapper to mark the event listener we're creating.
// FIXME: Should pass the global object associated with the node
- setWrapper(vm, asObject(toJS(lexicalGlobalObject, globalObject, *m_originalNode)));
+ setWrapperWhenInitializingJSFunction(vm, asObject(toJS(lexicalGlobalObject, globalObject, *m_originalNode)));
}
// Add the event's home element to the scope
