Bug 118568 – Use GOwnPtr in PluginProcessProxyUnix to manage stdOut variable

• Component: New Bugs
• Status: RESOLVED FIXED

[Sergio Correia (qrwteyrutiyoup)]

[WK2][UNIX] Free standard output obtained from g_spawn_sync() with g_free

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 206471 [details]
Patch

[Martin Robinson]

Comment on attachment 206471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206471&action=review

I'm not sure that g_free is necessarily better than free, but a GOwnptr probably makes sense. :)

> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:73
> -    char* stdOut = 0;
> +    gchar* stdOut = 0;

Why is this necessary? They are the same type and in WebKit code we typically use the non-Glib versions.

> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:91
> +        g_free(stdOut);

Why not switch to using GOwnPtr here?

[Sergio Correia (qrwteyrutiyoup)]

(In reply to comment #2)
> (From update of attachment 206471 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=206471&action=review
> 
> I'm not sure that g_free is necessarily better than free, but a GOwnptr probably makes sense. :)
> 
> > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:73
> > -    char* stdOut = 0;
> > +    gchar* stdOut = 0;
> 
> Why is this necessary? They are the same type and in WebKit code we typically use the non-Glib versions.
>

Good point. We should keep using char, then.
 
> > Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:91
> > +        g_free(stdOut);
> 
> Why not switch to using GOwnPtr here?

Yeah, I thought of using it, but since this code is also shared with EFL, I wasn't sure it would be okay. I am going to upload an updated version using GOwnPtr. Thanks for the review.

[Chris Dumez]

Comment on attachment 206471 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206471&action=review

>>> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:91
>>> +        g_free(stdOut);
>> 
>> Why not switch to using GOwnPtr here?
> 
> Yeah, I thought of using it, but since this code is also shared with EFL, I wasn't sure it would be okay. I am going to upload an updated version using GOwnPtr. Thanks for the review.

Yes, it is fine for the EFL port. This file uses glib already (e.g. g_spawn_sync()).

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 206474 [details]
Patch

[Martin Robinson]

Comment on attachment 206474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206474&action=review

Looks good except for the include line.

> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:39
> +#include <GOwnPtr.h>

I think you want <wtf/gobject/GOwnPr.h> here.

[Sergio Correia (qrwteyrutiyoup)]

Comment on attachment 206474 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=206474&action=review

>> Source/WebKit2/UIProcess/Plugins/unix/PluginProcessProxyUnix.cpp:39
>> +#include <GOwnPtr.h>
> 
> I think you want <wtf/gobject/GOwnPr.h> here.

My bad, this time I did only an EFL build here. I am finishing the Gtk build with the include fix and once it finishes I will reupload and ask for cq. Thanks.

[Sergio Correia (qrwteyrutiyoup)]

Created attachment 206476 [details]
Patch

Include fixed.

[Build Bot]

Comment on attachment 206474 [details]
Patch

Attachment 206474 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/962174

New failing tests:
svg/batik/masking/maskRegions.svg

[Build Bot]

Created attachment 206478 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[WebKit Commit Bot]

Comment on attachment 206476 [details]
Patch

Clearing flags on attachment: 206476

Committed r152575: <http://trac.webkit.org/changeset/152575>