Bug 12170

Summary:

RenderView holds dangling reference to RenderObjects as selection markers

Product:

WebKit

Reporter:

Dex Deacon <occupant4>

Component:

Layout and Rendering

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Major

CC:

Priority:

Version:

420+

Hardware:

All

OS:

All

Attachments:

Description	Flags
sets assertions to catch dangling ref	none
simplified test page	none
proposed patch	none
fixed patch as darin requested	none

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	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
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.