Bug 46139

Summary:

[chromium] LayerChromium and its derived types should use virtual destructors

Product:

WebKit

Reporter:

Vangelis Kokkevis <vangelis>

Component:

WebCore Misc.

Assignee:

Vangelis Kokkevis <vangelis>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, jamesr, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

OS:

OS X 10.5

Attachments:

Description	Flags
proposed patch	jamesr: review+
Proposed patch - Does proper ref counting for the compositor	none
proposed patch - fixed style issue	jamesr: review+

Vangelis Kokkevis

Reported 2010-09-20 17:02:26 PDT

Currently LayerChromium doesn't define it destructor as virtual which means that destructors for derived types don't execute when layers get removed from the layer tree.

Attachments
proposed patch (1.79 KB, patch) 2010-09-20 18:29 PDT, Vangelis Kokkevis	jamesr: review+	Details Formatted Diff Diff
Proposed patch - Does proper ref counting for the compositor (11.31 KB, patch) 2010-09-27 15:26 PDT, Vangelis Kokkevis	no flags	Details Formatted Diff Diff
proposed patch - fixed style issue (11.30 KB, patch) 2010-09-27 15:33 PDT, Vangelis Kokkevis	jamesr: review+	Details Formatted Diff Diff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.

Vangelis Kokkevis

Comment 1 2010-09-20 18:29:14 PDT

Created attachment 68170 [details] proposed patch

James Robinson

Comment 2 2010-09-20 18:32:21 PDT

Comment on attachment 68170 [details] proposed patch R=me Technically the virtual keyword on ContentLayerChromium is redundant, but I prefer to have it for clarity.

Vangelis Kokkevis

Comment 3 2010-09-20 18:36:17 PDT

Committed r67906: <http://trac.webkit.org/changeset/67906>

Vangelis Kokkevis

Comment 4 2010-09-20 19:13:16 PDT

Reverted r67906 for reason: Change causes chromium to crash when switching pages Committed r67912: <http://trac.webkit.org/changeset/67912>

Vangelis Kokkevis

Comment 5 2010-09-27 15:26:34 PDT

Created attachment 68973 [details] Proposed patch - Does proper ref counting for the compositor Corrected the reason why the previous patch was causing crashes which was that LayerChromium destructors could end up getting called after the layer compositor and its associated gl context were destroyed. This patch makes the LayerRendererChromium a ref counted class and the LayerChromium's used by it are now holding a reference to LayerRendererChromium they are used with. This ensures that the gl context used by the layers will outlive the layers themselves.

WebKit Review Bot

Comment 6 2010-09-27 15:28:47 PDT

Attachment 68973 [details] did not pass style-queue: Failed to run "['WebKitTools/Scripts/check-webkit-style']" exit_code: 1 WebCore/platform/graphics/chromium/ContentLayerChromium.cpp:138: One line control clauses should not use braces. [whitespace/braces] [4] Total errors found: 1 in 9 files If any of these errors are false positives, please file a bug against check-webkit-style.

Vangelis Kokkevis

Comment 7 2010-09-27 15:33:37 PDT

Created attachment 68975 [details] proposed patch - fixed style issue

James Robinson

Comment 8 2010-09-27 15:52:24 PDT

Comment on attachment 68975 [details] proposed patch - fixed style issue Looks good!

Vangelis Kokkevis

Comment 9 2010-09-27 16:36:14 PDT

Committed r68442: <http://trac.webkit.org/changeset/68442>

WebKit Review Bot

Comment 10 2010-09-27 17:21:14 PDT

http://trac.webkit.org/changeset/68442 might have broken Leopard Intel Release (Tests) The following changes are on the blame list: http://trac.webkit.org/changeset/68441 http://trac.webkit.org/changeset/68442 http://trac.webkit.org/changeset/68443 http://trac.webkit.org/changeset/68444 http://trac.webkit.org/changeset/68445

Note You need to log in before you can comment on or make changes to this bug.