Allow appup instruction delete_module module which is not loaded

The appup instruction 'delete_module' would cause a crash during
upgrade if the module to be deleted was not loaded. The reason was
that the release_handler tried to read the version number of the old
module after the code path had changed to point to the new version of
the application. Thus, if the module had not been loaded before the
upgrade, there would no longer be any such module in the path
(delete_module indicates that the module is deleted from the new
version of the application).

This is corrected by letting the release_handler read the old version
of the module only if the module is updated - not if it is
removed. And it is always read before the code path is changed.

11 files changed, 205 insertions, 51 deletions

diff --git a/lib/sasl/src/release_handler_1.erl b/lib/sasl/src/release_handler_1.erl
index 3d6bc9fbc3..ff62f847ac 100644
--- a/lib/sasl/src/release_handler_1.erl
+++ b/lib/sasl/src/release_handler_1.erl
@@ -33,7 +33,7 @@
 %% libdirs   = [{Lib, LibVsn, LibDir}] - Maps Lib to Vsn and Directory
 %% unpurged  = [{Mod, soft_purge | brutal_purge}]
 %% vsns      = [{Mod, OldVsn, NewVsn}] - remember the old vsn of a mod
-%%                  before it is removed/a new vsn is loaded; the new vsn
+%%                  before a new vsn is loaded; the new vsn
 %%                  is kept in case of a downgrade, where the code_change
 %%                  function receives the vsn of the module to downgrade
 %%                  *to*.
@@ -224,7 +224,7 @@ eval({load_object_code, {Lib, LibVsn, Modules}}, EvalState) ->
 				    FName = filename:join([LibDir, "ebin", File]),
 				    case erl_prim_loader:get_file(FName) of
 					{ok, Bin, FName2} ->
-					    NVsns = add_new_vsn(Mod, Bin, Vsns),
+					    NVsns = add_vsns(Mod, Bin, Vsns),
 					    {[{Mod, Bin, FName2} | Bins],NVsns};
 					error ->
 					    throw({error, {no_such_file,FName}})
@@ -258,32 +258,21 @@ eval({load, {Mod, _PrePurgeMethod, PostPurgeMethod}}, EvalState) ->
     {value, {_Mod, Bin, File}} = lists:keysearch(Mod, 1, Bins),
     % load_binary kills all procs running old code
     % if soft_purge, we know that there are no such procs now
-    Vsns = EvalState#eval_state.vsns,
-    NewVsns = add_old_vsn(Mod, Vsns),
     code:load_binary(Mod, File, Bin),
     % Now, the prev current is old.  There might be procs
     % running it.  Find them.
     Unpurged = do_soft_purge(Mod,PostPurgeMethod,EvalState#eval_state.unpurged),
     EvalState#eval_state{bins = lists:keydelete(Mod, 1, Bins),
-			 unpurged = Unpurged,
-			 vsns = NewVsns};
+			 unpurged = Unpurged};
 eval({remove, {Mod, _PrePurgeMethod, PostPurgeMethod}}, EvalState) ->
-    % purge kills all procs running old code
-    % if soft_purge, we know that there are no such procs now
-    Vsns = EvalState#eval_state.vsns,
-    NewVsns = add_old_vsn(Mod, Vsns),
+    %% purge kills all procs running old code
+    %% if soft_purge, we know that there are no such procs now
     code:purge(Mod),
     code:delete(Mod),
-    % Now, the prev current is old.  There might be procs
-    % running it.  Find them.
-    Unpurged =
-	case code:soft_purge(Mod) of
-	    true -> EvalState#eval_state.unpurged;
-	    false -> [{Mod, PostPurgeMethod} | EvalState#eval_state.unpurged]
-	end,
-%%    Bins = EvalState#eval_state.bins,
-%%    EvalState#eval_state{bins = lists:keydelete(Mod, 1, Bins),
-    EvalState#eval_state{unpurged = Unpurged, vsns = NewVsns};
+    %% Now, the prev current is old.  There might be procs
+    %% running it.  Find them.
+    Unpurged = do_soft_purge(Mod,PostPurgeMethod,EvalState#eval_state.unpurged),
+    EvalState#eval_state{unpurged = Unpurged};
 eval({purge, Modules}, EvalState) ->
     % Now, if there are any processes still executing old code, OR
     % if some new processes started after suspend but before load,
@@ -606,26 +595,20 @@ sync_nodes(Id, Nodes) ->
 		  end,
 		  NNodes).
 
-add_old_vsn(Mod, Vsns) ->
+add_vsns(Mod, NewBin, Vsns) ->
+    OldVsn = get_current_vsn(Mod),
+    NewVsn = get_vsn(NewBin),
     case lists:keysearch(Mod, 1, Vsns) of
-	{value, {Mod, undefined, NewVsn}} ->
-	    OldVsn = get_current_vsn(Mod),
-	    lists:keyreplace(Mod, 1, Vsns, {Mod, OldVsn, NewVsn});
-	{value, {Mod, _OldVsn, _NewVsn}} ->
-	    Vsns;
+	{value, {Mod, OldVsn0, NewVsn0}} ->
+	    lists:keyreplace(Mod, 1, Vsns, {Mod,
+					    replace_undefined(OldVsn0,OldVsn),
+					    replace_undefined(NewVsn0,NewVsn)});
 	false ->
-	    OldVsn = get_current_vsn(Mod),
-	    [{Mod, OldVsn, undefined} | Vsns]
+	    [{Mod, OldVsn, NewVsn} | Vsns]
     end.
 
-add_new_vsn(Mod, Bin, Vsns) ->
-    NewVsn = get_vsn(Bin),
-    case lists:keysearch(Mod, 1, Vsns) of
-	{value, {Mod, OldVsn, undefined}} ->
-	    lists:keyreplace(Mod, 1, Vsns, {Mod, OldVsn, NewVsn});
-	false ->
-	    [{Mod, undefined, NewVsn} | Vsns]
-    end.
+replace_undefined(undefined,Vsn) -> Vsn;
+replace_undefined(Vsn,_) -> Vsn.
 
 %%-----------------------------------------------------------------
 %% Func: get_current_vsn/1
@@ -645,7 +628,9 @@ get_current_vsn(Mod) ->
 	{ok, Bin, _File2} ->
 	    get_vsn(Bin);
 	error ->
-	    throw({error, {no_such_file, File}})
+	    %% This is the case when a new module is added, there will
+	    %% be no current version of it at the time of this call.
+	    undefined
     end.
 
 %%-----------------------------------------------------------------
diff --git a/lib/sasl/test/release_handler_SUITE.erl b/lib/sasl/test/release_handler_SUITE.erl
index d1939694ea..9c7733b7ec 100644
--- a/lib/sasl/test/release_handler_SUITE.erl
+++ b/lib/sasl/test/release_handler_SUITE.erl
@@ -55,7 +55,7 @@ win32_cases() ->
 
 %% Cases that can be run on all platforms
 cases() ->
-    [otp_2740, otp_2760, otp_5761, otp_9402, instructions, eval_appup].
+    [otp_2740, otp_2760, otp_5761, otp_9402, otp_9417, instructions, eval_appup].
 
 groups() ->
     [{release,[],
@@ -683,7 +683,7 @@ otp_9402(Conf) when is_list(Conf) ->
     %% Check path
     Dir1 = filename:join([LibDir, "a-1.1"]),
     Dir1 = rpc:call(Node, code, lib_dir, [a]),
-    ABeam = code:which(a),
+    ABeam = rpc:call(Node, code, which, [a]),
 
     %% Install second release, with no changed modules
     {ok, RelVsn2} =
@@ -696,7 +696,6 @@ otp_9402(Conf) when is_list(Conf) ->
     ok = rpc:call(Node, release_handler, install_file,
 		  [RelVsn2, filename:join(Rel2Dir, "sys.config")]),
 
-    %% Install RelVsn2
     {ok, RelVsn1, []} =
 	rpc:call(Node, release_handler, install_release, [RelVsn2]),
 
@@ -707,7 +706,7 @@ otp_9402(Conf) when is_list(Conf) ->
     true = filelib:is_regular(filename:join(APrivDir2,"file")),
 
     %% Just to make sure no modules have been re-loaded
-    ABeam = code:which(a),
+    ABeam = rpc:call(Node, code, which, [a]),
 
     %% Install RelVsn1 again
     {ok, _OtherVsn, []} =
@@ -719,11 +718,65 @@ otp_9402(Conf) when is_list(Conf) ->
     false = filelib:is_regular(filename:join(APrivDir1,"file")),
 
     %% Just to make sure no modules have been re-loaded
-    ABeam = code:which(a),
+    ABeam = rpc:call(Node, code, which, [a]),
 
     ok.
 
 
+%% When a module is deleted in an appup instruction, the upgrade
+%% failed if the module was not loaded.
+otp_9417(Conf) when is_list(Conf) ->
+    %% Set some paths
+    PrivDir = priv_dir(Conf),
+    Dir = filename:join(PrivDir,"otp_9417"),
+    LibDir = filename:join(?config(data_dir, Conf), "lib"),
+
+    %% Create the releases
+    Rel1 = create_and_install_fake_first_release(Dir,
+						 [{b,"1.0",LibDir}]),
+    Rel2 = create_fake_upgrade_release(Dir,
+				       "2",
+				       [{b,"2.0",LibDir}],
+				       {Rel1,Rel1,[LibDir]}),
+    Rel1Dir = filename:dirname(Rel1),
+    Rel2Dir = filename:dirname(Rel2),
+
+    %% Start a slave node
+    {ok, Node} = t_start_node(otp_9417, Rel1, filename:join(Rel1Dir,"sys.config")),
+
+    %% Check paths
+    Dir1 = filename:join([LibDir, "b-1.0"]),
+    Dir1 = rpc:call(Node, code, lib_dir, [b]),
+    BLibBeam = filename:join([Dir1,"ebin","b_lib.beam"]),
+    BLibBeam = rpc:call(Node,code,which,[b_lib]),
+    false = rpc:call(Node,code,is_loaded,[b_lib]),
+    false = rpc:call(Node,code,is_loaded,[b_server]),
+
+    %% Install second release, which removes b_lib module
+    {ok, RelVsn2} =
+	rpc:call(Node, release_handler, set_unpacked,
+		 [Rel2++".rel", [{b,"2.0",LibDir}]]),
+    ok = rpc:call(Node, release_handler, install_file,
+		  [RelVsn2, filename:join(Rel2Dir, "relup")]),
+    ok = rpc:call(Node, release_handler, install_file,
+		  [RelVsn2, filename:join(Rel2Dir, "start.boot")]),
+    ok = rpc:call(Node, release_handler, install_file,
+		  [RelVsn2, filename:join(Rel2Dir, "sys.config")]),
+
+    {ok, _RelVsn1, []} =
+	rpc:call(Node, release_handler, install_release, [RelVsn2]),
+
+    %% Check that the module does no longer exist
+    false = rpc:call(Node, code, is_loaded, [b_lib]),
+    non_existing = rpc:call(Node, code, which, [b_lib]),
+
+    %% And check some paths
+    Dir2 = filename:join([LibDir, "b-2.0"]),
+    Dir2 = rpc:call(Node, code, lib_dir, [b]),
+    BServerBeam = filename:join([Dir2,"ebin","b_server.beam"]),
+    {file,BServerBeam} = rpc:call(Node,code,is_loaded,[b_server]),
+    ok.
+
 %% Test upgrade and downgrade of applications
 eval_appup(Conf) when is_list(Conf) ->
 
diff --git a/lib/sasl/test/release_handler_SUITE_data/Makefile.src b/lib/sasl/test/release_handler_SUITE_data/Makefile.src
index abaa08f810..a12e526d2e 100644
--- a/lib/sasl/test/release_handler_SUITE_data/Makefile.src
+++ b/lib/sasl/test/release_handler_SUITE_data/Makefile.src
@@ -1,16 +1,19 @@
 EFLAGS=+debug_info
 
 P2B= \
-     P2B/a-2.0/ebin/a.beam \
-     P2B/a-2.0/ebin/a_sup.beam
+     P2B/a-2.0/ebin/a.@EMULATOR@ \
+     P2B/a-2.0/ebin/a_sup.@EMULATOR@
 
 LIB= \
-     lib/a-1.2/ebin/a.beam \
-     lib/a-1.2/ebin/a_sup.beam \
-     lib/a-1.1/ebin/a.beam \
-     lib/a-1.1/ebin/a_sup.beam \
-     lib/a-1.0/ebin/a.beam \
-     lib/a-1.0/ebin/a_sup.beam \
+     lib/a-1.2/ebin/a.@EMULATOR@ \
+     lib/a-1.2/ebin/a_sup.@EMULATOR@ \
+     lib/a-1.1/ebin/a.@EMULATOR@ \
+     lib/a-1.1/ebin/a_sup.@EMULATOR@ \
+     lib/a-1.0/ebin/a.@EMULATOR@ \
+     lib/a-1.0/ebin/a_sup.@EMULATOR@ \
+     lib/b-1.0/ebin/b_server.@EMULATOR@ \
+     lib/b-1.0/ebin/b_lib.@EMULATOR@ \
+     lib/b-2.0/ebin/b_server.@EMULATOR@
 
 APP= \
      app1_app2/lib1/app1-1.0/ebin/app1_sup.@EMULATOR@ \
@@ -64,6 +67,15 @@ lib/a-1.2/ebin/a.@EMULATOR@: lib/a-1.2/src/a.erl
 lib/a-1.2/ebin/a_sup.@EMULATOR@: lib/a-1.2/src/a_sup.erl
 	erlc $(EFLAGS) -olib/a-1.2/ebin lib/a-1.2/src/a_sup.erl
 
+lib/b-1.0/ebin/b_server.@EMULATOR@: lib/b-1.0/src/b_server.erl
+	erlc $(EFLAGS) -olib/b-1.0/ebin lib/b-1.0/src/b_server.erl
+lib/b-1.0/ebin/b_lib.@EMULATOR@: lib/b-1.0/src/b_lib.erl
+	erlc $(EFLAGS) -olib/b-1.0/ebin lib/b-1.0/src/b_lib.erl
+
+lib/b-2.0/ebin/b_server.@EMULATOR@: lib/b-2.0/src/b_server.erl
+	erlc $(EFLAGS) -olib/b-2.0/ebin lib/b-2.0/src/b_server.erl
+
+
 
 app1_app2/lib1/app1-1.0/ebin/app1_sup.@EMULATOR@: app1_app2/lib1/app1-1.0/src/app1_sup.erl
 	erlc $(EFLAGS) -oapp1_app2/lib1/app1-1.0/ebin app1_app2/lib1/app1-1.0/src/app1_sup.erl
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/README b/lib/sasl/test/release_handler_SUITE_data/lib/README
index e539580ac0..667d21d4cf 100644
--- a/lib/sasl/test/release_handler_SUITE_data/lib/README
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/README
@@ -6,4 +6,11 @@ can be upgraded to from a-1.0. Module a has changed
 
 a-1.2:
 can be upgraded to from a-1.1.
-No module have changed, but priv dir is added including one 'file'
\ No newline at end of file
+No module have changed, but priv dir is added including one 'file'
+
+b-1.0:
+start version, includes b_lib and b_server
+
+b-2.0:
+can be upgraded to from b-1.0.
+Removes b_lib and updates b_server
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/ebin/b.app b/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/ebin/b.app
new file mode 100644
index 0000000000..00347b2754
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/ebin/b.app
@@ -0,0 +1,7 @@
+%% -*- erlang -*-
+{application, b,
+ [{description, "B  CXC 138 12"},
+  {vsn, "1.0"},
+  {modules, [{b_server, 1},{b_lib, 1}]},
+  {registered, [b_server]},
+  {applications, [kernel, stdlib]}]}.
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/src/b_lib.erl b/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/src/b_lib.erl
new file mode 100644
index 0000000000..7e8a308a5e
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/src/b_lib.erl
@@ -0,0 +1,3 @@
+-module(b_lib).
+-compile(export_all).
+foo() -> ok.
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/src/b_server.erl b/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/src/b_server.erl
new file mode 100644
index 0000000000..e1a80a076f
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-1.0/src/b_server.erl
@@ -0,0 +1,37 @@
+-module(b_server).
+
+-behaviour(gen_server).
+
+%% API
+-export([start_link/0]).
+
+%% gen_server callbacks
+-export([init/1, handle_call/3, handle_cast/2, handle_info/2,
+	 terminate/2, code_change/3]).
+
+-define(SERVER, ?MODULE).
+
+-record(state, {}).
+
+start_link() ->
+    gen_server:start_link({local, ?SERVER}, ?MODULE, [], []).
+
+init([]) ->
+    {ok, #state{}}.
+
+handle_call(_Request, _From, State) ->
+    Reply = ok,
+    {reply, Reply, State}.
+
+handle_cast(_Msg, State) ->
+    {noreply, State}.
+
+handle_info(_Info, State) ->
+    {noreply, State}.
+
+terminate(_Reason, _State) ->
+    ok.
+
+code_change(OldVsn, State, Extra) ->
+    file:write_file("/tmp/b_server",io_lib:format("~p~n",[{"1.0",OldVsn,Extra}])),
+    {ok, State}.
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/ebin/b.app b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/ebin/b.app
new file mode 100644
index 0000000000..73c8e42b32
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/ebin/b.app
@@ -0,0 +1,7 @@
+%% -*- erlang -*-
+{application, b,
+ [{description, "B  CXC 138 12"},
+  {vsn, "2.0"},
+  {modules, [{b_server, 1}]},
+  {registered, [b_server]},
+  {applications, [kernel, stdlib]}]}.
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/ebin/b.appup b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/ebin/b.appup
new file mode 100644
index 0000000000..d261a37732
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/ebin/b.appup
@@ -0,0 +1,3 @@
+{"2.0",
+ [{"1.0",[{delete_module,b_lib}, {update,b_server,{advanced,[]}}]}],
+ [{"1.0",[{add_module,b_lib},{update,b_server,{advanced,[]}}]}]}.
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/src/b_lib.erl b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/src/b_lib.erl
new file mode 100644
index 0000000000..7e8a308a5e
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/src/b_lib.erl
@@ -0,0 +1,3 @@
+-module(b_lib).
+-compile(export_all).
+foo() -> ok.
diff --git a/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/src/b_server.erl b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/src/b_server.erl
new file mode 100644
index 0000000000..f8bfbdaff7
--- /dev/null
+++ b/lib/sasl/test/release_handler_SUITE_data/lib/b-2.0/src/b_server.erl
@@ -0,0 +1,37 @@
+-module(b_server).
+
+-behaviour(gen_server).
+
+%% API
+-export([start_link/0]).
+
+%% gen_server callbacks
+-export([init/1, handle_call/3, handle_cast/2, handle_info/2,
+	 terminate/2, code_change/3]).
+
+-define(SERVER, ?MODULE).
+
+-record(state, {}).
+
+start_link() ->
+    gen_server:start_link({local, ?SERVER}, ?MODULE, [], []).
+
+init([]) ->
+    {ok, #state{}}.
+
+handle_call(_Request, _From, State) ->
+    Reply = ok,
+    {reply, Reply, State}.
+
+handle_cast(_Msg, State) ->
+    {noreply, State}.
+
+handle_info(_Info, State) ->
+    {noreply, State}.
+
+terminate(_Reason, _State) ->
+    ok.
+
+code_change(OldVsn, State, Extra) ->
+    file:write_file("/tmp/b_server",io_lib:format("~p~n",[{"2.0",OldVsn,Extra}])),
+    {ok, State}.