RESOLVED FIXED 12170
RenderView holds dangling reference to RenderObjects as selection markers 
https://bugs.webkit.org/show_bug.cgi?id=12170
Summary RenderView holds dangling reference to RenderObjects as selection markers
Dex Deacon
Reported 2007-01-08 15:31:39 PST
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.
Attachments
sets assertions to catch dangling ref (668 bytes, patch)
2007-01-08 15:36 PST, Dex Deacon 		no flags
DetailsFormatted DiffDiff
simplified test page (483 bytes, text/html)
2007-01-08 16:33 PST, Dex Deacon 		no flags
Details
proposed patch (1.66 KB, patch)
2007-01-08 17:44 PST, Dex Deacon 		no flags
DetailsFormatted DiffDiff
fixed patch as darin requested (1.90 KB, patch)
2007-01-08 18:15 PST, Dex Deacon 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dex Deacon
Comment 1 2007-01-08 15:36:28 PST
Created attachment 12314 [details] 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.
Dex Deacon
Comment 2 2007-01-08 16:33:23 PST
Created attachment 12317 [details] 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).
Dex Deacon
Comment 3 2007-01-08 17:44:46 PST
Created attachment 12320 [details] 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.
Darin Adler
Comment 4 2007-01-08 17:56:52 PST
Comment on attachment 12320 [details] 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.
Dex Deacon
Comment 5 2007-01-08 18:15:43 PST
Created attachment 12323 [details] fixed patch as darin requested
Darin Adler
Comment 6 2007-01-08 22:17:25 PST
Comment on attachment 12323 [details] fixed patch as darin requested r=me
Darin Adler
Comment 7 2007-01-09 06:57:44 PST
I would have liked to put those assertions into RenderView too, but they still fire. Maybe there's still a real bug here.
Darin Adler
Comment 8 2007-01-09 07:00:44 PST
Comment on attachment 12323 [details] fixed patch as darin requested Committed revision 18709. Clearing the review flag on the patch because the bug may need to stay open anyway.
Dex Deacon
Comment 9 2007-01-09 11:08:11 PST
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).
Alexey Proskuryakov
Comment 10 2010-06-11 16:16:09 PDT
I agree with the previous comment, these assertions seem too string to try and make them always pass.
Note You need to log in before you can comment on or make changes to this bug.