Bug 102183

I skipped them on Qt to paint the bots green - r134554. Please unskip them with the proper fix.

I thought these were pre-existing test failures, but I'll investigate and double check.

I've reproduced the regression. Investigating for the fix now.

Created attachment 174107 [details] Fix. 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.

Oh ... and thanks to Csaba for identifying and reporting the regressions.

Ref: <rdar://problem/12698338>.

Thanks for the quick fix, all tests pass for me on Qt with it. Please _unskip_ these tests on Qt when you land the patch. Thanks.

Created attachment 174248 [details] A more appropriate fix. This patch has a more appropriate fix that conforms to the contract that when JSEventListener::m_wrapper is 0, we expect JSEventListener::m_jsFunction to be 0.

Comment on attachment 174248 [details] A more appropriate fix. View in context: https://bugs.webkit.org/attachment.cgi?id=174248&action=review r=me > Source/WebCore/bindings/js/JSEventListener.cpp:169 > + // accessed. m_jsFunction should effectively be 0 in that case. > + JSC::JSObject* jsFunction = m_wrapper ? m_jsFunction.get() : 0; The duplication of this functionality and commentary suggests that a helper function is in order. I won't require that here since you've posted a follow-up patch that obsoletes this code.

Fix landed in r134666: <http://trac.webkit.org/changeset/134666>.

(In reply to comment #10) > Fix landed in r134666: <http://trac.webkit.org/changeset/134666>. and unskip landed in http://trac.webkit.org/changeset/134768