Bug 94808

Summary: [EFL][WK2] Free ewk context data on program exit.
Product: WebKit Reporter: Kangil Han <kangil.han>
Component: WebKit EFLAssignee: Chris Dumez <cdumez>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, enmi.lee, gyuyoung.kim, kenneth, lucas.de.marchi, naginenis, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Linux   
Bug Depends on:    
Bug Blocks: 94697    
Attachments:
Description Flags
patch
none
Patch
none
Patch
none
Patch none

Description Kangil Han 2012-08-23 06:59:43 PDT
Current implementation doesn't delete ewk context data.
This patch fixed it by adding Ewk_Context to Ewk_View_Private_Data and deleting it explicitly when private data is deleted.
Comment 1 Kangil Han 2012-08-23 07:06:41 PDT
Created attachment 160153 [details]
patch
Comment 2 Ryuan Choi 2012-08-23 07:30:29 PDT
Comment on attachment 160153 [details]
patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:118
> +        ewk_context_free(context);

I think that it is wrong because we can remove two ewk_views which use default context.
Comment 3 Chris Dumez 2012-08-23 07:39:44 PDT
Comment on attachment 160153 [details]
patch

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

>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:118
>> +        ewk_context_free(context);
> 
> I think that it is wrong because we can remove two ewk_views which use default context.

Yes. eunmi already has a patch to make Ewk_Context ref counted and solve this problem.
Comment 4 Chris Dumez 2012-08-23 07:42:10 PDT
Please see Bug 89186.
Comment 5 Chris Dumez 2012-08-23 07:49:27 PDT
Comment on attachment 160153 [details]
patch

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

>>> Source/WebKit2/UIProcess/API/efl/ewk_view.cpp:118
>>> +        ewk_context_free(context);
>> 
>> I think that it is wrong because we can remove two ewk_views which use default context.
> 
> Yes. eunmi already has a patch to make Ewk_Context ref counted and solve this problem.

Also, Ewk_Context is currently a static variable returned by ewk_context_default_get() so you cannot free it like this.
Comment 6 Chris Dumez 2012-08-23 08:09:01 PDT
This is not really a leak. We use singleton pattern so the variable lives as long as the program. The memory will get freed by the OS on exit.
Comment 7 Chris Dumez 2012-08-23 09:03:43 PDT
Created attachment 160179 [details]
Patch

New proposal.
Comment 8 Ryuan Choi 2012-08-23 09:09:15 PDT
Comment on attachment 160179 [details]
Patch

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

> Source/WebKit2/UIProcess/API/efl/ewk_context.cpp:214
> -    static Ewk_Context* defaultContext = createDefaultEwkContext();
> +    if (!defaultContext)
> +        defaultContext = new Ewk_Context(adoptWK(WKContextCreate()));
>  
>      return defaultContext;
>  }
>  
> +void ewk_context_default_free()
> +{
> +    delete defaultContext;
> +    defaultContext = 0;

Are there any issue if we just change like below ?
(might be DEFINE_STATIC_LOCAL)

 Ewk_Context* ewk_context_default_get()
 {
-    static Ewk_Context* defaultContext = createDefaultEwkContext();
+    static Ewk_Context defaultContext(WKContextCreate());
 
-    return defaultContext;
+    return &defaultContext;
 }
Comment 9 Chris Dumez 2012-08-23 09:41:23 PDT
Created attachment 160185 [details]
Patch

Take ryuan's feedback into consideration.

Note that I cannot use DEFINE_STATIC_LOCAL because of the way it is defined (it calls "new" internally).
Comment 10 Chris Dumez 2012-08-23 09:46:01 PDT
Created attachment 160186 [details]
Patch
Comment 11 WebKit Review Bot 2012-08-23 11:02:12 PDT
Comment on attachment 160186 [details]
Patch

Clearing flags on attachment: 160186

Committed r126448: <http://trac.webkit.org/changeset/126448>
Comment 12 WebKit Review Bot 2012-08-23 11:02:16 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Kangil Han 2012-08-23 18:13:59 PDT
I feel really really bad why chris landed this patch without asking my opinion. He had to review my patch and let me finish this!
Comment 14 Gyuyoung Kim 2012-08-23 19:07:13 PDT
(In reply to comment #13)
> I feel really really bad why chris landed this patch without asking my opinion. He had to review my patch and let me finish this!

Was there any discussion ? I don't barely see other contributor landed a patch without asking original patch owner.
Comment 15 Gyuyoung Kim 2012-08-23 19:11:34 PDT
(In reply to comment #14)

> Was there any discussion ? I don't barely see other contributor landed a patch without asking original patch owner.

Oops, typo. s/don't barely/barely/g
Comment 16 Kangil Han 2012-08-23 19:33:51 PDT
(In reply to comment #15)
> (In reply to comment #14)
> 
> > Was there any discussion ? I don't barely see other contributor landed a patch without asking original patch owner.
> 
> Oops, typo. s/don't barely/barely/g

Nope, chris never asked me about landing his code on my bug.
Source code diff might be small, but the process on detection was really hard stuff. That's why I am so angry at him!