JSEventListener::replaceJSFunctionForAttributeListener() should not replace m_wrapper unconditionally
Created attachment 450044 [details] Patch
Comment on attachment 450044 [details] Patch r=me
Comment on attachment 450044 [details] Patch Thanks Yusuke!
Committed r288648 (246456@main): <https://commits.webkit.org/246456@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450044 [details].
<rdar://problem/88098051>
Comment on attachment 450044 [details] Patch Why is it OK (or a memory improvement) to replace m_jsFunction unconditionally but replace m_wrapper conditionally? There seem to be some null checks of m_wrapper in JSEventListener. Wouldn't the choice not to update M-wrapper change their behavior? Write barrier changes the behavior of GC. Why is it OK to skip sometimes?
Comment on attachment 450044 [details] Patch If I understand the intent correctly, I *think* a better fix would be: m_jsFunction = Weak { function }; if (m_isInitialized) ASSERT(m_wrapper == wrapper); else { m_wrapper = Weak { wrapper }; m_isInitialized = true; } auto& vm = m_isolatedWorld->vm(); vm.writeBarrier(m_wrapper, m_jsFunction);
(In reply to Geoffrey Garen from comment #6) > Comment on attachment 450044 [details] > Patch > > Why is it OK (or a memory improvement) to replace m_jsFunction > unconditionally but replace m_wrapper conditionally? > > There seem to be some null checks of m_wrapper in JSEventListener. Wouldn't > the choice not to update M-wrapper change their behavior? As you've noted in https://bugs.webkit.org/show_bug.cgi?id=235658#c7, we hope (assert) the m_wrapper stays alive and unchanged. > Write barrier changes the behavior of GC. Why is it OK to skip sometimes? Write barrier is being set up only in case of JSLazyEventListener, where we create m_jsFunction from a string; that's not the case for replaceJSFunctionForAttributeListener(), so I guess the better fix would be just: m_jsFunction = Weak { function }; if (m_isInitialized) ASSERT(m_wrapper == wrapper); else { m_wrapper = Weak { wrapper }; m_isInitialized = true; } <no write barrier necessary>
Reopening to attach new patch.
Created attachment 450081 [details] Follow-up patch
Created attachment 450083 [details] Follow-up patch Fix comparison.
Created attachment 450184 [details] Follow-up patch Pass correct wrapper for Window-reflecting event handlers and improve ChangeLog.
Comment on attachment 450184 [details] Follow-up patch Geoff, thank you for valuable comments that led to discovering & fixing the incorrect behavior, which was probably user-facing.
Committed r288753 (246544@main): <https://commits.webkit.org/246544@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 450184 [details].