From 2b495c9750adb1f38ce2e72bdd6f3700d5aa7887 Mon Sep 17 00:00:00 2001 From: Andrey Lushnikov Date: Mon, 2 Nov 2020 16:21:34 -0800 Subject: [PATCH] browser(firefox): fix SimpleChannel to await initialization (#4311) As Joel noticed recently, MessageManager in firefox doesn't guarantee message delivery if the opposite end hasn't been initialized yet. In this case, message will be silently dropped on the ground. To fix this, we establish a handshake in SimpleChannel to make sure that both ends are initialized, end buffer outgoing messages until this happens. Drive-by: serialize dialog events to only deliver *after* the `Page.ready` protocol event. Otherwise, we deliver dialog events to the unreported page. --- browser_patches/firefox/BUILD_NUMBER | 4 +- .../firefox/juggler/SimpleChannel.js | 57 ++++++++++++++++--- .../firefox/juggler/content/FrameTree.js | 4 +- .../firefox/juggler/content/WorkerMain.js | 4 +- .../firefox/juggler/protocol/PageHandler.js | 21 ++++++- 5 files changed, 72 insertions(+), 18 deletions(-) diff --git a/browser_patches/firefox/BUILD_NUMBER b/browser_patches/firefox/BUILD_NUMBER index 77bbcb4d4a..cb074bfc0d 100644 --- a/browser_patches/firefox/BUILD_NUMBER +++ b/browser_patches/firefox/BUILD_NUMBER @@ -1,2 +1,2 @@ -1198 -Changed: lushnikov@chromium.org Thu 29 Oct 2020 04:23:02 PM PDT +1199 +Changed: lushnikov@chromium.org Mon 02 Nov 2020 04:10:47 PM PST diff --git a/browser_patches/firefox/juggler/SimpleChannel.js b/browser_patches/firefox/juggler/SimpleChannel.js index 7cd5ae26c6..59b29532ab 100644 --- a/browser_patches/firefox/juggler/SimpleChannel.js +++ b/browser_patches/firefox/juggler/SimpleChannel.js @@ -17,10 +17,11 @@ class SimpleChannel { }; mm.addMessageListener(SIMPLE_CHANNEL_MESSAGE_NAME, messageListener); - channel.transport.sendMessage = obj => mm.sendAsyncMessage(SIMPLE_CHANNEL_MESSAGE_NAME, obj); - channel.transport.dispose = () => { - mm.removeMessageListener(SIMPLE_CHANNEL_MESSAGE_NAME, messageListener); - }; + channel.setTransport({ + sendMessage: obj => mm.sendAsyncMessage(SIMPLE_CHANNEL_MESSAGE_NAME, obj), + dispose: () => mm.removeMessageListener(SIMPLE_CHANNEL_MESSAGE_NAME, messageListener), + }); + return channel; } @@ -30,14 +31,39 @@ class SimpleChannel { this._connectorId = 0; this._pendingMessages = new Map(); this._handlers = new Map(); - this._bufferedRequests = []; + this._bufferedIncomingMessages = []; + this._bufferedOutgoingMessages = []; this.transport = { sendMessage: null, dispose: null, }; + this._ready = false; this._disposed = false; } + setTransport(transport) { + this.transport = transport; + // connection handshake: + // 1. There are two channel ends in different processes. + // 2. Both ends start in the `ready = false` state, meaning that they will + // not send any messages over transport. + // 3. Once channel end is created, it sends `READY` message to the other end. + // 4. Eventually, at least one of the ends receives `READY` message and responds with + // `READY_ACK`. We assume at least one of the ends will receive "READY" event from the other, since + // channel ends have a "parent-child" relation, i.e. one end is always created before the other one. + // 5. Once channel end receives either `READY` or `READY_ACK`, it transitions to `ready` state. + this.transport.sendMessage('READY'); + } + + _markAsReady() { + if (this._ready) + return; + this._ready = true; + for (const msg of this._bufferedOutgoingMessages) + this.transport.sendMessage(msg); + this._bufferedOutgoingMessages = []; + } + dispose() { if (this._disposed) return; @@ -72,8 +98,8 @@ class SimpleChannel { throw new Error('ERROR: double-register for namespace ' + namespace); this._handlers.set(namespace, handler); // Try to re-deliver all pending messages. - const bufferedRequests = this._bufferedRequests; - this._bufferedRequests = []; + const bufferedRequests = this._bufferedIncomingMessages; + this._bufferedIncomingMessages = []; for (const data of bufferedRequests) { this._onMessage(data); } @@ -98,11 +124,24 @@ class SimpleChannel { const promise = new Promise((resolve, reject) => { this._pendingMessages.set(id, {connectorId, resolve, reject, methodName, namespace}); }); - this.transport.sendMessage({requestId: id, methodName, params, namespace}); + const message = {requestId: id, methodName, params, namespace}; + if (this._ready) + this.transport.sendMessage(message); + else + this._bufferedOutgoingMessages.push(message); return promise; } async _onMessage(data) { + if (data === 'READY') { + this.transport.sendMessage('READY_ACK'); + this._markAsReady(); + return; + } + if (data === 'READY_ACK') { + this._markAsReady(); + return; + } if (data.responseId) { const {resolve, reject} = this._pendingMessages.get(data.responseId); this._pendingMessages.delete(data.responseId); @@ -114,7 +153,7 @@ class SimpleChannel { const namespace = data.namespace; const handler = this._handlers.get(namespace); if (!handler) { - this._bufferedRequests.push(data); + this._bufferedIncomingMessages.push(data); return; } const method = handler[data.methodName]; diff --git a/browser_patches/firefox/juggler/content/FrameTree.js b/browser_patches/firefox/juggler/content/FrameTree.js index 32359d75d0..98ee5c5f19 100644 --- a/browser_patches/firefox/juggler/content/FrameTree.js +++ b/browser_patches/firefox/juggler/content/FrameTree.js @@ -527,10 +527,10 @@ class Worker { workerDebugger.initialize('chrome://juggler/content/content/WorkerMain.js'); this._channel = new SimpleChannel(`content::worker[${this._workerId}]`); - this._channel.transport = { + this._channel.setTransport({ sendMessage: obj => workerDebugger.postMessage(JSON.stringify(obj)), dispose: () => {}, - }; + }); this._workerDebuggerListener = { QueryInterface: ChromeUtils.generateQI([Ci.nsIWorkerDebuggerListener]), onMessage: msg => void this._channel._onMessage(JSON.parse(msg)), diff --git a/browser_patches/firefox/juggler/content/WorkerMain.js b/browser_patches/firefox/juggler/content/WorkerMain.js index 9b9bcf2900..3d0c1168cb 100644 --- a/browser_patches/firefox/juggler/content/WorkerMain.js +++ b/browser_patches/firefox/juggler/content/WorkerMain.js @@ -9,10 +9,10 @@ loadSubScript('chrome://juggler/content/SimpleChannel.js'); const channel = new SimpleChannel('worker::worker'); const eventListener = event => channel._onMessage(JSON.parse(event.data)); this.addEventListener('message', eventListener); -channel.transport = { +channel.setTransport({ sendMessage: msg => postMessage(JSON.stringify(msg)), dispose: () => this.removeEventListener('message', eventListener), -}; +}); const runtime = new Runtime(true /* isWorker */); diff --git a/browser_patches/firefox/juggler/protocol/PageHandler.js b/browser_patches/firefox/juggler/protocol/PageHandler.js index 0556a01df2..fe5ccbd191 100644 --- a/browser_patches/firefox/juggler/protocol/PageHandler.js +++ b/browser_patches/firefox/juggler/protocol/PageHandler.js @@ -73,8 +73,12 @@ class PageHandler { this._reportedFrameIds = new Set(); this._networkEventsForUnreportedFrameIds = new Map(); - for (const dialog of this._pageTarget.dialogs()) - this._onDialogOpened(dialog); + // `Page.ready` protocol event is emitted whenever page has completed initialization, e.g. + // finished all the transient navigations to the `about:blank`. + // + // We'd like to avoid reporting meaningful events before the `Page.ready` since they are likely + // to be ignored by the protocol clients. + this._isPageReady = false; if (this._pageTarget.screencastInfo()) this._onScreencastStarted(); @@ -102,7 +106,7 @@ class PageHandler { pageNavigationAborted: emitProtocolEvent('Page.navigationAborted'), pageNavigationCommitted: emitProtocolEvent('Page.navigationCommitted'), pageNavigationStarted: emitProtocolEvent('Page.navigationStarted'), - pageReady: emitProtocolEvent('Page.ready'), + pageReady: this._onPageReady.bind(this), pageSameDocumentNavigation: emitProtocolEvent('Page.sameDocumentNavigation'), pageUncaughtError: emitProtocolEvent('Page.uncaughtError'), pageWorkerCreated: this._onWorkerCreated.bind(this), @@ -130,7 +134,16 @@ class PageHandler { this._session.emitEvent('Page.screencastStarted', { screencastId: info.videoSessionId, file: info.file }); } + _onPageReady(event) { + this._isPageReady = true; + this._session.emitEvent('Page.ready'); + for (const dialog of this._pageTarget.dialogs()) + this._onDialogOpened(dialog); + } + _onDialogOpened(dialog) { + if (!this._isPageReady) + return; this._session.emitEvent('Page.dialogOpened', { dialogId: dialog.id(), type: dialog.type(), @@ -140,6 +153,8 @@ class PageHandler { } _onDialogClosed(dialog) { + if (!this._isPageReady) + return; this._session.emitEvent('Page.dialogClosed', { dialogId: dialog.id(), }); }