Bug 235658 – JSEventListener::replaceJSFunctionForAttributeListener() should not replace m_wrapper unconditionally

• Component: DOM
• Status: RESOLVED FIXED

[Alexey Shvayka]

JSEventListener::replaceJSFunctionForAttributeListener() should not replace m_wrapper unconditionally

[Alexey Shvayka]

Created attachment 450044 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 450044 [details]
Patch

r=me

[Alexey Shvayka]

Comment on attachment 450044 [details]
Patch

Thanks Yusuke!

[EWS]

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].

[Radar WebKit Bug Importer]

<rdar://problem/88098051>

[Geoffrey Garen]

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?

[Geoffrey Garen]

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

[Alexey Shvayka]

(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>

[Alexey Shvayka]

Reopening to attach new patch.

[Alexey Shvayka]

Created attachment 450081 [details]
Follow-up patch

[Alexey Shvayka]

Created attachment 450083 [details]
Follow-up patch

Fix comparison.

[Alexey Shvayka]

Created attachment 450184 [details]
Follow-up patch

Pass correct wrapper for Window-reflecting event handlers and improve ChangeLog.

[Alexey Shvayka]

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.

[EWS]

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].