102183 2012-11-13 23:16:13 -0800 REGRESSION(r134495): It made svg/custom/use-instanceRoot-event-listeners.xhtml fail and fast/events/attribute-listener-deletion-crash.html timeout 2026-04-01 09:30:30 -0700 1 1 1 Unclassified WebKit New Bugs 420+ All All RESOLVED FIXED InRadar P1 Critical --- 79666 79668 101985 1 ossy mark.lam ggaren mark.lam ossy wjdybdhalbady zan oldest_to_newest 766477 0 ossy 2012-11-13 23:16:13 -0800 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 '|' ... 766486 1 ossy 2012-11-13 23:21:00 -0800 I skipped them on Qt to paint the bots green - r134554. Please unskip them with the proper fix. 766491 2 mark.lam 2012-11-13 23:27:09 -0800 I thought these were pre-existing test failures, but I'll investigate and double check. 766510 3 mark.lam 2012-11-13 23:49:34 -0800 I've reproduced the regression. Investigating for the fix now. 766623 4 174107 mark.lam 2012-11-14 01:41:01 -0800 Created attachment 174107 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. 766625 5 mark.lam 2012-11-14 01:42:05 -0800 Oh ... and thanks to Csaba for identifying and reporting the regressions. 766647 6 mark.lam 2012-11-14 02:00:41 -0800 Ref: <rdar://problem/12698338>. 766679 7 ossy 2012-11-14 02:41:49 -0800 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. 767257 8 174248 mark.lam 2012-11-14 13:48:31 -0800 Created attachment 174248 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. 767264 9 174248 ggaren 2012-11-14 13:52:05 -0800 Comment on attachment 174248 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. 767294 10 mark.lam 2012-11-14 14:09:28 -0800 Fix landed in r134666: <http://trac.webkit.org/changeset/134666>. 767998 11 ossy 2012-11-15 06:09:58 -0800 (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 174107 2012-11-14 01:41:01 -0800 2012-11-14 13:48:31 -0800 Fix. bug-102183.patch text/plain 1746 mark.lam SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzNDU2OSkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTExLTE0ICBNYXJrIExh bSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBGaXhlZCByZWdyZXNzaW9ucyBkdWUg dG8gYWRkaW5nIEpTRXZlbnRMaXN0ZW5lcjo6bV93cmFwcGVyIG51bGwgY2hlY2tzLgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyMTgzLgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEV2ZW50TGlzdGVuZXJN YXA6OnJlbW92ZSgpIHJlbGllcyBvbiBWZWN0b3I6OmZpbmQoKSB0byBmaW5kIHRoZSBldmVudAor ICAgICAgICBsaXN0ZW5lciB0byByZW1vdmUuIEZvciBsaXN0ZW5lcnMgd2hvc2UgbV93cmFwcGVy IGFuZCBtX2pzRnVuY3Rpb24KKyAgICAgICAgYXJlIGJvdGggdW5pbml0aWFsaXplZCAoc3RpbGwg MCkgYXMgaW4gdGhlIGNhc2Ugb2YgbGF6eSBldmVudAorICAgICAgICBsaXN0ZW5lcnMsIHRoZSBt X3dyYXBwZXIgbnVsbCBjaGVjayBicm9rZSB0aGlzLiBUaGUgZml4IGlzIHRvIGFkZAorICAgICAg ICBhIHBvaW50ZXIgY2hlY2suIElmIHRoZSBhZGRyZXNzZXMgb2YgdGhlIDIgZXZlbnQgbGlzdGVu ZXJzIGJlaW5nCisgICAgICAgIGNvbXBhcmVkIGFyZSB0aGUgc2FtZSwgdGhlbiB0aGUgMiBtdXN0 IGJlIHRoZSBzYW1lLgorCisgICAgICAgIE5vIG5ldyB0ZXN0cy4KKworICAgICAgICAqIGJpbmRp bmdzL2pzL0pTRXZlbnRMaXN0ZW5lci5jcHA6CisgICAgICAgIChXZWJDb3JlOjpKU0V2ZW50TGlz dGVuZXI6Om9wZXJhdG9yPT0pOgorCiAyMDEyLTExLTE0ICBLZW50YXJvIEhhcmEgIDxoYXJha2Vu QGNocm9taXVtLm9yZz4KIAogICAgICAgICBbVjhdIERPTSB3cmFwcGVyIG9iamVjdHMgc2hvdWxk IGJlIGNvbGxlY3RlZCBpbiBtaW5vciBHQyBjeWNsZXMKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2Jp bmRpbmdzL2pzL0pTRXZlbnRMaXN0ZW5lci5jcHAKPT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291cmNlL1dlYkNv cmUvYmluZGluZ3MvanMvSlNFdmVudExpc3RlbmVyLmNwcAkocmV2aXNpb24gMTM0NTQyKQorKysg U291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNFdmVudExpc3RlbmVyLmNwcAkod29ya2luZyBj b3B5KQpAQCAtMTYzLDYgKzE2MywxMCBAQCBib29sIEpTRXZlbnRMaXN0ZW5lcjo6dmlydHVhbGlz QXR0cmlidXRlCiAKIGJvb2wgSlNFdmVudExpc3RlbmVyOjpvcGVyYXRvcj09KGNvbnN0IEV2ZW50 TGlzdGVuZXImIGxpc3RlbmVyKQogeworICAgIC8vIElmIHdlJ3ZlIGdvdCB0aGUgc2FtZSBhZGRy ZXNzLCB3ZSBtdXN0IGJlIHRoZSBzYW1lLgorICAgIGlmICh0aGlzID09ICZsaXN0ZW5lcikKKyAg ICAgICAgcmV0dXJuIHRydWU7CisKICAgICAvLyBJZiBtX3dyYXBwZXIgaXMgMCwgdGhlbiBqc0Z1 bmN0aW9uIGlzIHpvbWJpZWQsIGFuZCBzaG91bGQgbmV2ZXIgYmUgYWNjZXNzZWQuCiAgICAgaWYg KCFtX3dyYXBwZXIpCiAgICAgICAgIHJldHVybiBmYWxzZTsK 174248 2012-11-14 13:48:31 -0800 2012-11-14 13:52:05 -0800 A more appropriate fix. bug-102183.patch text/plain 3291 mark.lam SW5kZXg6IFNvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBTb3VyY2UvV2Vi Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDEzNDY1NCkKKysrIFNvdXJjZS9XZWJDb3JlL0NoYW5n ZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDIyIEBACisyMDEyLTExLTE0ICBNYXJrIExh bSAgPG1hcmsubGFtQGFwcGxlLmNvbT4KKworICAgICAgICBGaXhlZCByZWdyZXNzaW9ucyBkdWUg dG8gYWRkaW5nIEpTRXZlbnRMaXN0ZW5lcjo6bV93cmFwcGVyIG51bGwgY2hlY2tzLgorICAgICAg ICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTAyMTgzLgorCisgICAg ICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEZpeGVkIEpTRXZlbnRM aXN0ZW5lcjo6b3BlcmF0b3I9PSgpIHRvIHdvcmsgd2l0aGluIHRoZSBjb250cmFjdCB0aGF0Cisg ICAgICAgIHdoZW4gbV93cmFwcGVyIGlzIDAsIG1fanNGdW5jdGlvbiBpcyBhbHNvIGV4cGVjdGVk IHRvIGJlIDAuIEFsc28gZml4ZWQKKyAgICAgICAgc29tZSB0eXBvcyBpbiBjb21tZW50cy4KKwor ICAgICAgICBObyBuZXcgdGVzdHMuCisKKyAgICAgICAgKiBiaW5kaW5ncy9qcy9KU0V2ZW50TGlz dGVuZXIuY3BwOgorICAgICAgICAoV2ViQ29yZTo6SlNFdmVudExpc3RlbmVyOjp2aXNpdEpTRnVu Y3Rpb24pOgorICAgICAgICAoV2ViQ29yZTo6SlNFdmVudExpc3RlbmVyOjpvcGVyYXRvcj09KToK KyAgICAgICAgKiBiaW5kaW5ncy9qcy9KU0V2ZW50TGlzdGVuZXIuaDoKKyAgICAgICAgKFdlYkNv cmU6OkpTRXZlbnRMaXN0ZW5lcjo6anNGdW5jdGlvbik6CisKIDIwMTItMTEtMTQgIE5hdGUgQ2hh cGluICA8amFwaGV0QGNocm9taXVtLm9yZz4KIAogICAgICAgICBNb3ZlIGVtcHR5IGxvYWRpbmcg dG8gRG9jdW1lbnRMb2FkZXIsIHNpbXBsaWZ5IEZyYW1lTG9hZGVyOjppbml0KCkKSW5kZXg6IFNv dXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTRXZlbnRMaXN0ZW5lci5jcHAKPT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQot LS0gU291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNFdmVudExpc3RlbmVyLmNwcAkocmV2aXNp b24gMTM0NTQyKQorKysgU291cmNlL1dlYkNvcmUvYmluZGluZ3MvanMvSlNFdmVudExpc3RlbmVy LmNwcAkod29ya2luZyBjb3B5KQpAQCAtNjQsNyArNjQsNyBAQCBKU09iamVjdCogSlNFdmVudExp c3RlbmVyOjppbml0aWFsaXplSlNGCiAKIHZvaWQgSlNFdmVudExpc3RlbmVyOjp2aXNpdEpTRnVu Y3Rpb24oU2xvdFZpc2l0b3ImIHZpc2l0b3IpCiB7Ci0gICAgLy8gSWYgbV93cmFwcGVyIGlzIDAs IHRoZW4ganNGdW5jdGlvbiBpcyB6b21iaWVkLCBhbmQgc2hvdWxkIG5ldmVyIGJlIGFjY2Vzc2Vk LgorICAgIC8vIElmIG1fd3JhcHBlciBpcyAwLCB0aGVuIG1fanNGdW5jdGlvbiBpcyB6b21iaWVk LCBhbmQgc2hvdWxkIG5ldmVyIGJlIGFjY2Vzc2VkLgogICAgIGlmICghbV93cmFwcGVyKQogICAg ICAgICByZXR1cm47CiAKQEAgLTE2MywxMiArMTYzLDE0IEBAIGJvb2wgSlNFdmVudExpc3RlbmVy Ojp2aXJ0dWFsaXNBdHRyaWJ1dGUKIAogYm9vbCBKU0V2ZW50TGlzdGVuZXI6Om9wZXJhdG9yPT0o Y29uc3QgRXZlbnRMaXN0ZW5lciYgbGlzdGVuZXIpCiB7Ci0gICAgLy8gSWYgbV93cmFwcGVyIGlz IDAsIHRoZW4ganNGdW5jdGlvbiBpcyB6b21iaWVkLCBhbmQgc2hvdWxkIG5ldmVyIGJlIGFjY2Vz c2VkLgotICAgIGlmICghbV93cmFwcGVyKQotICAgICAgICByZXR1cm4gZmFsc2U7Ci0KLSAgICBp ZiAoY29uc3QgSlNFdmVudExpc3RlbmVyKiBqc0V2ZW50TGlzdGVuZXIgPSBKU0V2ZW50TGlzdGVu ZXI6OmNhc3QoJmxpc3RlbmVyKSkKLSAgICAgICAgcmV0dXJuIG1fanNGdW5jdGlvbiA9PSBqc0V2 ZW50TGlzdGVuZXItPm1fanNGdW5jdGlvbiAmJiBtX2lzQXR0cmlidXRlID09IGpzRXZlbnRMaXN0 ZW5lci0+bV9pc0F0dHJpYnV0ZTsKKyAgICBpZiAoY29uc3QgSlNFdmVudExpc3RlbmVyKiBqc0V2 ZW50TGlzdGVuZXIgPSBKU0V2ZW50TGlzdGVuZXI6OmNhc3QoJmxpc3RlbmVyKSkgeworICAgICAg ICAvLyBJZiBtX3dyYXBwZXIgaXMgMCwgdGhlbiBtX2pzRnVuY3Rpb24gaXMgem9tYmllZCwgYW5k IHNob3VsZCBuZXZlciBiZQorICAgICAgICAvLyBhY2Nlc3NlZC4gbV9qc0Z1bmN0aW9uIHNob3Vs ZCBlZmZlY3RpdmVseSBiZSAwIGluIHRoYXQgY2FzZS4KKyAgICAgICAgSlNDOjpKU09iamVjdCog anNGdW5jdGlvbiA9IG1fd3JhcHBlciA/IG1fanNGdW5jdGlvbi5nZXQoKSA6IDA7CisgICAgICAg IEpTQzo6SlNPYmplY3QqIG90aGVySlNGdW5jdGlvbiA9IGpzRXZlbnRMaXN0ZW5lci0+bV93cmFw cGVyID8KKyAgICAgICAgICAgIGpzRXZlbnRMaXN0ZW5lci0+bV9qc0Z1bmN0aW9uLmdldCgpIDog MDsKKyAgICAgICAgcmV0dXJuIGpzRnVuY3Rpb24gPT0gb3RoZXJKU0Z1bmN0aW9uICYmIG1faXNB dHRyaWJ1dGUgPT0ganNFdmVudExpc3RlbmVyLT5tX2lzQXR0cmlidXRlOworICAgIH0KICAgICBy ZXR1cm4gZmFsc2U7CiB9CiAKSW5kZXg6IFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pTRXZl bnRMaXN0ZW5lci5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL2pzL0pT RXZlbnRMaXN0ZW5lci5oCShyZXZpc2lvbiAxMzQ1NDIpCisrKyBTb3VyY2UvV2ViQ29yZS9iaW5k aW5ncy9qcy9KU0V2ZW50TGlzdGVuZXIuaAkod29ya2luZyBjb3B5KQpAQCAtOTAsNyArOTAsNyBA QCBuYW1lc3BhY2UgV2ViQ29yZSB7CiAgICAgICAgIC8vIHdvcmxkIGFuZCBjYW4gaGF2ZSB6b21i aWUgbV9qc0Z1bmN0aW9ucy4KICAgICAgICAgQVNTRVJUKCFtX2lzb2xhdGVkV29ybGQtPmlzTm9y bWFsKCkgfHwgbV93cmFwcGVyIHx8ICFtX2pzRnVuY3Rpb24pOwogCi0gICAgICAgIC8vIElmIG1f d3JhcHBlciBpcyAwLCB0aGVuIGpzRnVuY3Rpb24gaXMgem9tYmllZCwgYW5kIHNob3VsZCBuZXZl ciBiZSBhY2Nlc3NlZC4KKyAgICAgICAgLy8gSWYgbV93cmFwcGVyIGlzIDAsIHRoZW4gbV9qc0Z1 bmN0aW9uIGlzIHpvbWJpZWQsIGFuZCBzaG91bGQgbmV2ZXIgYmUgYWNjZXNzZWQuCiAgICAgICAg IGlmICghbV93cmFwcGVyKQogICAgICAgICAgICAgcmV0dXJuIDA7CiAK