Bug 45550

Summary:

[GTK] Placement new / manual destructor invocation should be used on private GObject memory

Product:

WebKit

Reporter:

Martin Robinson <mrobinson>

Component:

WebKitGTK

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

abarth, eric, gustavo, mrobinson, webkit.review.bot, xan.lopez

Priority:

Keywords:

Gtk

Version:

528+ (Nightly build)

Hardware:

OS:

Linux

Bug Depends on:

Bug Blocks:

21594

Attachments:

Description	Flags
Patch making this change for WebKitWebView	none

Martin Robinson

Reported 2010-09-10 10:31:52 PDT

GLib allocates and deallocates GObject private data structs itself. When those structs contain C++ members, their constructors and destructors are not called. This is not only dangerous, it makes RefPtr-type smart pointers much less useful. We can fix this problem by calling placement new on the private data struct during instance initialization and calling the destructor during finalization.

Attachments
Patch making this change for WebKitWebView (9.83 KB, patch) 2010-09-10 10:49 PDT, Martin Robinson	no flags	Details Formatted Diff Diff
View All Add attachment proposed patch, testcase, etc.

Martin Robinson

Comment 1 2010-09-10 10:49:58 PDT

Created attachment 67200 [details] Patch making this change for WebKitWebView

Xan Lopez

Comment 2 2010-09-10 10:57:48 PDT

Comment on attachment 67200 [details] Patch making this change for WebKitWebView >- if (priv->tooltipText) { >- gtk_tooltip_set_text(tooltip, priv->tooltipText); >+ if (priv->tooltipText.length() > 0) { Mmm, should use isEmpty() for consistency sake? >+ gtk_tooltip_set_text(tooltip, priv->tooltipText.data()); > return TRUE; > } > Looks great otherwise.

Martin Robinson

Comment 3 2010-09-10 11:02:35 PDT

(In reply to comment #2) Thanks for the review! > Mmm, should use isEmpty() for consistency sake? CString doesn't have an isEmpty unfortunately. I could see it being useful though. Perhaps I'll propose that in another patch.

Martin Robinson

Comment 4 2010-09-10 11:05:05 PDT

Comment on attachment 67200 [details] Patch making this change for WebKitWebView Clearing flags on attachment: 67200 Committed r67215: <http://trac.webkit.org/changeset/67215>

Martin Robinson

Comment 5 2010-09-10 11:05:09 PDT

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 6 2010-09-10 11:42:49 PDT

http://trac.webkit.org/changeset/67215 might have broken Chromium Mac Release The following changes are on the blame list: http://trac.webkit.org/changeset/67216 http://trac.webkit.org/changeset/67215 http://trac.webkit.org/changeset/67205 http://trac.webkit.org/changeset/67206 http://trac.webkit.org/changeset/67207

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