Bug 101989 – JSC: JSEventListener's m_jsFunction should be a weak ref

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Mark Lam]

The GC can collect the JSFunction object without JSEventListener's knowledge.  Hence, JSEventListener's m_jsFunction ref should be a weak ref to reflect this.

[Mark Lam]

Created attachment 173774 [details]
1st draft.

[Early Warning System Bot]

Comment on attachment 173774 [details]
1st draft.

Attachment 173774 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14810697

[Mark Lam]

Created attachment 173777 [details]
2nd draft: added missing file from last patch.

[Early Warning System Bot]

Comment on attachment 173777 [details]
2nd draft: added missing file from last patch.

Attachment 173777 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14810702

[Early Warning System Bot]

Comment on attachment 173777 [details]
2nd draft: added missing file from last patch.

Attachment 173777 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/14809661

[Mark Lam]

Comment on attachment 173777 [details]
2nd draft: added missing file from last patch.

Something is fishy.  Why is it not building?  Will investigate.

[Mark Lam]

Created attachment 173801 [details]
3rd time's the charm.

[Geoffrey Garen]

Comment on attachment 173801 [details]
3rd time's the charm.

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?

[Mark Lam]

(In reply to comment #8)
> (From update of attachment 173801 [details])
> 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?

Unfortunately not.  Just presenting it to let you have a look in case I made some major mistake.

> > 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"?

OK, will change to "appendUnbarrieredWeak" to be consistent (plus it's easier to grep for when we don't use overloaded names).  There are already 2 other pre-existing "appendUnbarrieredX" functions.

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

Thanks.  Will fix and change to .get().

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

It's a remnant from when I was still trying to understand this code.  I should be able to change it to use .get() and remove this.

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

There used to be an assertion here that was invalidated by the change to use weak refs.  This comment was meant to explain the circumstances behind why the assertion should not be there, but appears to be incomplete (my thought was probably interrupted in mid edit).  After making the change to check if m_wrapper is 0 at the top of JSEventListener::jsFunction(), the change is no longer necessary and can be reverted.

Thanks.

[Mark Lam]

Created attachment 174061 [details]
Fix.

All the issues that Geoff pointed out has been addressed.  The website rendering issue turned out to be because the Weak implementation did not have an operation==() implementation.  As a result, when we changed m_jsFunction to be a Weak ref, JSEventListener::operator==() (which was comparing m_jsFunctions) was getting the wrong result.  This is now fixed by providing an implementation of operator==(const Weak<T>&, const Weak<T>&).

[Geoffrey Garen]

Comment on attachment 174061 [details]
Fix.

r=me

[Mark Lam]

Fix landed in r134697: <http://trac.webkit.org/changeset/134697>.