From c054ac3d4df7fe8b2f58204e677f275bd8f0e60c Mon Sep 17 00:00:00 2001 From: Dan Gudmundsson Date: Wed, 30 Oct 2013 16:46:54 +0100 Subject: wx: Fix cleanup of event handlers Event handlers was not removed after objects/process had been delete/died, which causes memory leakage and that fun's was kept in the wx_server process. Code that might be purged and the server died. --- lib/wx/api_gen/wx_extra/wxEvtHandler.c_src | 14 ++-- lib/wx/api_gen/wx_gen_cpp.erl | 2 +- lib/wx/c_src/gen/wxe_events.cpp | 2 +- lib/wx/c_src/gen/wxe_funcs.cpp | 14 ++-- lib/wx/c_src/wxe_events.h | 39 +++++----- lib/wx/c_src/wxe_impl.cpp | 20 ++++- lib/wx/src/wxe_server.erl | 118 +++++++++++++++++++---------- lib/wx/test/wx_event_SUITE.erl | 77 ++++++++++++++++++- lib/wx/test/wx_test_lib.erl | 20 ++--- 9 files changed, 218 insertions(+), 88 deletions(-) (limited to 'lib/wx') diff --git a/lib/wx/api_gen/wx_extra/wxEvtHandler.c_src b/lib/wx/api_gen/wx_extra/wxEvtHandler.c_src index 4e492db045..9c5f46b253 100644 --- a/lib/wx/api_gen/wx_extra/wxEvtHandler.c_src +++ b/lib/wx/api_gen/wx_extra/wxEvtHandler.c_src @@ -29,13 +29,13 @@ case 100: { // wxEvtHandler::Connect int eventType = wxeEventTypeFromAtom(bp); bp += *eventTypeLen; char *class_name = bp; bp+= *class_nameLen; if(eventType > 0 ) { - wxeCallbackData * Evt_cb = new wxeCallbackData(Ecmd.caller,(void *) This, - class_name,*fun_cb, - *skip, userData); - This->Connect((int) *winid,(int) *lastId,eventType, - (wxObjectEventFunction)(wxEventFunction) &wxeEvtListener::forward, - Evt_cb, Listener); - rt.addAtom("ok"); + wxeCallbackData * Evt_cb = new wxeCallbackData(Ecmd.caller,getRef(This, memenv), + class_name,*fun_cb, + *skip, userData, Listener); + This->Connect((int) *winid,(int) *lastId,eventType, + (wxObjectEventFunction)(wxEventFunction) &wxeEvtListener::forward, + Evt_cb, Listener); + rt.addAtom("ok"); } else { rt.addAtom("badarg"); rt.addAtom("event_type"); diff --git a/lib/wx/api_gen/wx_gen_cpp.erl b/lib/wx/api_gen/wx_gen_cpp.erl index f00fc0c720..7e35ebfa83 100644 --- a/lib/wx/api_gen/wx_gen_cpp.erl +++ b/lib/wx/api_gen/wx_gen_cpp.erl @@ -1223,7 +1223,7 @@ encode_events(Evs) -> w("~n rt.addAtom((char*)\"wx\");~n" " rt.addInt((int) event->GetId());~n" - " rt.addRef(getRef((void *)(cb->obj), memenv), cb->class_name);~n" + " rt.addRef(cb->obj, cb->class_name);~n" " rt.addExt2Term(cb->user_data);~n"), w(" switch(Etype->cID) {~n"), diff --git a/lib/wx/c_src/gen/wxe_events.cpp b/lib/wx/c_src/gen/wxe_events.cpp index a6a37cb37f..fb3a065448 100644 --- a/lib/wx/c_src/gen/wxe_events.cpp +++ b/lib/wx/c_src/gen/wxe_events.cpp @@ -346,7 +346,7 @@ bool sendevent(wxEvent *event, ErlDrvTermData port) rt.addAtom((char*)"wx"); rt.addInt((int) event->GetId()); - rt.addRef(getRef((void *)(cb->obj), memenv), cb->class_name); + rt.addRef(cb->obj, cb->class_name); rt.addExt2Term(cb->user_data); switch(Etype->cID) { case 164: {// wxCommandEvent diff --git a/lib/wx/c_src/gen/wxe_funcs.cpp b/lib/wx/c_src/gen/wxe_funcs.cpp index 2d8dbb242b..ae8b1fd8b1 100644 --- a/lib/wx/c_src/gen/wxe_funcs.cpp +++ b/lib/wx/c_src/gen/wxe_funcs.cpp @@ -93,13 +93,13 @@ case 100: { // wxEvtHandler::Connect int eventType = wxeEventTypeFromAtom(bp); bp += *eventTypeLen; char *class_name = bp; bp+= *class_nameLen; if(eventType > 0 ) { - wxeCallbackData * Evt_cb = new wxeCallbackData(Ecmd.caller,(void *) This, - class_name,*fun_cb, - *skip, userData); - This->Connect((int) *winid,(int) *lastId,eventType, - (wxObjectEventFunction)(wxEventFunction) &wxeEvtListener::forward, - Evt_cb, Listener); - rt.addAtom("ok"); + wxeCallbackData * Evt_cb = new wxeCallbackData(Ecmd.caller,getRef(This, memenv), + class_name,*fun_cb, + *skip, userData, Listener); + This->Connect((int) *winid,(int) *lastId,eventType, + (wxObjectEventFunction)(wxEventFunction) &wxeEvtListener::forward, + Evt_cb, Listener); + rt.addAtom("ok"); } else { rt.addAtom("badarg"); rt.addAtom("event_type"); diff --git a/lib/wx/c_src/wxe_events.h b/lib/wx/c_src/wxe_events.h index 6bbb0dfa13..718e0ad120 100644 --- a/lib/wx/c_src/wxe_events.h +++ b/lib/wx/c_src/wxe_events.h @@ -30,6 +30,20 @@ public: int cID; }; +/* One EvtListener per listening erlang process */ +/* If callbacks are used the receiver is wxe_master process */ +/* and a wxeEvtListener pre callback is registered */ +class wxeEvtListener : public wxEvtHandler +{ + public: + wxeEvtListener(ErlDrvTermData Thisport) : port(Thisport) + {} + // {fprintf(stderr, "Creating %x\r\n", (unsigned int) this); fflush(stderr);} + ~wxeEvtListener() {} + void forward(wxEvent& event); + ErlDrvTermData port; +}; + void initEventTable(); int wxeEventTypeFromAtom(char *etype_atom); @@ -37,32 +51,17 @@ int wxeEventTypeFromAtom(char *etype_atom); class wxeCallbackData : public wxObject { public: - wxeCallbackData(ErlDrvTermData caller, void *req, char *req_type, - int funcb, int skip_ev, wxeErlTerm * userData); + wxeCallbackData(ErlDrvTermData caller, int req, char *req_type, + int funcb, int skip_ev, wxeErlTerm * userData, + wxeEvtListener *handler_cb); ~wxeCallbackData(); + wxeEvtListener * handler; ErlDrvTermData listener; int fun_id; - void * obj; + int obj; char class_name[40]; int skip; wxeErlTerm * user_data; }; -/* One EvtListener per listening erlang process */ -/* If callbacks are used the receiver is wxe_master process */ -/* and a wxeEvtListener pre callback is registered */ -class wxeEvtListener : public wxEvtHandler -{ -public: - wxeEvtListener(ErlDrvTermData Thisport) : port(Thisport) - {} - // {fprintf(stderr, "Creating %x\r\n", (unsigned int) this); fflush(stderr);} - void forward(wxEvent& event); - ~wxeEvtListener() { - ((WxeApp *)wxTheApp)->clearPtr(this); - // fprintf(stderr, "Deleteing %x\r\n", (unsigned int) this); fflush(stderr); - }; - ErlDrvTermData port; -}; - #endif diff --git a/lib/wx/c_src/wxe_impl.cpp b/lib/wx/c_src/wxe_impl.cpp index 8adec4be07..4968075659 100644 --- a/lib/wx/c_src/wxe_impl.cpp +++ b/lib/wx/c_src/wxe_impl.cpp @@ -880,8 +880,9 @@ wxETreeItemData::~wxETreeItemData() * CallbackData * * ****************************************************************************/ -wxeCallbackData::wxeCallbackData(ErlDrvTermData caller,void * req, char *req_type, - int funcb, int skip_ev, wxeErlTerm * userData) +wxeCallbackData::wxeCallbackData(ErlDrvTermData caller, int req, char *req_type, + int funcb, int skip_ev, wxeErlTerm * userData, + wxeEvtListener *handler_cb) : wxObject() { listener = caller; @@ -890,13 +891,26 @@ wxeCallbackData::wxeCallbackData(ErlDrvTermData caller,void * req, char *req_typ strcpy(class_name, req_type); skip = skip_ev; user_data = userData; + handler = handler_cb; } wxeCallbackData::~wxeCallbackData() { - // fprintf(stderr, "CBD Deleteing %x %s\r\n", (unsigned int) this, class_name); fflush(stderr); + // fprintf(stderr, "CBD Deleteing %p %s\r\n", this, class_name); fflush(stderr); if(user_data) { delete user_data; } + ptrMap::iterator it; + it = ((WxeApp *)wxTheApp)->ptr2ref.find(handler); + if(it != ((WxeApp *)wxTheApp)->ptr2ref.end()) { + wxeRefData *refd = it->second; + wxeReturn rt = wxeReturn(WXE_DRV_PORT, refd->memenv->owner, false); + rt.addAtom("wx_delete_cb"); + rt.addInt(fun_id); + rt.addRef(refd->ref, "wxeEvtListener"); + rt.addRef(obj, class_name); + rt.addTupleCount(4); + rt.send(); + } } /* **************************************************************************** diff --git a/lib/wx/src/wxe_server.erl b/lib/wx/src/wxe_server.erl index a8604c76b9..45184a3bc8 100644 --- a/lib/wx/src/wxe_server.erl +++ b/lib/wx/src/wxe_server.erl @@ -36,7 +36,7 @@ terminate/2, code_change/3]). -record(state, {port,cb_port,users,cleaners=[],cb,cb_cnt}). --record(user, {objects=[], events=[], evt_handler}). +-record(user, {events=[], evt_handler}). -record(event, {object, callback, cb_handler}). -define(APPLICATION, wxe). @@ -157,26 +157,40 @@ handle_cast(_Msg, State) -> handle_info(Cb = {_, _, '_wx_invoke_cb_'}, State) -> invoke_cb(Cb, State), {noreply, State}; -handle_info({wx_delete_cb, FunId}, State0 = #state{cb=CB}) when is_integer(FunId) -> - case get(FunId) of - undefined -> - {noreply, State0}; - Fun -> - erase(FunId), - {noreply, State0#state{cb=gb_trees:delete(Fun, CB)}} + +handle_info({wx_delete_cb, FunId}, State) + when is_integer(FunId) -> + {noreply, delete_fun(FunId, State)}; + +handle_info({wx_delete_cb, Id, EvtListener, Obj}, State = #state{users=Users}) -> + From = erase(EvtListener), + case gb_trees:lookup(From, Users) of + none -> + {noreply, delete_fun(Id, State)}; + {value, User0} -> + User = cleanup_evt_listener(User0, EvtListener, Obj), + {noreply, delete_fun(Id, State#state{users=gb_trees:update(From, User, Users)})} end; + handle_info({'DOWN',_,process,Pid,_}, State=#state{users=Users0,cleaners=Cs}) -> try User = gb_trees:get(Pid,Users0), Users = gb_trees:delete(Pid,Users0), Env = wx:get_env(), - Cleaner = spawn_link(fun() -> cleanup(Env,Pid,[User]) end), - {noreply, State#state{users=Users,cleaners=[Cleaner|Cs]}} + case User of + #user{events=[], evt_handler=undefined} -> %% No need to spawn + {noreply, State#state{users=Users,cleaners=Cs}}; + _ -> + Cleaner = spawn_link(fun() -> cleanup(Env,Pid,[User]) end), + {noreply, State#state{users=Users,cleaners=[Cleaner|Cs]}} + end catch _E:_R -> %% ?log("Error: ~p ~p", [_E,_R]), {noreply, State} end; -handle_info(Msg = {'_wxe_destroy_', Pid}, State) -> + +handle_info(Msg = {'_wxe_destroy_', Pid}, State) + when is_pid(Pid) -> case erlang:is_process_alive(Pid) of true -> Pid ! Msg, @@ -185,6 +199,7 @@ handle_info(Msg = {'_wxe_destroy_', Pid}, State) -> ok end, {noreply, State}; + handle_info(_Info, State) -> ?log("Unknown message ~p sent to ~p~n",[_Info, ?MODULE]), {noreply, State}. @@ -212,10 +227,10 @@ handle_connect(Object, EvData, From, State0 = #state{users=Users}) -> CBHandler = Handler0, Handler = Handler0; undefined when Callback =:= 0 -> - Handler = new_evt_listener(State0), + Handler = new_evt_listener(State0, From), CBHandler = Handler; _ -> - CBHandler = new_evt_listener(State0), + CBHandler = new_evt_listener(State0, From), Handler = Handler0 end, Evs = [#event{object=Object,callback=Callback, cb_handler=CBHandler}|Evs0], @@ -238,9 +253,9 @@ handle_connect(Object, EvData, From, State0 = #state{users=Users}) -> invoke_cb({{Ev=#wx{}, Ref=#wx_ref{}}, FunId,_}, _S) -> %% Event callbacks case get(FunId) of - Fun when is_function(Fun) -> + {Fun,_} when is_function(Fun) -> invoke_callback(fun() -> Fun(Ev, Ref), <<>> end); - Pid when is_pid(Pid) -> %% wx_object sync event + {Pid,_} when is_pid(Pid) -> %% wx_object sync event invoke_callback(Pid, Ev, Ref); Err -> ?log("Internal Error ~p~n",[Err]) @@ -248,7 +263,7 @@ invoke_cb({{Ev=#wx{}, Ref=#wx_ref{}}, FunId,_}, _S) -> invoke_cb({FunId, Args, _}, _S) when is_list(Args), is_integer(FunId) -> %% Overloaded functions case get(FunId) of - Fun when is_function(Fun) -> + {Fun,_} when is_function(Fun) -> invoke_callback(fun() -> Fun(Args) end); Err -> ?log("Internal Error ~p ~p ~p~n",[Err, FunId, Args]) @@ -311,10 +326,12 @@ get_wx_object_state(Pid) -> _ -> ignore end. -new_evt_listener(State) -> +new_evt_listener(State, Owner) -> #wx_env{port=Port} = wx:get_env(), _ = erlang:port_control(Port,98,<<>>), - get_result(State). + Listener = get_result(State), + put(Listener, Owner), + Listener. get_result(_State) -> receive @@ -326,43 +343,66 @@ get_result(_State) -> attach_fun(Fun, S = #state{cb=CB,cb_cnt=Next}) -> case gb_trees:lookup(Fun,CB) of {value, ID} -> + {Fun, N} = get(ID), + put(ID, {Fun,N+1}), {ID,S}; none -> - put(Next,Fun), + put(Next,{Fun, 1}), {Next,S#state{cb=gb_trees:insert(Fun,Next,CB),cb_cnt=Next+1}} end. +delete_fun(0, State) -> State; +delete_fun(FunId, State = #state{cb=CB}) -> + case get(FunId) of + undefined -> + State; + {Fun,N} when N < 2 -> + erase(FunId), + State#state{cb=gb_trees:delete(Fun, CB)}; + {Fun,N} -> + put(FunId, {Fun, N-1}), + State + end. + +cleanup_evt_listener(U=#user{events=Evs0,evt_handler=Handler}, EvtListener, Object) -> + {Evs, Del} = lists:foldl(fun(#event{object=Obj,cb_handler=CBH}, {Acc, Delete}) + when CBH =:= EvtListener, Obj =:= Object -> + {Acc, Delete}; + (Event = #event{cb_handler=CBH}, {Acc, _Delete}) + when CBH =:= EvtListener -> + {[Event|Acc], false}; + (Event, {Acc, Delete}) -> + {[Event|Acc], Delete} + end, {[], true}, Evs0), + Del andalso wxEvtHandler:destroy_evt_listener(EvtListener), + case Del andalso Handler =:= EvtListener of + true -> + U#user{events=Evs, evt_handler=undefined}; + false -> + U#user{events=Evs} + end. + handle_disconnect(Object, Evh, From, State0 = #state{users=Users0}) -> - User0 = #user{events=Evs0, evt_handler=PidH} = gb_trees:get(From, Users0), + #user{events=Evs0} = gb_trees:get(From, Users0), Fun = wxEvtHandler:get_callback(Evh), case find_handler(Evs0, Object, Fun) of - [] -> - {reply, false, State0}; + [] -> {reply, false, State0}; Handlers -> case disconnect(Object,Evh, Handlers) of - Ev = #event{callback=CB, cb_handler=Handler} -> - case is_function(CB) of - true -> wxEvtHandler:destroy_evt_listener(Handler); - false -> ignore - end, - User = case lists:delete(Ev,Evs0) of - [] when PidH =/= undefined -> - wxEvtHandler:destroy_evt_listener(PidH), - User0#user{events=[], evt_handler=undefined}; - Evs -> - User0#user{events=Evs} - end, - {reply, true, State0#state{users=gb_trees:update(From,User,Users0)}}; + #event{} -> + {reply, true, State0}; Result -> {reply, Result, State0} end end. disconnect(Object,Evh,[Ev=#event{cb_handler=Handler}|Evs]) -> - case wxEvtHandler:disconnect_impl(Handler,Object,Evh) of + try wxEvtHandler:disconnect_impl(Handler,Object,Evh) of true -> Ev; false -> disconnect(Object, Evh, Evs); Error -> Error + catch _:_ -> + false end; disconnect(_, _, []) -> false. @@ -393,9 +433,9 @@ cleanup(Env, _Pid, Data) -> gen_server:cast(Env#wx_env.sv, {cleaned, self()}), normal. -cleanup(#user{objects=_Os,events=Evs, evt_handler=Handler}) -> - lists:foreach(fun(#event{object=_O, callback=CB, cb_handler=CbH}) -> - %%catch wxEvtHandler:disconnect_impl(CbH,O), +cleanup(#user{events=Evs, evt_handler=Handler}) -> + lists:foreach(fun(#event{object=O, callback=CB, cb_handler=CbH}) -> + catch wxEvtHandler:disconnect_impl(CbH,O), case is_function(CB) of true -> wxEvtHandler:destroy_evt_listener(CbH); diff --git a/lib/wx/test/wx_event_SUITE.erl b/lib/wx/test/wx_event_SUITE.erl index f19adb430d..6a9f19ad51 100644 --- a/lib/wx/test/wx_event_SUITE.erl +++ b/lib/wx/test/wx_event_SUITE.erl @@ -48,7 +48,7 @@ suite() -> [{ct_hooks,[ts_install_cth]}]. all() -> [connect, disconnect, connect_msg_20, connect_cb_20, mouse_on_grid, spin_event, connect_in_callback, recursive, - char_events + char_events, callback_clean ]. groups() -> @@ -420,3 +420,78 @@ char_events(Config) -> wx_test_lib:flush(), wx_test_lib:wx_destroy(Frame, Config). + +callback_clean(TestInfo) when is_atom(TestInfo) -> wx_test_lib:tc_info(TestInfo); +callback_clean(Config) -> + %% Be sure that event handling are cleanup up correctly and don't keep references to old + %% fun's and event listeners + Wx = wx:new(), + Frame = wxFrame:new(Wx, ?wxID_ANY, "Frame Window"), + wxFrame:show(Frame), + + %% wx:debug([verbose,driver]), + Dlg = wxDialog:new(Frame, ?wxID_ANY, "Testing"), + Panel = wxPanel:new(Dlg, []), + Sizer = wxBoxSizer:new(?wxVERTICAL), + Button = wxButton:new(Panel, 600, [{label, "Foobar"}]), + wxSizer:add(Sizer, Button, [{proportion,1}, {flag, ?wxEXPAND}]), + wxSizer:add(Sizer, wxDialog:createStdDialogButtonSizer(Dlg,?wxOK bor ?wxCANCEL)), + wxDialog:setSizerAndFit(Dlg, Sizer), + + Env = wx:get_env(), + SetupEventHandlers = + fun() -> + wx:set_env(Env), + Me = self(), + Print = fun(#wx{id=ID, event=#wxCommand{}},Ev) -> + io:format("~p Clicked ~p~n", [self(), ID]), + Me ! #wx{event=#wxClose{}}, + wxEvent:skip(Ev, [{skip, true}]); + (#wx{id=ID, event=#wxClose{}},Ev) -> + io:format("~p Closed ~p~n", [self(), ID]), + wxEvent:skip(Ev, [{skip, true}]) + end, + + wxDialog:connect(Dlg, command_button_clicked,[{callback,Print}]), + wxDialog:connect(Dlg, close_window, [{skip, true}]) + end, + ?m({[],[],[]}, white_box_check_event_handlers()), + Pid = spawn_link(fun() -> + SetupEventHandlers(), + receive #wx{event=#wxClose{}} -> ok; + remove -> ok + end + end), + timer:sleep(500), %% Give it time to remove it + ?m({[{Pid,_,_}],[_],[_]}, white_box_check_event_handlers()), + + Pid ! remove, + timer:sleep(500), %% Give it time to remove it + ?m({[],[],[]}, white_box_check_event_handlers()), + + SetupEventHandlers(), + ?m({[{_,_,_}],[_],[_]}, white_box_check_event_handlers()), + + wxDialog:show(Dlg), + wx_test_lib:wx_close(Dlg, Config), + wxDialog:destroy(Dlg), + timer:sleep(500), %% Give it time to remove it + ?m({[],[],[]}, white_box_check_event_handlers()), + + wx_test_lib:flush(), + io:format("**Deleting Frame**~n",[]), + wx_test_lib:wx_destroy(Frame, Config). + %% timer:sleep(infinity), + %% ok. + +white_box_check_event_handlers() -> + {_,_,Server,_} = wx:get_env(), + {status, _, _, [Env, _, _, _, Data]} = sys:get_status(Server), + [_H, _data, {data, [{_, Record}]}] = Data, + {state, _Port1, _Port2, Users, [], CBs, _Next} = Record, + {[{Pid, Evs, Listener} || + {Pid, {user, Evs, Listener}} <- gb_trees:to_list(Users), + (Evs =/= [] orelse Listener =/= undefined)], %% Ignore empty + gb_trees:to_list(CBs), + [Funs || Funs = {Id, {Fun,_}} <- Env, is_integer(Id), is_function(Fun)] + }. diff --git a/lib/wx/test/wx_test_lib.erl b/lib/wx/test/wx_test_lib.erl index 8509d6be6f..9b65a50864 100644 --- a/lib/wx/test/wx_test_lib.erl +++ b/lib/wx/test/wx_test_lib.erl @@ -1,7 +1,7 @@ %% %% %CopyrightBegin% %% -%% Copyright Ericsson AB 2008-2010. All Rights Reserved. +%% Copyright Ericsson AB 2008-2013. All Rights Reserved. %% %% The contents of this file are subject to the Erlang Public License, %% Version 1.1, (the "License"); you may not use this file except in @@ -129,28 +129,30 @@ pick_msg() -> user_available(Config) -> false /= proplists:get_value(user, Config, false). - wx_destroy(Frame, Config) -> + wx_close(Frame, Config), + ?m(ok, wx:destroy()). + +wx_close(Frame, Config) -> case proplists:get_value(user, Config, false) of false -> timer:sleep(100), - ?m(ok, wxFrame:destroy(Frame)), - ?m(ok, wx:destroy()); + ?m(ok, wxWindow:destroy(Frame)); true -> timer:sleep(500), - ?m(ok, wxFrame:destroy(Frame)), - ?m(ok, wx:destroy()); + ?m(ok, wxWindow:destroy(Frame)); step -> %% Wait for user to close window ?m(ok, wxEvtHandler:connect(Frame, close_window, [{skip,true}])), - wait_for_close() + wait_for_close(), + catch wxEvtHandler:disconnect(Frame, close_window), + ok end. wait_for_close() -> receive #wx{event=#wxClose{}} -> - ?log("Got close~n",[]), - ?m(ok, wx:destroy()); + ?log("Got close~n",[]); #wx{obj=Obj, event=Event} -> try Name = wxTopLevelWindow:getTitle(Obj), -- cgit v1.2.3