Bug 12170 - RenderView holds dangling reference to RenderObjects as selection markers
Summary: RenderView holds dangling reference to RenderObjects as selection markers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 420+
Hardware: All All
: P1 Major
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2007-01-08 15:31 PST by Dex Deacon
Modified: 2010-06-11 16:16 PDT (History)
1 user (show)

See Also:


Attachments
sets assertions to catch dangling ref (668 bytes, patch)
2007-01-08 15:36 PST, Dex Deacon
no flags Details | Formatted Diff | Diff
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 Details | Formatted Diff | Diff
fixed patch as darin requested (1.90 KB, patch)
2007-01-08 18:15 PST, Dex Deacon
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dex Deacon 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.
Comment 1 Dex Deacon 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.
Comment 2 Dex Deacon 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).
Comment 3 Dex Deacon 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.
Comment 4 Darin Adler 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.
Comment 5 Dex Deacon 2007-01-08 18:15:43 PST
Created attachment 12323 [details]
fixed patch as darin requested
Comment 6 Darin Adler 2007-01-08 22:17:25 PST
Comment on attachment 12323 [details]
fixed patch as darin requested

r=me
Comment 7 Darin Adler 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.
Comment 8 Darin Adler 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.
Comment 9 Dex Deacon 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).
Comment 10 Alexey Proskuryakov 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.