Bug 83004

Summary: [Plugins] delete ws_info regardless of window mode
Product: WebKit Reporter: Kwangmin Bang <justine.bang>
Component: Plug-insAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: hausmann, jmalonzo, vestbo
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: Linux   
Attachments:
Description Flags
[UNIX Plugins] delete ws_info, remove memory leak
none
[UNIX Plugins] delete ws_info, remove memory leak
none
[UNIX Plugins] delete ws_info, remove memory leak
none
[UNIX Plugins] delete ws_info, remove memory leak
none
delete without null checking. hausmann: review+, hausmann: commit-queue-

Description Kwangmin Bang 2012-04-02 23:44:45 PDT
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.
Comment 1 Kwangmin Bang 2012-04-02 23:48:19 PDT
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.
Comment 2 Kwangmin Bang 2012-04-15 20:03:36 PDT
Created attachment 137267 [details]
[UNIX Plugins] delete ws_info, remove memory leak

rebase
and change casting code to C++ style.
Comment 3 Simon Hausmann 2012-04-16 01:04:53 PDT
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.
Comment 4 Simon Hausmann 2012-04-16 01:07:38 PDT
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++.
Comment 5 Kwangmin Bang 2012-04-16 01:30:08 PDT
Created attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

change the title.
revise wrong word.
remove unneeded if().
Comment 6 Kwangmin Bang 2012-04-16 01:33:59 PDT
Comment on attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

mark this as obsolete
Comment 7 Kwangmin Bang 2012-04-16 01:38:57 PDT
Comment on attachment 137298 [details]
[UNIX Plugins] delete ws_info, remove memory leak

sorry for my mistake
Comment 8 Kwangmin Bang 2012-04-16 02:01:08 PDT
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 @@
>+2012-04-15  Bang Kwang min  <justine.bang@samsung.com>
>+
>+        [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  <jamesr@chromium.org>
> 
>         [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
>
Comment 9 Kwangmin Bang 2012-04-16 02:02:14 PDT
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.
Comment 10 Kwangmin Bang 2012-04-16 02:12:41 PDT
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.
Comment 11 Simon Hausmann 2012-04-16 02:30:05 PDT
(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?
Comment 12 Simon Hausmann 2012-04-16 03:48:10 PDT
(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
Comment 13 Kwangmin Bang 2012-04-16 04:52:24 PDT
(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.
Comment 14 Kwangmin Bang 2012-04-16 17:04:14 PDT
Created attachment 137433 [details]
delete without null checking.

delete without null checking. you can say that.
Comment 15 Kwangmin Bang 2012-04-16 17:06:16 PDT
*** Bug 84021 has been marked as a duplicate of this bug. ***
Comment 16 Simon Hausmann 2012-04-17 00:46:06 PDT
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.
Comment 17 Simon Hausmann 2012-04-17 00:47:52 PDT
Committed r114358: <http://trac.webkit.org/changeset/114358>