Bug 102183

Summary: REGRESSION(r134495): It made svg/custom/use-instanceRoot-event-listeners.xhtml fail and fast/events/attribute-listener-deletion-crash.html timeout
Product: WebKit Reporter: Csaba Osztrogonác <ossy>
Component: New BugsAssignee: Mark Lam <mark.lam>
Status: RESOLVED FIXED    
Severity: Critical CC: ggaren, mark.lam, ossy, zan
Priority: P1 Keywords: InRadar
Version: 420+   
Hardware: All   
OS: All   
Bug Depends on:    
Bug Blocks: 79666, 79668, 101985    
Attachments:
Description Flags
Fix.
none
A more appropriate fix. ggaren: review+

Description Csaba Osztrogonác 2012-11-13 23:16:13 PST
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 '|'
...
Comment 1 Csaba Osztrogonác 2012-11-13 23:21:00 PST
I skipped them on Qt to paint the bots green - r134554.
Please unskip them with the proper fix.
Comment 2 Mark Lam 2012-11-13 23:27:09 PST
I thought these were pre-existing test failures, but I'll investigate and double check.
Comment 3 Mark Lam 2012-11-13 23:49:34 PST
I've reproduced the regression.  Investigating for the fix now.
Comment 4 Mark Lam 2012-11-14 01:41:01 PST
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.
Comment 5 Mark Lam 2012-11-14 01:42:05 PST
Oh ... and thanks to Csaba for identifying and reporting the regressions.
Comment 6 Mark Lam 2012-11-14 02:00:41 PST
Ref: <rdar://problem/12698338>.
Comment 7 Csaba Osztrogonác 2012-11-14 02:41:49 PST
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.
Comment 8 Mark Lam 2012-11-14 13:48:31 PST
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 9 Geoffrey Garen 2012-11-14 13:52:05 PST
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.
Comment 10 Mark Lam 2012-11-14 14:09:28 PST
Fix landed in r134666: <http://trac.webkit.org/changeset/134666>.
Comment 11 Csaba Osztrogonác 2012-11-15 06:09:58 PST
(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