Summary: | [EFL][WK2] Free ewk context data on program exit. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Kangil Han <kangil.han> | ||||||||||
Component: | WebKit EFL | Assignee: | 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
Kangil Han
2012-08-23 06:59:43 PDT
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. 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! |