Bug 102183 – REGRESSION(r134495): It made svg/custom/use-instanceRoot-event-listeners.xhtml fail and fast/events/attribute-listener-deletion-crash.html timeout

• Component: New Bugs
• Status: RESOLVED FIXED

[Csaba Osztrogonác]

on all ports:

--- /Volumes/Data/slave/lion-release-tests-wk1/build/layout-test-results/svg/custom/use-instanceRoot-event-listeners-expected.txt
+++ /Volumes/Data/slave/lion-release-tests-wk1/build/layout-test-results/svg/custom/use-instanceRoot-event-listeners-actual.txt
@@ -6,8 +6,10 @@
 Test 1 / 8 PASSED
 Test 2 / 8 PASSED
 Test 3 / 8 PASSED
+Test 4 / 8 FAILED (expected: 'mouseup' actual: 'mousedown')
 Test 4 / 8 PASSED
 Test 5 / 8 PASSED
+Test 6 / 8 FAILED (expected: 'mouseup' actual: 'mousedown')
 Test 6 / 8 PASSED
 Test 7 / 8 PASSED
 Test 8 / 8 PASSED


--- /Volumes/Data/slave/lion-release-tests-wk1/build/layout-test-results/fast/events/attribute-listener-deletion-crash-expected.txt
+++ /Volumes/Data/slave/lion-release-tests-wk1/build/layout-test-results/fast/events/attribute-listener-deletion-crash-actual.txt
@@ -18,4 +18,39416 @@
 CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
 CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
 CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
-PASS
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
+CONSOLE MESSAGE: line 1: SyntaxError: Unexpected token '|'
...

[Csaba Osztrogonác]

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

[Mark Lam]

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

[Mark Lam]

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

[Mark Lam]

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.

[Mark Lam]

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

[Mark Lam]

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

[Csaba Osztrogonác]

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.

[Mark Lam]

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.

[Geoffrey Garen]

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.

[Mark Lam]

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

[Csaba Osztrogonác]

(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