Bug 46139 - [chromium] LayerChromium and its derived types should use virtual destructors
Summary: [chromium] LayerChromium and its derived types should use virtual destructors
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC OS X 10.5
: P2 Normal
Assignee: Vangelis Kokkevis
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-09-20 17:02 PDT by Vangelis Kokkevis
Modified: 2010-09-27 17:21 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Vangelis Kokkevis 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.
Comment 1 Vangelis Kokkevis 2010-09-20 18:29:14 PDT
Created attachment 68170 [details]
proposed patch
Comment 2 James Robinson 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.
Comment 3 Vangelis Kokkevis 2010-09-20 18:36:17 PDT
Committed r67906: <http://trac.webkit.org/changeset/67906>
Comment 4 Vangelis Kokkevis 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>
Comment 5 Vangelis Kokkevis 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.
Comment 6 WebKit Review Bot 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.
Comment 7 Vangelis Kokkevis 2010-09-27 15:33:37 PDT
Created attachment 68975 [details]
proposed patch - fixed style issue
Comment 8 James Robinson 2010-09-27 15:52:24 PDT
Comment on attachment 68975 [details]
proposed patch - fixed style issue

Looks good!
Comment 9 Vangelis Kokkevis 2010-09-27 16:36:14 PDT
Committed r68442: <http://trac.webkit.org/changeset/68442>