fix: requestSingleInstanceLock API sometimes hangs

[rzhao271]

Description of Change

Fixes #33736

CC @nornagon @deepak1556

Checklist

• PR description included and stakeholders cc'd
• npm test passes
• tests are changed or added
• relevant documentation is changed or added
• PR release notes describe the change in a way relevant to app developers, and are capitalized, punctuated, and past tense.

Release Notes

Notes: Fixed an issue with the app.requestSingleInstanceLock() API where it would sometimes hang.

[trop]

@rzhao271 has manually backported this PR to "18-x-y", please check out #33778

[deepak1556]

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.

[deepak1556]

Refs #33778 (comment)

[deepak1556]

Can you rebase against latest main to address the CI failures, thanks!

[Yakinopat]

this is what also me I was asking that question please help me. whith ensues

…

On Thu, 14 Apr 2022, 07:01 Robo, ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In patches/chromium/feat_add_data_transfer_to_requestsingleinstancelock.patch <#33777 (comment)>: > + 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), 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 <https://github.com/electron/electron/blob/3d4d39d67b072a7c355dada2881cb6791e9e9f53/shell/browser/api/electron_api_app.cc#L518> which returns false only when the first instance is shutting down. Problem: 1. StoreAck corresponds to ack_callback <https://github.com/electron/electron/blob/3d4d39d67b072a7c355dada2881cb6791e9e9f53/shell/browser/api/electron_api_app.cc#L1121> 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. — Reply to this email directly, view it on GitHub <#33777 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AHU32BJLTV435EJCXFR7ALDVE6KDFANCNFSM5TMDY2LA> . You are receiving this because you are subscribed to this thread.Message ID: ***@***.***>

[deepak1556]

Failing test is unrelated and a known flake on main.

/cc @jkleinsc

[deepak1556]

@rzhao271 can you rebase on latest main to address the flaky test failure, Refs #34020

[jkleinsc]

Merging as CI failure unrelated to PR change.

[release-clerk]

Release Notes Persisted

Fixed an issue with the app.requestSingleInstanceLock() API where it would sometimes hang.

[trop]

I have automatically backported this PR to "19-x-y", please check out #34071