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.
Created attachment 160153 [details] patch
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 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.
Please see Bug 89186.
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.
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.
Created attachment 160179 [details] Patch New proposal.
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; }
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).
Created attachment 160186 [details] Patch
Comment on attachment 160186 [details] Patch Clearing flags on attachment: 160186 Committed r126448: <http://trac.webkit.org/changeset/126448>
All reviewed patches have been landed. Closing bug.
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!
(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.
(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
(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!