Bug 94808 – [EFL][WK2] Free ewk context data on program exit.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit EFL

[Kangil Han]

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.

[Kangil Han]

Created attachment 160153 [details]
patch

[Ryuan Choi]

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.

[Chris Dumez]

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.

[Chris Dumez]

Please see Bug 89186.

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 160179 [details]
Patch

New proposal.

[Ryuan Choi]

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;
 }

[Chris Dumez]

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).

[Chris Dumez]

Created attachment 160186 [details]
Patch

[WebKit Review Bot]

Comment on attachment 160186 [details]
Patch

Clearing flags on attachment: 160186

Committed r126448: <http://trac.webkit.org/changeset/126448>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Kangil Han]

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!

[Gyuyoung Kim]

(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.

[Gyuyoung Kim]

(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

[Kangil Han]

(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!