Attachment #392893 for bug #208642

View Details Raw Unified Return to Bug 208642

| Differences between



2020-03-07  Yusuke Suzuki  <ysuzuki@apple.com>

        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 NOBODY (OOPS!).

        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. We should remove such an condition once we have confident that there are no code using this hack.

        * 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-05  Sam Weinig  <weinig@apple.com>

        Move JavaScriptCore related feature defines from FeatureDefines.xcconfig to PlatformEnableCocoa.h

Lines 52-59 JSEventListener::JSEventListener(JSObject* function, JSObject* wrapper, bool isA a/Source/WebCore/bindings/js/JSEventListener.cpp_sec1
- a/Source/WebCore/bindings/js/JSEventListener.cpp -2 / +4 lines
52	, m_isolatedWorld(isolatedWorld)	52	, m_isolatedWorld(isolatedWorld)
53	{	53	{
54	if (wrapper) {	54	if (wrapper) {
55	JSC::Heap::heap(wrapper)->writeBarrier(wrapper, function);	55	JSC::VM& vm = m_isolatedWorld->vm();
56	m_jsFunction = JSC::Weak<JSC::JSObject>(function);	56	m_jsFunction = JSC::Weak<JSC::JSObject>(function);
		57	vm.heap.writeBarrier(wrapper, function);
		58	m_isInitialized = true;
57	} else	59	} else
58	ASSERT(!function);	60	ASSERT(!function);
59	}	61	}
Lines 80-86 JSObject* JSEventListener::initializeJSFunction(ScriptExecutionContext&) const a/Source/WebCore/bindings/js/JSEventListener.cpp_sec2
80		82
81	void JSEventListener::visitJSFunction(SlotVisitor& visitor)	83	void JSEventListener::visitJSFunction(SlotVisitor& visitor)
82	{	84	{
83	// If m_wrapper is null, then m_jsFunction is zombied, and should never be accessed.	85	// If m_wrapper is null, we are not keeping m_jsFunction alive.
84	if (!m_wrapper)	86	if (!m_wrapper)
85	return;	87	return;
86		88



    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(); }

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;

{
    // 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(vm, m_wrapper.get());

    if (!m_isInitialized) {
        ASSERT(!m_jsFunction);
        auto* function = initializeJSFunction(scriptExecutionContext);
        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;
        }
    }

    // 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)
    if (!m_wrapper)
        return nullptr;

    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();
}

Lines 173-179 JSObject* JSLazyEventListener::initializeJSFunction(ScriptExecutionContext& exec a/Source/WebCore/bindings/js/JSLazyEventListener.cpp_sec1
- a/Source/WebCore/bindings/js/JSLazyEventListener.cpp -1 / +1 lines
173	if (!wrapper()) {	173	if (!wrapper()) {
174	// Ensure that 'node' has a JavaScript wrapper to mark the event listener we're creating.	174	// Ensure that 'node' has a JavaScript wrapper to mark the event listener we're creating.
175	// FIXME: Should pass the global object associated with the node	175	// FIXME: Should pass the global object associated with the node
176	setWrapper(vm, asObject(toJS(lexicalGlobalObject, globalObject, *m_originalNode)));	176	setWrapperWhenInitializingJSFunction(vm, asObject(toJS(lexicalGlobalObject, globalObject, *m_originalNode)));
177	}	177	}
178		178
179	// Add the event's home element to the scope	179	// Add the event's home element to the scope

Return to Bug 208642

Lines 52-58 class JSEventListener : public EventListener { a/Source/WebCore/bindings/js/JSEventListener.h_sec1
- a/Source/WebCore/bindings/js/JSEventListener.h -16 / +23 lines
52	DOMWrapperWorld& isolatedWorld() const { return m_isolatedWorld; }	52	DOMWrapperWorld& isolatedWorld() const { return m_isolatedWorld; }
53		53
54	JSC::JSObject* wrapper() const { return m_wrapper.get(); }	54	JSC::JSObject* wrapper() const { return m_wrapper.get(); }
55	void setWrapper(JSC::VM&, JSC::JSObject* wrapper) const { m_wrapper = JSC::Weak<JSC::JSObject>(wrapper); }
56		55
57	virtual String sourceURL() const { return String(); }	56	virtual String sourceURL() const { return String(); }
58	virtual TextPosition sourcePosition() const { return TextPosition(); }	57	virtual TextPosition sourcePosition() const { return TextPosition(); }
Lines 66-75 class JSEventListener : public EventListener { a/Source/WebCore/bindings/js/JSEventListener.h_sec2
66	protected:	65	protected:
67	JSEventListener(JSC::JSObject* function, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld&);	66	JSEventListener(JSC::JSObject* function, JSC::JSObject* wrapper, bool isAttribute, DOMWrapperWorld&);
68	void handleEvent(ScriptExecutionContext&, Event&) override;	67	void handleEvent(ScriptExecutionContext&, Event&) override;
		68	void setWrapperWhenInitializingJSFunction(JSC::VM&, JSC::JSObject* wrapper) const { m_wrapper = JSC::Weak<JSC::JSObject>(wrapper); }
69		69
70	private:	70	private:
71	mutable JSC::Weak<JSC::JSObject> m_jsFunction;	71	mutable JSC::Weak<JSC::JSObject> m_jsFunction;
72	mutable JSC::Weak<JSC::JSObject> m_wrapper;	72	mutable JSC::Weak<JSC::JSObject> m_wrapper;
		73	mutable bool m_isInitialized { false };
73		74
74	bool m_isAttribute;	75	bool m_isAttribute;
75	Ref<DOMWrapperWorld> m_isolatedWorld;	76	Ref<DOMWrapperWorld> m_isolatedWorld;
Lines 95-122 inline JSC::JSObject* JSEventListener::jsFunction(ScriptExecutionContext& script a/Source/WebCore/bindings/js/JSEventListener.h_sec3
95	{	96	{
96	// initializeJSFunction can trigger code that deletes this event listener	97	// initializeJSFunction can trigger code that deletes this event listener
97	// before we're done. It should always return null in this case.	98	// before we're done. It should always return null in this case.
		99	JSC::VM& vm = m_isolatedWorld->vm();
98	auto protect = makeRef(const_cast<JSEventListener&>(*this));	100	auto protect = makeRef(const_cast<JSEventListener&>(*this));
99	JSC::Strong<JSC::JSObject> wrapper(m_isolatedWorld->vm(), m_wrapper.get());	101	JSC::Strong<JSC::JSObject> wrapper(vm, m_wrapper.get());
100		102
101	if (!m_jsFunction) {	103	if (!m_isInitialized) {
		104	ASSERT(!m_jsFunction);
102	auto* function = initializeJSFunction(scriptExecutionContext);	105	auto* function = initializeJSFunction(scriptExecutionContext);
103	if (auto* wrapper = m_wrapper.get())	106	if (function) {
104	JSC::Heap::heap(wrapper)->writeBarrier(wrapper, function);	107	m_jsFunction = JSC::Weak<JSC::JSObject>(function);
105	m_jsFunction = JSC::Weak<JSC::JSObject>(function);	108	// When JSFunction is initialized, initializeJSFunction must ensure that m_wrapper should be initialized too.
		109	ASSERT(m_wrapper);
		110	vm.heap.writeBarrier(m_wrapper.get(), function);
		111	m_isInitialized = true;
		112	}
106	}	113	}
107		114
108	// Verify that we have a valid wrapper protecting our function from	115	// m_wrapper and m_jsFunction are Weak<>. nullptr of these fields do not mean that this event-listener is not initialized yet.
109	// garbage collection. That is except for when we're not in the normal	116	// If this is initialized once, m_isInitialized should be true, and then m_wrapper and m_jsFunction must be alive. m_wrapper's
110	// world and can have zombie m_jsFunctions.	117	// liveness should be kept correctly by using ActiveDOMObject, output-constraints, etc. And m_jsFunction must be alive if m_wrapper
111	ASSERT(!m_isolatedWorld->isNormal() \|\| m_wrapper \|\| !m_jsFunction);	118	// is alive since JSEventListener marks m_jsFunction in JSEventListener::visitJSFunction if m_wrapper is alive.
112		119	// If the event-listener is not initialized yet, we should skip invoking this event-listener.
113	// If m_wrapper is null, then m_jsFunction is zombied, and should never be accessed.	120	if (!m_isInitialized)
114	if (!m_wrapper)
115	return nullptr;	121	return nullptr;
116		122
117	// Try to verify that m_jsFunction wasn't recycled. (Not exact, since an	123	ASSERT(m_wrapper);
118	// event listener can be almost anything, but this makes test-writing easier).	124	ASSERT(m_jsFunction);
119	ASSERT(!m_jsFunction \|\| static_cast<JSC::JSCell*>(m_jsFunction.get())->isObject());	125	// 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.
		126	ASSERT(static_cast<JSC::JSCell*>(m_jsFunction.get())->isObject());
120		127
121	return m_jsFunction.get();	128	return m_jsFunction.get();
122	}	129	}

Lines 1-3 a/Source/WebCore/ChangeLog_sec1
- a/Source/WebCore/ChangeLog +42 lines
		1	2020-03-07 Yusuke Suzuki <ysuzuki@apple.com>
		2
		3	REGRESSION: (r257905) [ Mac wk2 Debug ] ASSERTION FAILED: !m_isolatedWorld->isNormal() \|\| m_wrapper \|\| !m_jsFunction
		4	https://bugs.webkit.org/show_bug.cgi?id=208642
		5
		6	Reviewed by NOBODY (OOPS!).
		7
		8	This patch fixes stale assertions and comments in JSEventListener.h, which has various problems.
		9
		10	1. This assertion is saying, "If m_wrapper is dead, m_jsFunction must be dead". This is wrong. Given that we have conservative
		11	GC, JSC never guarantees such a condition. Even if m_wrapper is dead, m_jsFunction can be alive by various reasons: conservative
		12	GC finds it, user code stores this function somewhere reachable from the root, etc.
		13	The reason why this wrong assertion exists here is because the JSEventListener code and assertion assume that m_jsFunction is nullptr
		14	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
		15	is not retained. This `!m_jsFunction` check mixes "it is not initialized" and "it is already initialized but collected".
		16	The correct assertion should be checking `m_wrapper` and `m_jsFunction` are alive (not checking deadness, which is not guaranteed) if
		17	the event-listener is once initialized. This patch adds m_isInitialized member to track this status separately from `m_wrapper` and
		18	`m_jsFunction`.
		19	2. JSEventListener::jsFunction has `if (!m_jsFunction)` condition. But this is not correct. This can revive JSFunction if it is collected
		20	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
		21	is initialized, keeping m_wrapper and m_jsFunction alive is the responsibility of JSEventListener's owner.
		22	3. The comments about "zombie m_jsFunctions" is wrong. We are using JSC::Weak<>. So if the object gets collected, it returns
		23	nullptr, not getting a zombie pointer.
		24	4. We are emitting write-barrier in a wrong order. In the heavily stressed scenario, it is possible that concurrent marking
		25	scans JSEventListener just after we emit the write-barrier, and this marking misses the assigned value. We must emit
		26	a write-barrier after the assignment. If the write-barrier code is written after the assignment, it correctly offers memory
		27	fence to ensure this ordering.
		28	5. We also remove "world is not normal, anything is allowed" assertion. The assertion is allowing non-normal world to get dead m_wrapper.
		29	But skipping event handlers only in non-normal world does not make sense. And it is originally added as a hack to avoid assertions
		30	caused by non-normal world. We should remove such an condition once we have confident that there are no code using this hack.
		31
		32	* bindings/js/JSEventListener.cpp:
		33	(WebCore::JSEventListener::JSEventListener):
		34	(WebCore::JSEventListener::visitJSFunction):
		35	* bindings/js/JSEventListener.h:
		36	(WebCore::JSEventListener::wrapper const):
		37	(WebCore::JSEventListener::setWrapperWhenInitializingJSFunction const):
		38	(WebCore::JSEventListener::jsFunction const):
		39	(WebCore::JSEventListener::setWrapper const): Deleted.
		40	* bindings/js/JSLazyEventListener.cpp:
		41	(WebCore::JSLazyEventListener::initializeJSFunction const):
		42
1	2020-03-05 Sam Weinig <weinig@apple.com>	43	2020-03-05 Sam Weinig <weinig@apple.com>
2		44
3	Move JavaScriptCore related feature defines from FeatureDefines.xcconfig to PlatformEnableCocoa.h	45	Move JavaScriptCore related feature defines from FeatureDefines.xcconfig to PlatformEnableCocoa.h