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

refactor: try just using regular [Sync] for MessageSync #21776

Merged
merged 1 commit into from Jan 22, 2020
Merged

refactor: try just using regular [Sync] for MessageSync #21776

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/browser/api/browser-window.js
View file
Open in desktop
Expand Up @@ -116,7 +116,7 @@ BrowserWindow.getFocusedWindow = () => {

BrowserWindow.fromWebContents = (webContents) => {
for (const window of BrowserWindow.getAllWindows()) {
if (window.webContents.equal(webContents)) return window
if (window.webContents && window.webContents.equal(webContents)) return window
}
}

Expand Down
5 changes: 1 addition & 4 deletions shell/common/api/api.mojom
View file
Open in desktop
Expand Up @@ -42,10 +42,7 @@ interface ElectronBrowser {

// Emits an event on |channel| from the ipcMain JavaScript object in the main
// process, and waits synchronously for a response.
//
// NB. this is not marked [Sync] because mojo synchronous methods can be
// reordered with respect to asynchronous methods on the same channel.
// Instead, callers can manually block on the response to this method.
[Sync]
MessageSync(
bool internal,
string channel,
Expand Down
120 changes: 26 additions & 94 deletions shell/renderer/api/atom_api_renderer_ipc.cc
View file
Open in desktop
Expand Up @@ -35,20 +35,13 @@ RenderFrame* GetCurrentRenderFrame() {

class IPCRenderer : public mate::Wrappable<IPCRenderer> {
public:
explicit IPCRenderer(v8::Isolate* isolate)
: task_runner_(base::CreateSingleThreadTaskRunner({base::ThreadPool()})) {
explicit IPCRenderer(v8::Isolate* isolate) {
Init(isolate);
RenderFrame* render_frame = GetCurrentRenderFrame();
DCHECK(render_frame);

// Bind the interface on the background runner. All accesses will be via
// the thread-safe pointer. This is to support our "fake-sync"
// MessageSync() hack; see the comment in IPCRenderer::SendSync.
electron::mojom::ElectronBrowserPtrInfo info;
render_frame->GetRemoteInterfaces()->GetInterface(mojo::MakeRequest(&info));
electron_browser_ptr_ =
electron::mojom::ThreadSafeElectronBrowserPtr::Create(std::move(info),
task_runner_);
render_frame->GetRemoteInterfaces()->GetInterface(
mojo::MakeRequest(&electron_browser_ptr_));
}

static void BuildPrototype(v8::Isolate* isolate,
Expand All @@ -69,7 +62,7 @@ class IPCRenderer : public mate::Wrappable<IPCRenderer> {
void Send(bool internal,
const std::string& channel,
const base::ListValue& arguments) {
electron_browser_ptr_->get()->Message(internal, channel, arguments.Clone());
electron_browser_ptr_->Message(internal, channel, arguments.Clone());
}

v8::Local<v8::Promise> Invoke(mate::Arguments* args,
Expand All @@ -78,7 +71,7 @@ class IPCRenderer : public mate::Wrappable<IPCRenderer> {
electron::util::Promise p(args->isolate());
auto handle = p.GetHandle();

electron_browser_ptr_->get()->Invoke(
electron_browser_ptr_->Invoke(
channel, arguments.Clone(),
base::BindOnce([](electron::util::Promise p,
base::Value result) { p.Resolve(result); },
Expand All @@ -92,103 +85,42 @@ class IPCRenderer : public mate::Wrappable<IPCRenderer> {
int32_t web_contents_id,
const std::string& channel,
const base::ListValue& arguments) {
electron_browser_ptr_->get()->MessageTo(
internal, send_to_all, web_contents_id, channel, arguments.Clone());
electron_browser_ptr_->MessageTo(internal, send_to_all, web_contents_id,
channel, arguments.Clone());
}

void SendToHost(const std::string& channel,
const base::ListValue& arguments) {
electron_browser_ptr_->get()->MessageHost(channel, arguments.Clone());
electron_browser_ptr_->MessageHost(channel, arguments.Clone());
}

base::Value SendSync(bool internal,
const std::string& channel,
const base::ListValue& arguments) {
// We aren't using a true synchronous mojo call here. We're calling an
// asynchronous method and blocking on the result. The reason we're doing
// this is a little complicated, so buckle up.
//
// Mojo has a concept of synchronous calls. However, synchronous calls are
// dangerous. In particular, it's quite possible for two processes to call
// synchronous methods on each other and cause a deadlock. Mojo has a
// mechanism to avoid this kind of deadlock: if a process is waiting on the
// result of a synchronous call, and it receives an incoming call for a
// synchronous method, it will process that request immediately, even
// though it's currently blocking. However, if it receives an incoming
// request for an _asynchronous_ method, that can't cause a deadlock, so it
// stashes the request on a queue to be processed once the synchronous
// thing it's waiting on returns.
//
// This behavior is useful for preventing deadlocks, but it is inconvenient
// here because it can result in messages being reordered. If the main
// process is awaiting the result of a synchronous call (which it does only
// very rarely, since it's bad to block the main process), and we send
// first an asynchronous message to the main process, followed by a
// synchronous message, then the main process will process the synchronous
// one first.
//
// It turns out, Electron has some dependency on message ordering,
// especially during window shutdown, and getting messages out of order can
// result in, for example, remote objects disappearing unexpectedly. To
// avoid these issues and guarantee consistent message ordering, we send
// all messages to the main process as asynchronous messages. This causes
// them to always be queued and processed in the same order they were
// received, even if they were received while the main process was waiting
// on a synchronous call.
//
// However, in the calling process, we still need to block on the result,
// because the caller is expecting a result synchronously. So we do a bit
// of a trick: we pass the Mojo handle over to a worker thread, send the
// asynchronous message from that thread, and then block on the result.
// It's important that we pass the handle over to the worker thread,
// because that allows Mojo to process incoming messages (most importantly,
// the response to our request) on that thread. If we didn't pass it to a
// worker thread, and instead sent the call from the main thread, we would
// never receive a response because Mojo wouldn't be able to run its
// message handling code, because the main thread would be tied up blocking
// on the WaitableEvent.
//
// Phew. If you got this far, here's a gold star: ⭐️

base::Value result;

// A task is posted to a worker thread to execute the request so that
// this thread may block on a waitable event. It is safe to pass raw
// pointers to |result| and |response_received_event| as this stack frame
// will survive until the request is complete.

base::WaitableEvent response_received_event;
task_runner_->PostTask(
FROM_HERE, base::BindOnce(&IPCRenderer::SendMessageSyncOnWorkerThread,
base::Unretained(this),
base::Unretained(&response_received_event),
base::Unretained(&result), internal, channel,
arguments.Clone()));
response_received_event.Wait();
electron_browser_ptr_->MessageSync(internal, channel, arguments.Clone(),
&result);

// // A task is posted to a worker thread to execute the request so that
// // this thread may block on a waitable event. It is safe to pass raw
// // pointers to |result| and |response_received_event| as this stack frame
// // will survive until the request is complete.

// base::WaitableEvent response_received_event;
// task_runner_->PostTask(
// FROM_HERE,
// base::BindOnce(&IPCRenderer::SendMessageSyncOnWorkerThread,
// base::Unretained(this),
// base::Unretained(&response_received_event),
// base::Unretained(&result), internal,
// channel, arguments.Clone()));
// response_received_event.Wait();
return result;
}

private:
void SendMessageSyncOnWorkerThread(base::WaitableEvent* event,
base::Value* result,
bool internal,
const std::string& channel,
base::Value arguments) {
electron_browser_ptr_->get()->MessageSync(
internal, channel, std::move(arguments),
base::BindOnce(&IPCRenderer::ReturnSyncResponseToMainThread,
base::Unretained(event), base::Unretained(result)));
}
static void ReturnSyncResponseToMainThread(base::WaitableEvent* event,
base::Value* result,
base::Value response) {
*result = std::move(response);
event->Signal();
}

scoped_refptr<base::SequencedTaskRunner> task_runner_;
scoped_refptr<electron::mojom::ThreadSafeElectronBrowserPtr>
electron_browser_ptr_;
electron::mojom::ElectronBrowserPtr electron_browser_ptr_;
};

void Initialize(v8::Local<v8::Object> exports,
Expand Down
8 changes: 3 additions & 5 deletions spec-main/api-browser-window-spec.ts
View file
Open in desktop
Expand Up @@ -599,13 +599,11 @@ describe('BrowserWindow module', () => {
})

describe('BrowserWindow.getFocusedWindow()', () => {
it('returns the opener window when dev tools window is focused', (done) => {
it('returns the opener window when dev tools window is focused', async () => {
w.show()
w.webContents.once('devtools-focused', () => {
expect(BrowserWindow.getFocusedWindow()).to.equal(w)
done()
})
w.webContents.openDevTools({ mode: 'undocked' })
await emittedOnce(w.webContents, 'devtools-focused')
expect(BrowserWindow.getFocusedWindow()).to.equal(w)
})
})

Expand Down