Skip to content
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

Jump to bottom

fix: requestSingleInstanceLock API sometimes hangs #33777

Merged
merged 1 commit into from May 4, 2022
Merged

fix: requestSingleInstanceLock API sometimes hangs #33777

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
30 changes: 21 additions & 9 deletions patches/chromium/feat_add_data_transfer_to_requestsingleinstancelock.patch
View file
Open in desktop
Expand Up @@ -282,7 +282,7 @@ index be2c417c07a4206fac4a9a6c03e516fd0493c942..78f74b0b21242553b6af98628dc48190
return PROCESS_NOTIFIED;
}
diff --git a/chrome/browser/process_singleton_win.cc b/chrome/browser/process_singleton_win.cc
index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f941384b249a5d 100644
index ec725b44296266bea1a51aea889463a0bba8449c..3bb74c08cd78b11cd9925a6bfafc62d05018b627 100644
--- a/chrome/browser/process_singleton_win.cc
+++ b/chrome/browser/process_singleton_win.cc
@@ -21,6 +21,7 @@
Expand Down Expand Up @@ -406,7 +406,7 @@ index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f94138
bool ProcessLaunchNotification(
const ProcessSingleton::NotificationCallback& notification_callback,
UINT message,
@@ -151,16 +233,23 @@ bool ProcessLaunchNotification(
@@ -151,16 +233,35 @@ bool ProcessLaunchNotification(

// Handle the WM_COPYDATA message from another process.
const COPYDATASTRUCT* cds = reinterpret_cast<COPYDATASTRUCT*>(lparam);
Expand All @@ -423,18 +423,30 @@ index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f94138

- *result = notification_callback.Run(parsed_command_line, current_directory) ?
- TRUE : FALSE;
+ // notification_callback.Run waits for StoreAck to
+ // run to completion before moving onwards.
+ // Therefore, we cannot directly send the SendBackAck
+ // callback instead, as it would hang the program
+ // during the ConnectNamedPipe call.
+ g_write_ack_callback_called = false;
+ *result = notification_callback.Run(parsed_command_line, current_directory,
+ std::move(additional_data),
+ base::BindRepeating(&StoreAck))
+ ? TRUE
+ : FALSE;
+ g_ack_timer.Start(FROM_HERE, base::Seconds(0),
+ base::BindOnce(&SendBackAck));
+ if (*result) {
+ // If *result is TRUE, we return NOTIFY_SUCCESS.
+ // Only for that case does the second process read
+ // the acknowledgement. Therefore, only send back
+ // the acknowledgement if *result is TRUE,
+ // otherwise the program hangs during the ConnectNamedPipe call.
+ g_ack_timer.Start(FROM_HERE, base::Seconds(0),
Copy link
Member

Choose a reason for hiding this comment

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

I am still not convinced for the purpose of the timer here, consider the following sequence,

  • ProcessLaunchNotification gets called as soon as the second instance calls chrome::AttemptToNotifyRunningChrome
  • chrome::AttemptToNotifyRunningChrome triggers ReadAck as soon as the notification to the message window is successful
  • On the first instance side, notification_callback.Run represents NotificationCallbackWrapper which returns false only when the first instance is shutting down.

Problem:

  1. StoreAck corresponds to ack_callback which may get called by user once preventDefault action is executed if not we call it with nullptr for backwards compatibility. In the case, were user executes the StoreAck how does SendBackAck ensure the user input is obtained before piping data to the second instance. The timer fires as soon as notification_callback.Run is executed which does not correspond to user executing StoreAck.

  2. ReadAck is called eagerly for all success scenarios irrespective of whether the first instance has data to send or not.

Solutions:

For supporting the cases of sending data from first instance to second instance, we should have a message window on the second instance as well and the first instance should notify this message window to prepare for reading data back once the first instance knows for sure there is data to send. This would eliminate the timer used here and also ensure SendBackAck is responsible for controlling the ack pipe creation.

+ base::BindOnce(&SendBackAck));
+ }
return true;
}

@@ -261,9 +350,13 @@ bool ProcessSingleton::EscapeVirtualization(
@@ -261,9 +362,13 @@ bool ProcessSingleton::EscapeVirtualization(
ProcessSingleton::ProcessSingleton(
const std::string& program_name,
const base::FilePath& user_data_dir,
Expand All @@ -449,7 +461,7 @@ index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f94138
program_name_(program_name),
is_app_sandboxed_(is_app_sandboxed),
is_virtualized_(false),
@@ -278,6 +371,37 @@ ProcessSingleton::~ProcessSingleton() {
@@ -278,6 +383,37 @@ ProcessSingleton::~ProcessSingleton() {
::CloseHandle(lock_file_);
}

Expand Down Expand Up @@ -487,7 +499,7 @@ index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f94138
// Code roughly based on Mozilla.
ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
TRACE_EVENT0("startup", "ProcessSingleton::NotifyOtherProcess");
@@ -290,8 +414,9 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
@@ -290,8 +426,9 @@ ProcessSingleton::NotifyResult ProcessSingleton::NotifyOtherProcess() {
return PROCESS_NONE;
}

Expand All @@ -498,7 +510,7 @@ index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f94138
return PROCESS_NOTIFIED;
case chrome::NOTIFY_FAILED:
remote_window_ = NULL;
@@ -429,6 +554,18 @@ bool ProcessSingleton::Create() {
@@ -429,6 +566,18 @@ bool ProcessSingleton::Create() {
<< "Lock file can not be created! Error code: " << error;

if (lock_file_ != INVALID_HANDLE_VALUE) {
Expand All @@ -517,7 +529,7 @@ index ec725b44296266bea1a51aea889463a0bba8449c..a3d4dd1efc950033855a1f2783f94138
// Set the window's title to the path of our user data directory so
// other Chrome instances can decide if they should forward to us.
TRACE_EVENT0("startup", "ProcessSingleton::Create:CreateWindow");
@@ -456,6 +593,7 @@ bool ProcessSingleton::Create() {
@@ -456,6 +605,7 @@ bool ProcessSingleton::Create() {
}

void ProcessSingleton::Cleanup() {
Expand Down