[webkit-reviews] review denied: [Bug 101989] JSC: JSEventListener's m_jsFunction should be a weak ref : [Attachment 173801] 3rd time's the charm.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 101989: JSC: JSEventListener's m_jsFunction should be a weak ref
https://bugs.webkit.org/show_bug.cgi?id=101989

Attachment 173801: 3rd time's the charm.
https://bugs.webkit.org/attachment.cgi?id=173801&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=173801&action=review


You mentioned that you saw problems on websites with an earlier version of this
patch. Does this patch fix those problems?

> Source/JavaScriptCore/heap/SlotVisitor.h:61
> +    void appendUnbarrieredWeakRef(Weak<T>*);

We don't usually use the abbreviation "Ref" outside of CoreFoundation classes.
How about just calling this "appendUnbarrieredWeak" or "appendUnbarriered"?

> Source/JavaScriptCore/heap/SlotVisitorInlines.h:73
> +    if (weakRef && *weakRef)

I don't think weakRef can ever be NULL here.

Instead of the *weakRef test, I think you either need a call to .get() or a
call to operator!(). It's an error to use operator*() after a weak pointer has
been collected.

> Source/JavaScriptCore/heap/Weak.h:160
> +template<typename T> inline JSValue Weak<T>::referee()
> +{
> +    return m_impl ? m_impl->jsValue() : JSValue();
> +}

Why do we need this function, when we already have operator*() and .get()?

> Source/WebCore/bindings/js/JSEventListener.cpp:69
> +    // JSEventListener may outlive its m_jsFunction. In this case, we will
> +    // m_jsFunction to be 0.

"we will": seems like you meant something else here.

There's a lot of commentary about m_jsFunction here, but this function doesn't
use that data member. Does any of this explain why the base class
implementation of initializeJSFunction() should return 0?