Skip to content

fix: 多次打开微软登录弹窗后报错 #5878

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
CiiLu wants to merge 1 commit into
base: main
Choose a base branch
from
+11 −2
Open

fix: 多次打开微软登录弹窗后报错 #5878

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 10 additions & 2 deletions HMCL/src/main/java/org/jackhuang/hmcl/game/OAuthServer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@

import java.io.IOException;
import java.security.SecureRandom;
import java.util.*;
import java.util.Base64;
import java.util.HashMap;
import java.util.Locale;
import java.util.Map;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.ExecutionException;

Expand Down Expand Up @@ -167,6 +170,7 @@ public static class Factory implements OAuth.Callback {
public final EventManager<GrantDeviceCodeEvent> onGrantDeviceCode = new EventManager<>();
public final EventManager<OpenBrowserEvent> onOpenBrowserAuthorizationCode = new EventManager<>();
public final EventManager<OpenBrowserEvent> onOpenBrowserDevice = new EventManager<>();
private OAuthServer server;

Comment on lines +173 to 174
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OAuthServer.Factory.server is written from the background login task (via startServer) and read from the JavaFX thread (via stop() on cancel). As-is it’s not volatile/synchronized, so stop() may not reliably see the latest server instance and the callback port can remain bound. Make access thread-safe (e.g., volatile + synchronized stop/start, or an AtomicReference with getAndSet).

Copilot uses AI. Check for mistakes.
@Override
public OAuth.Session startServer() throws IOException, AuthenticationException {
Expand All @@ -177,7 +181,7 @@ public OAuth.Session startServer() throws IOException, AuthenticationException {
IOException exception = null;
for (int port : new int[]{29111, 29112, 29113, 29114, 29115}) {
try {
OAuthServer server = new OAuthServer(port);
server = new OAuthServer(port);
server.start(NanoHTTPD.SOCKET_READ_TIMEOUT, true);
return server;
Comment on lines 181 to 186
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

startServer() overwrites the server field without first stopping any existing instance. If a previous login flow left a server running (e.g., user switched login method or closed abnormally), this loses the reference and that server can keep occupying a port. Consider stopping/clearing any existing server before creating a new one, and only assigning to the field after server.start(...) succeeds.

Copilot uses AI. Check for mistakes.
} catch (IOException e) {
Expand Down Expand Up @@ -207,6 +211,10 @@ public String getClientId() {
return System.getProperty("hmcl.microsoft.auth.id",
JarUtils.getAttribute("hmcl.microsoft.auth.id", ""));
}

public void stop() {
if (server != null) server.stop();
Comment on lines +215 to +216
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

stop() stops the current server but keeps the reference. If this is called multiple times or after the server self-stops, it can lead to redundant stops and makes it harder to reason about lifecycle. Consider setting server = null after stopping (ideally atomically if you address thread-safety) so future calls are no-ops and the instance can be GC’d.

Suggested change
public void stop() {
if (server != null) server.stop();
public synchronized void stop() {
if (server != null) {
server.stop();
server = null;
}

Copilot uses AI. Check for mistakes.
}
}

public static class GrantDeviceCodeEvent extends Event {
Expand Down
1 change: 1 addition & 0 deletions HMCL/src/main/java/org/jackhuang/hmcl/ui/account/MicrosoftAccountLoginPane.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -261,6 +261,7 @@ private void cancelAllTasks() {
}

private void onCancel() {
Accounts.OAUTH_CALLBACK.stop();
cancelAllTasks();
if (cancelCallback != null) cancelCallback.run();
Comment on lines 263 to 266
Copy link

Copilot AI Mar 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Stopping the callback server only in onCancel() won’t cover other paths that call cancelAllTasks() (e.g., switching between authorization-code and device-code flows, or transitioning to LoginFailed). In those cases the HTTP server started for the authorization-code flow can keep running and keep the port occupied. Consider moving the Accounts.OAUTH_CALLBACK.stop() into cancelAllTasks() (or calling it from every path that cancels/abandons the authorization-code flow).

Copilot uses AI. Check for mistakes.
fireEvent(new DialogCloseEvent());
Expand Down
Loading