12170 2007-01-08 15:31:39 -0800 RenderView holds dangling reference to RenderObjects as selection markers 2010-06-11 16:16:09 -0700 1 1 1 Unclassified WebKit Layout and Rendering 420+ All All RESOLVED FIXED P1 Major --- 1 occupant4 webkit-unassigned ap oldest_to_newest 36289 0 occupant4 2007-01-08 15:31:39 -0800 I'll follow up with a reproduce case. Basically, the m_selectionStart and End members of RenderView are being kept past the lifetime of the RenderObjects they refer to. I've only seen it happen as a document is being destroyed. This can cause crashes on Windows, but is still a problem on all platforms. 36229 1 12314 occupant4 2007-01-08 15:36:28 -0800 Created attachment 12314 sets assertions to catch dangling ref Apply the test.patch I attached, build-webkit, and run gdb-safari. Load the following layout test from svn: LayoutTests/dom/html/level2/html/HTMLInputElement22.html . The ASSERT should fire. 36239 2 12317 occupant4 2007-01-08 16:33:23 -0800 Created attachment 12317 simplified test page This is a simplified test case. Point a patched version of safari at this html page instead of the layout test. (I basically gutted the layout test to include only what causes the assert/crash). 36178 3 12320 occupant4 2007-01-08 17:44:46 -0800 Created attachment 12320 proposed patch enums are signed on Windows, so in this case, m_selectionState = 4 results in m_selectionState == -4. This was causing a RenderText object not to be recognized as selected, so it was deleted without clearing the selection, resulting in a dangling ref to it. I was wrong that this applied to Mac as well. It appears Mac treats enums as unsigned in this case. My asserts were firing for other reasons. 36163 4 12320 darin 2007-01-08 17:56:52 -0800 Comment on attachment 12320 proposed patch Change looks fine, but there should be a comment to prevent a future programmer from changing it back again. Also the change log as attached here isn't landable. 36168 5 12323 occupant4 2007-01-08 18:15:43 -0800 Created attachment 12323 fixed patch as darin requested 36140 6 12323 darin 2007-01-08 22:17:25 -0800 Comment on attachment 12323 fixed patch as darin requested r=me 36050 7 darin 2007-01-09 06:57:44 -0800 I would have liked to put those assertions into RenderView too, but they still fire. Maybe there's still a real bug here. 36049 8 12323 darin 2007-01-09 07:00:44 -0800 Comment on attachment 12323 fixed patch as darin requested Committed revision 18709. Clearing the review flag on the patch because the bug may need to stay open anyway. 36022 9 occupant4 2007-01-09 11:08:11 -0800 I think it's OK that the assertions fire, because that code (setSelection) gets called from within RenderObject::destroy() (via RenderContainer::removeChild). As part of the cleanup of the object, it clears the selection, which calls that code. (Before this patch, setSelection wouldn't get called until after destroy() was finished, which was definitely bad). 237112 10 ap 2010-06-11 16:16:09 -0700 I agree with the previous comment, these assertions seem too string to try and make them always pass. 12314 2007-01-08 15:36:28 -0800 2010-06-10 14:26:29 -0700 sets assertions to catch dangling ref test.patch text/plain 668 occupant4 SW5kZXg6IFdlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclZpZXcuY3BwCj09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdl YkNvcmUvcmVuZGVyaW5nL1JlbmRlclZpZXcuY3BwCShyZXZpc2lvbiAxODY3NykKKysrIFdlYkNv cmUvcmVuZGVyaW5nL1JlbmRlclZpZXcuY3BwCSh3b3JraW5nIGNvcHkpCkBAIC0yNzcsNiArMjc3 LDkgQEAgdm9pZCBSZW5kZXJWaWV3OjpzZXRTZWxlY3Rpb24oUmVuZGVyT2JqZQogICAgICAgICBt X3NlbGVjdGlvbkVuZCA9PSBlbmQgJiYgbV9zZWxlY3Rpb25FbmRQb3MgPT0gZW5kUG9zKQogICAg ICAgICByZXR1cm47CiAKKyAgICBBU1NFUlQoIW1fc2VsZWN0aW9uU3RhcnQgfHwgbV9zZWxlY3Rp b25TdGFydC0+bm9kZSgpLT5yZW5kZXJlcigpKTsKKyAgICBBU1NFUlQoIW1fc2VsZWN0aW9uRW5k IHx8IG1fc2VsZWN0aW9uRW5kLT5ub2RlKCktPnJlbmRlcmVyKCkpOworCiAgICAgLy8gUmVjb3Jk IHRoZSBvbGQgc2VsZWN0ZWQgb2JqZWN0cy4gIFRoZXNlIHdpbGwgYmUgdXNlZCBsYXRlcgogICAg IC8vIHdoZW4gd2UgY29tcGFyZSBhZ2FpbnN0IHRoZSBuZXcgc2VsZWN0ZWQgb2JqZWN0cy4KICAg ICBpbnQgb2xkU3RhcnRQb3MgPSBtX3NlbGVjdGlvblN0YXJ0UG9zOwo= 12317 2007-01-08 16:33:23 -0800 2007-01-08 16:33:23 -0800 simplified test page testcase.html text/html 483 occupant4 PCFET0NUWVBFIEhUTUwgUFVCTElDICItLy9XM0MvL0RURCBIVE1MIDQuMDEgVHJhbnNpdGlvbmFs Ly9FTiI+CjxIVE1MPgo8SEVBRD4KPE1FVEEgSFRUUC1FUVVJVj0iQ29udGVudC1UeXBlIiBDT05U RU5UPSJ0ZXh0L2h0bWw7IENIQVJTRVQ9dXRmLTgiPgo8VElUTEU+TklTVCBET00gSFRNTCBUZXN0 IC0gSU5QVVQ8L1RJVExFPgo8c2NyaXB0IHR5cGU9J3RleHQvamF2YXNjcmlwdCc+CiAgZnVuY3Rp b24gbG9hZENvbXBsZXRlKCkgewogICAgdmFyIG5vZGVMaXN0ID0gZG9jdW1lbnQuZ2V0RWxlbWVu dHNCeVRhZ05hbWUoImlucHV0Iik7CiAgICB2YXIgdGVzdE5vZGUgPSBub2RlTGlzdC5pdGVtKDAp OwogICAgdGVzdE5vZGUuc2VsZWN0KCk7CiAgICBkb2N1bWVudC5vcGVuKCk7CiAgfQo8L3Njcmlw dD4KPC9IRUFEPgo8Qk9EWSBvbmxvYWQ9ImxvYWRDb21wbGV0ZSgpIj4KPElOUFVUIFZBTFVFPSJQ YXNzd29yZCIvPgo8L0JPRFk+CjwvSFRNTD4K 12320 2007-01-08 17:44:46 -0800 2007-01-09 07:12:38 -0800 proposed patch selection.patch text/plain 1704 occupant4 SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAxODY5MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTIgQEAKKzIwMDctMDEtMDggIERleCBEZWFjb24gIDxvY2N1cGFudDRAZ21haWwu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdB Uk5JTkc6IE5PIFRFU1QgQ0FTRVMgQURERUQgT1IgQ0hBTkdFRAorCisgICAgICAgICogcmVuZGVy aW5nL1JlbmRlclRleHQuaDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlclRleHQ6OnNlbGVjdGlv blN0YXRlKToKKwogMjAwNy0wMS0wOCAgQmV0aCBEYWtpbiAgPGJkYWtpbkBhcHBsZS5jb20+CiAK ICAgICAgICAgUmV2aWV3ZWQgYnkgSHlhdHQuCkluZGV4OiBXZWJDb3JlL3JlbmRlcmluZy9SZW5k ZXJUZXh0LmgKPT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PQotLS0gV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGV4dC5oCShy ZXZpc2lvbiAxODY3NykKKysrIFdlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRleHQuaAkod29ya2lu ZyBjb3B5KQpAQCAtMTE3LDcgKzExNyw3IEBAIHB1YmxpYzoKICAgICB2b2lkIHNldFRleHRXaXRo T2Zmc2V0KFBhc3NSZWZQdHI8U3RyaW5nSW1wbD4sIHVuc2lnbmVkIG9mZnNldCwgdW5zaWduZWQg bGVuLCBib29sIGZvcmNlID0gZmFsc2UpOwogCiAgICAgdmlydHVhbCBib29sIGNhbkJlU2VsZWN0 aW9uTGVhZigpIGNvbnN0IHsgcmV0dXJuIHRydWU7IH0KLSAgICB2aXJ0dWFsIFNlbGVjdGlvblN0 YXRlIHNlbGVjdGlvblN0YXRlKCkgY29uc3QgeyByZXR1cm4gbV9zZWxlY3Rpb25TdGF0ZTsgfQor ICAgIHZpcnR1YWwgU2VsZWN0aW9uU3RhdGUgc2VsZWN0aW9uU3RhdGUoKSBjb25zdCB7IHJldHVy biBzdGF0aWNfY2FzdDxTZWxlY3Rpb25TdGF0ZT4obV9zZWxlY3Rpb25TdGF0ZSk7IH0KICAgICB2 aXJ0dWFsIHZvaWQgc2V0U2VsZWN0aW9uU3RhdGUoU2VsZWN0aW9uU3RhdGUgcyk7CiAgICAgdmly dHVhbCBJbnRSZWN0IHNlbGVjdGlvblJlY3QoKTsKICAgICB2aXJ0dWFsIEludFJlY3QgY2FyZXRS ZWN0KGludCBvZmZzZXQsIEVBZmZpbml0eSwgaW50KiBleHRyYVdpZHRoVG9FbmRPZkxpbmUgPSAw KTsKQEAgLTE2Nyw3ICsxNjcsNyBAQCBwcml2YXRlOgogICAgIGludCBtX2JlZ2luTWluV2lkdGg7 CiAgICAgaW50IG1fZW5kTWluV2lkdGg7CiAKLSAgICBTZWxlY3Rpb25TdGF0ZSBtX3NlbGVjdGlv blN0YXRlIDogMyA7CisgICAgdW5zaWduZWQgbV9zZWxlY3Rpb25TdGF0ZSA6IDMgOwogICAgIGJv b2wgbV9oYXNCcmVha2FibGVDaGFyIDogMTsgLy8gV2hldGhlciBvciBub3Qgd2UgY2FuIGJlIGJy b2tlbiBpbnRvIG11bHRpcGxlIGxpbmVzLgogICAgIGJvb2wgbV9oYXNCcmVhayA6IDE7IC8vIFdo ZXRoZXIgb3Igbm90IHdlIGhhdmUgYSBoYXJkIGJyZWFrIChlLmcuLCA8cHJlPiB3aXRoICdcbicp LgogICAgIGJvb2wgbV9oYXNUYWIgOiAxOyAvLyBXaGV0aGVyIG9yIG5vdCB3ZSBoYXZlIGEgdmFy aWFibGUgd2lkdGggdGFiIGNoYXJhY3RlciAoZS5nLiwgPHByZT4gd2l0aCAnXHQnKS4K 12323 2007-01-08 18:15:43 -0800 2007-01-09 07:00:44 -0800 fixed patch as darin requested selection-2.patch text/plain 1948 occupant4 SW5kZXg6IFdlYkNvcmUvQ2hhbmdlTG9nCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIFdlYkNvcmUvQ2hhbmdlTG9n CShyZXZpc2lvbiAxODY5MCkKKysrIFdlYkNvcmUvQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBA IC0xLDMgKzEsMTMgQEAKKzIwMDctMDEtMDggIERleCBEZWFjb24gIDxvY2N1cGFudDRAZ21haWwu Y29tPgorCisgICAgICAgIFJldmlld2VkIGJ5IERhcmluLgorCisgICAgICAgICogcmVuZGVyaW5n L1JlbmRlclRleHQuaDoKKyAgICAgICAgKFdlYkNvcmU6OlJlbmRlclRleHQ6OnNlbGVjdGlvblN0 YXRlKToKKyAgICAgICAgQ2hhbmdlIHRoZSBtX3NlbGVjdGlvblN0YXRlIGVudW0tYml0ZmllbGQg dG8gYW4gdW5zaWduZWQtYml0ZmllbGQsCisgICAgICAgIGJlY2F1c2UgZW51bXMgb24gV2luZG93 cyBhcmUgc2lnbmVkLCB3aGljaCBjYXVzZWQgaXQgdG8gYmVjb21lCisgICAgICAgIG5lZ2F0aXZl ICh3aGVyZWFzIHRoZSB2YWxpZCB2YWx1ZXMgYXJlIGFsbCBwb3NpdGl2ZSkuCisKIDIwMDctMDEt MDggIEJldGggRGFraW4gIDxiZGFraW5AYXBwbGUuY29tPgogCiAgICAgICAgIFJldmlld2VkIGJ5 IEh5YXR0LgpJbmRleDogV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyVGV4dC5oCj09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT0K LS0tIFdlYkNvcmUvcmVuZGVyaW5nL1JlbmRlclRleHQuaAkocmV2aXNpb24gMTg2NzcpCisrKyBX ZWJDb3JlL3JlbmRlcmluZy9SZW5kZXJUZXh0LmgJKHdvcmtpbmcgY29weSkKQEAgLTExNyw3ICsx MTcsNyBAQCBwdWJsaWM6CiAgICAgdm9pZCBzZXRUZXh0V2l0aE9mZnNldChQYXNzUmVmUHRyPFN0 cmluZ0ltcGw+LCB1bnNpZ25lZCBvZmZzZXQsIHVuc2lnbmVkIGxlbiwgYm9vbCBmb3JjZSA9IGZh bHNlKTsKIAogICAgIHZpcnR1YWwgYm9vbCBjYW5CZVNlbGVjdGlvbkxlYWYoKSBjb25zdCB7IHJl dHVybiB0cnVlOyB9Ci0gICAgdmlydHVhbCBTZWxlY3Rpb25TdGF0ZSBzZWxlY3Rpb25TdGF0ZSgp IGNvbnN0IHsgcmV0dXJuIG1fc2VsZWN0aW9uU3RhdGU7IH0KKyAgICB2aXJ0dWFsIFNlbGVjdGlv blN0YXRlIHNlbGVjdGlvblN0YXRlKCkgY29uc3QgeyByZXR1cm4gc3RhdGljX2Nhc3Q8U2VsZWN0 aW9uU3RhdGU+KG1fc2VsZWN0aW9uU3RhdGUpOyB9CiAgICAgdmlydHVhbCB2b2lkIHNldFNlbGVj dGlvblN0YXRlKFNlbGVjdGlvblN0YXRlIHMpOwogICAgIHZpcnR1YWwgSW50UmVjdCBzZWxlY3Rp b25SZWN0KCk7CiAgICAgdmlydHVhbCBJbnRSZWN0IGNhcmV0UmVjdChpbnQgb2Zmc2V0LCBFQWZm aW5pdHksIGludCogZXh0cmFXaWR0aFRvRW5kT2ZMaW5lID0gMCk7CkBAIC0xNjcsNyArMTY3LDcg QEAgcHJpdmF0ZToKICAgICBpbnQgbV9iZWdpbk1pbldpZHRoOwogICAgIGludCBtX2VuZE1pbldp ZHRoOwogCi0gICAgU2VsZWN0aW9uU3RhdGUgbV9zZWxlY3Rpb25TdGF0ZSA6IDMgOworICAgIHVu c2lnbmVkIG1fc2VsZWN0aW9uU3RhdGUgOiAzOyAvLyBlbnVtcyBvbiBXaW5kb3dzIGFyZSBzaWdu ZWQsIHNvIHRoaXMgbmVlZHMgdG8gYmUgdW5zaWduZWQgdG8gcHJldmVudCBpdCB0dXJuaW5nIG5l Z2F0aXZlLiAKICAgICBib29sIG1faGFzQnJlYWthYmxlQ2hhciA6IDE7IC8vIFdoZXRoZXIgb3Ig bm90IHdlIGNhbiBiZSBicm9rZW4gaW50byBtdWx0aXBsZSBsaW5lcy4KICAgICBib29sIG1faGFz QnJlYWsgOiAxOyAvLyBXaGV0aGVyIG9yIG5vdCB3ZSBoYXZlIGEgaGFyZCBicmVhayAoZS5nLiwg PHByZT4gd2l0aCAnXG4nKS4KICAgICBib29sIG1faGFzVGFiIDogMTsgLy8gV2hldGhlciBvciBu b3Qgd2UgaGF2ZSBhIHZhcmlhYmxlIHdpZHRoIHRhYiBjaGFyYWN0ZXIgKGUuZy4sIDxwcmU+IHdp dGggJ1x0JykuCg==