Bug 83004 – [Plugins] delete ws_info regardless of window mode

• Status: RESOLVED
• Resolution: FIXED
• Component: Plug-ins

[Kwangmin Bang]

In UNIX port(Gtk/QT/Brackberry), m_npWindow.ws_info = new NPSetWindowCallbackStruct; is called in PluginView::platformStart().
each platform does not delete this object(m_npWindow.ws_info), and it is done in PluginView:stop().

However,
If m_isWindowed is false (it means windowless mode), it's not deleted.

m_npWindow.ws_info should be deleted even if plugin run as windowless mode to remove memory leak.


Every plugin works well after this patch without any crash.

[Kwangmin Bang]

Created attachment 135281 [details]
[UNIX Plugins] delete ws_info, remove memory leak

patch.

it's simple. i just removed a condition in the IF statement.

[Kwangmin Bang]

Created attachment 137267 [details]
[UNIX Plugins] delete ws_info, remove memory leak

rebase
and change casting code to C++ style.

[Simon Hausmann]

Comment on attachment 135281 [details]
[UNIX Plugins] delete ws_info, remove memory leak

Mark this as obsolete because there's a second patch for review.

[Simon Hausmann]

Comment on attachment 137267 [details]
[UNIX Plugins] delete ws_info, remove memory leak

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

Looks good in general, but I think you can remove the if() altogether and there are a few language nitpicks :)

> Source/WebCore/ChangeLog:3
> +        [Plugins] delete ws_info regardless window mode.

"regardless window mode" -> "regardless of the window mode"

> Source/WebCore/ChangeLog:10
> +        Additionally, change casting synta to C++ style.

synta -> syntax

> Source/WebCore/plugins/PluginView.cpp:370
> +    if (m_npWindow.ws_info)
> +           delete static_cast<NPSetWindowCallbackStruct *>(m_npWindow.ws_info);

I don't think the if() is needed at all. delete 0 is perfectly valid C++.

[Kwangmin Bang]

Created attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

change the title.
revise wrong word.
remove unneeded if().

[Kwangmin Bang]

Comment on attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

mark this as obsolete

[Kwangmin Bang]

Comment on attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

sorry for my mistake

[Kwangmin Bang]

Comment on attachment 137267 [details]
[UNIX Plugins] delete ws_info, remove memory leak

>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 1863a58..2b59ef9 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,17 @@
>[phone]  Bang Kwang min  <[e-mail]>
>+
>+        [Plugins] delete ws_info regardless of window mode.
>+        https://bugs.webkit.org/show_bug.cgi?id=83004
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        m_npWindow.ws_info should be deleted even if plugin run as windowless mode to remove memory leak.
>+        Every plugin works well after this patch without any crash.
>+        Additionally, change casting syntax to C++ style.
>+
>+        * plugins/PluginView.cpp:
>+        (WebCore::PluginView::stop):
>+
> 2012-04-15  James Robinson  <[e-mail]>
> 
>         [chromium] LayerRendererChromium shouldn't know anything about CCLayerImpl
>diff --git a/Source/WebCore/plugins/PluginView.cpp b/Source/WebCore/plugins/PluginView.cpp
>index 749f609..389a8fe 100644
>--- a/Source/WebCore/plugins/PluginView.cpp
>+++ b/Source/WebCore/plugins/PluginView.cpp
>@@ -366,8 +366,8 @@ void PluginView::stop()
>     }
> 
> #ifdef XP_UNIX
>-    if (m_isWindowed && m_npWindow.ws_info)
>-           delete (NPSetWindowCallbackStruct *)m_npWindow.ws_info;
>+    if (m_npWindow.ws_info)
>+           delete static_cast<NPSetWindowCallbackStruct *>(m_npWindow.ws_info);
>     m_npWindow.ws_info = 0;
> #endif
>

[Kwangmin Bang]

Comment on attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

as i think, delete 0 is the problem
so, please review previous patch. i modified the title and comment.

[Kwangmin Bang]

Created attachment 137307 [details]
[UNIX Plugins] delete ws_info, remove memory leak

rebase.

I'm sorry i'm not accustomed to manage the attachment.


-
if remove if(), it does not pass qt port test.

[Simon Hausmann]

(In reply to comment #10)
> Created an attachment (id=137307) [details]
> [UNIX Plugins] delete ws_info, remove memory leak
> 
> rebase.
> 
> I'm sorry i'm not accustomed to manage the attachment.

No worries, it seems you got the hang of it :)
 
> -
> if remove if(), it does not pass qt port test.

Can you elaborate a bit on that?

[Simon Hausmann]

(In reply to comment #9)
> (From update of attachment 137298 [details])
> as i think, delete 0 is the problem

See also http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.8

[Kwangmin Bang]

(In reply to comment #12)
> (In reply to comment #9)
> > (From update of attachment 137298 [details] [details])
> > as i think, delete 0 is the problem
> 
> See also http://www.parashift.com/c++-faq-lite/freestore-mgmt.html#faq-16.8

I agree with you, and i has uploaded the patch, but it failed to pass test.
please see attachment id 137298. you can see qt, qt-wk2 and efl are pink color.

To know why it failed, i'm trying to build QT.

[Kwangmin Bang]

Created attachment 137433 [details]
delete without null checking.

delete without null checking. you can say that.

[Kwangmin Bang]

*** Bug 84021 has been marked as a duplicate of this bug. ***

[Simon Hausmann]

Comment on attachment 137433 [details]
delete without null checking.

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

> Source/WebCore/plugins/PluginView.cpp:369
> +    delete static_cast<NPSetWindowCallbackStruct *>(m_npWindow.ws_info);

Small coding style glitch, space after *. I'll fix it when landing.

[Simon Hausmann]

Committed r114358: <http://trac.webkit.org/changeset/114358>