Don't call RecordFailedTunnelStat with a nil tunnelErr

psiphon/serverApi.go

@@ -728,7 +728,8 @@ func RecordRemoteServerListStat(
 }
 
 // RecordFailedTunnelStat records metrics for a failed tunnel dial, including
-// dial parameters and error condition (tunnelErr).
+// dial parameters and error condition (tunnelErr). No record is created when
+// tunnelErr is nil.
 //
 // This uses the same reporting facility, with the same caveats, as
 // RecordRemoteServerListStat.
@@ -745,6 +746,16 @@ func RecordFailedTunnelStat(
 		return nil
 	}
 
+	// Callers should not call RecordFailedTunnelStat with a nil tunnelErr, as
+	// this is not a useful stat and it results in a nil pointer dereference.
+	// This check catches potential bug cases. An example edge case, now
+	// fixed, is deferred error handlers, such as the ones in in
+	// dialTunnel/tunnel.Activate, which may be invoked in the case of a
+	// panic, which can occur before any error value is returned.
+	if tunnelErr == nil {
+		return errors.TraceNew("no error")
+	}
+
 	lastConnected, err := getLastConnected()
 	if err != nil {
 		return errors.Trace(err)

psiphon/tunnel.go

@@ -182,16 +182,18 @@ func (tunnel *Tunnel) Activate(
 	defer func() {
 		if !activationSucceeded && baseCtx.Err() != context.Canceled {
 			tunnel.dialParams.Failed(tunnel.config)
-			_ = RecordFailedTunnelStat(
-				tunnel.config,
-				tunnel.dialParams,
-				tunnel.livenessTestMetrics,
-				-1,
-				-1,
-				retErr)
 			if tunnel.extraFailureAction != nil {
 				tunnel.extraFailureAction()
 			}
+			if retErr != nil {
+				_ = RecordFailedTunnelStat(
+					tunnel.config,
+					tunnel.dialParams,
+					tunnel.livenessTestMetrics,
+					-1,
+					-1,
+					retErr)
+			}
 		}
 	}()
 
@@ -704,16 +706,18 @@ func dialTunnel(
 	defer func() {
 		if !dialSucceeded && baseCtx.Err() != context.Canceled {
 			dialParams.Failed(config)
-			_ = RecordFailedTunnelStat(
-				config,
-				dialParams,
-				failedTunnelLivenessTestMetrics,
-				-1,
-				-1,
-				retErr)
 			if extraFailureAction != nil {
 				extraFailureAction()
 			}
+			if retErr != nil {
+				_ = RecordFailedTunnelStat(
+					config,
+					dialParams,
+					failedTunnelLivenessTestMetrics,
+					-1,
+					-1,
+					retErr)
+			}
 		}
 	}()