[webkit-reviews] review requested: [Bug 102183] REGRESSION(r134495): It made svg/custom/use-instanceRoot-event-listeners.xhtml fail and fast/events/attribute-listener-deletion-crash.html timeout : [Attachment 174107] Fix.

[bugzilla-daemon at webkit.org]

Mark Lam <mark.lam at apple.com> has asked  for review:
Bug 102183: REGRESSION(r134495): It made
svg/custom/use-instanceRoot-event-listeners.xhtml fail and
fast/events/attribute-listener-deletion-crash.html timeout
https://bugs.webkit.org/show_bug.cgi?id=102183

Attachment 174107: Fix.
https://bugs.webkit.org/attachment.cgi?id=174107&action=review

------- Additional Comments from Mark Lam <mark.lam at apple.com>
EventListenerMap::remove() relies on Vector::find() to find the event listener
to remove.  For listeners whose m_wrapper and m_jsFunction are both
uninitialized (still 0) as in the case of lazy event listeners, the m_wrapper
null check broke this.

The fix is to add a pointer check.  If the addresses of the 2 event listeners
being compared are the same, then the 2 must be the same.  Hence, we can return
true for JSEventListener::operator==() in this case.

Note: previously, operator==() would simply compare the values of m_jsFunction
and m_isAttribute.  With lazy event listeners, it is possible that multiple
listeners will have a NULL m_jsFunction, and it is not too improbable that
their m_isAttribute is also the same (since it is a boolean).  In that previous
case, there could be a false positive, and EventListenerMap::remove() could
inadvertently remove the wrong lazy event listener simply because its
m_jsFunction wasn't realized yet at the time.  This false positive could result
in incorrect behavior (realizing the wrong lazy function) and possibly
corrupted memory (this part is conjecture ... I'm not certain of it). 

Similarly, in the current state of the code (after the addition of the null
check for m_wrapper), I don't think, when m_jsFunction is 0, that comparing
m_jsFunction and m_isAttribute is an adequate test for identifying the listener
EventListenerMap::remove() is searching for.  Instead, the solution I'm
applying to simply compare the addresses of the 2 listeners is guaranteed to be
correct if the 2 addresses are the same.  At worst, we will get a false
negative which can result in a leak (the listener cannot be removed), but it
will not cause incorrect behavior nor corrupt memory.

However, the API for removeEventListener() (from the EventTarget down) requires
the address of the event listener as an arg.  Note that both the constructors
of JSEventListener and JSLazyEventListener are protected and private
respectively.  The only way to make one of these listeners is to call a
create() factory method.  This suggests that it is not likely that a client
would construct another instance of JSEventListener (or the lazy one) just to
use it for comparison in removeEventListener().  Hence, it is unlikely that
we'll see false negatives from comparing the addresses of listeners.

At any rate, with this patch does fix the regressions, and the webkit tests are
happy again.