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.

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

Fix images breaking on Firefox after a while (add heartbeat to prevent background page reap) #1852

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 3 additions & 0 deletions ext/js/application.js
Original file line number Diff line number Diff line change
Expand Up @@ -220,6 +220,9 @@ export class Application extends EventDispatcher {
if (mediaDrawingWorker !== null) {
api.connectToDatabaseWorker(mediaDrawingWorkerToBackendChannel.port1);
}
setInterval(() => {
void api.heartbeat();
}, 20 * 1000);

const {tabId, frameId} = await api.frameInformationGet();
const crossFrameApi = new CrossFrameAPI(api, tabId, frameId);
Expand Down
20 changes: 20 additions & 0 deletions ext/js/background/backend.js
Original file line number Diff line number Diff line change
Expand Up @@ -184,6 +184,7 @@
['findAnkiNotes', this._onApiFindAnkiNotes.bind(this)],
['openCrossFramePort', this._onApiOpenCrossFramePort.bind(this)],
['getLanguageSummaries', this._onApiGetLanguageSummaries.bind(this)],
['heartbeat', this._onApiHeartbeat.bind(this)],
]);

/** @type {import('api').PmApiMap} */
Expand Down Expand Up @@ -247,6 +248,7 @@

// On Chrome, this is for receiving messages sent with navigator.serviceWorker, which has the benefit of being able to transfer objects, but doesn't accept callbacks
(/** @type {ServiceWorkerGlobalScope & typeof globalThis} */ (globalThis)).addEventListener('message', this._onPmMessage.bind(this));
(/** @type {ServiceWorkerGlobalScope & typeof globalThis} */ (globalThis)).addEventListener('messageerror', this._onPmMessageError.bind(this));

if (this._canObservePermissionsChanges()) {
const onPermissionsChanged = this._onWebExtensionEventWrapper(this._onPermissionsChanged.bind(this));
Expand All @@ -259,6 +261,7 @@

/** @type {import('api').PmApiHandler<'connectToDatabaseWorker'>} */
async _onPmConnectToDatabaseWorker(_params, ports) {
console.log('Backend: _onPmConnectToDatabaseWorker');

Check failure on line 264 in ext/js/background/backend.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
if (ports !== null && ports.length > 0) {
await this._dictionaryDatabase.connectToDatabaseWorker(ports[0]);
}
Expand Down Expand Up @@ -303,6 +306,7 @@
// connectToBackend2
e.ports[0].onmessage = this._onPmMessage.bind(this);
});
sharedWorkerBridge.port.addEventListener('messageerror', this._onPmMessageError.bind(this));
sharedWorkerBridge.port.start();
}
try {
Expand Down Expand Up @@ -449,6 +453,16 @@
return invokeApiMapHandler(this._pmApiMap, action, params, [event.ports], () => {});
}

/**
* @param {MessageEvent<import('api').PmApiMessageAny>} event
*/
_onPmMessageError(event) {
const error = new ExtensionError('Backend: Error receiving message via postMessage');
error.data = event;
log.error(error);
}


/**
* @param {chrome.tabs.ZoomChangeInfo} event
*/
Expand Down Expand Up @@ -527,6 +541,7 @@

/** @type {import('api').ApiHandler<'termsFind'>} */
async _onApiTermsFind({text, details, optionsContext}) {
console.log('Backend: _onApiTermsFind');

Check failure on line 544 in ext/js/background/backend.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
const options = this._getProfileOptions(optionsContext, false);
const {general: {resultOutputMode: mode, maxResults}} = options;
const findTermsOptions = this._getTranslatorFindTermsOptions(mode, details, options);
Expand Down Expand Up @@ -1031,6 +1046,11 @@
return getLanguageSummaries();
}

/** @type {import('api').ApiHandler<'heartbeat'>} */
_onApiHeartbeat() {
return void 0;
}

// Command handlers

/**
Expand Down
12 changes: 12 additions & 0 deletions ext/js/background/offscreen.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@
import {API} from '../comm/api.js';
import {ClipboardReader} from '../comm/clipboard-reader.js';
import {createApiMap, invokeApiMapHandler} from '../core/api-map.js';
import {ExtensionError} from '../core/extension-error.js';
import {log} from '../core/log.js';
import {arrayBufferToBase64} from '../data/array-buffer-util.js';
import {DictionaryDatabase} from '../dictionary/dictionary-database.js';
import {WebExtension} from '../extension/web-extension.js';
Expand Down Expand Up @@ -187,6 +189,7 @@ export class Offscreen {
_createAndRegisterPort() {
const mc = new MessageChannel();
mc.port1.onmessage = this._onMcMessage.bind(this);
mc.port1.onmessageerror = this._onMcMessageError.bind(this);
this._api.registerOffscreenPort([mc.port2]);
}

Expand All @@ -202,4 +205,13 @@ export class Offscreen {
const {action, params} = event.data;
invokeApiMapHandler(this._mcApiMap, action, params, [event.ports], () => {});
}

/**
* @param {MessageEvent<import('offscreen').McApiMessageAny>} event
*/
_onMcMessageError(event) {
const error = new ExtensionError('Offscreen: Error receiving message via postMessage');
error.data = event;
log.error(error);
}
}
18 changes: 18 additions & 0 deletions ext/js/comm/api.js
Original file line number Diff line number Diff line change
Expand Up @@ -381,6 +381,23 @@
return this._invoke('openCrossFramePort', {targetTabId, targetFrameId});
}

/**
* This is used to keep the background page alive on Firefox MV3, as it does not support offscreen.
* The reason that backend persistency is required on FF is actually different from the reason it's required on Chromium --
* on Chromium, persistency (which we achieve via the offscreen page, not via this heartbeat) is required because the load time
* for the IndexedDB is incredibly long, which makes the first lookup after the extension sleeps take one minute+, which is
* not acceptable. However, on Firefox, the database is backed by sqlite and starts very fast. Instead, the problem is that the
* media-drawing-worker on the frontend holds a MessagePort to the database-worker on the backend, which closes when the extension
* sleeps, because the database-worker is killed and currently there is no way to detect a closed port due to
* https://github.com/whatwg/html/issues/1766 / https://github.com/whatwg/html/issues/10201
*
* So this is our only choice. We can remove this once there is a way to gracefully detect the closed MessagePort and rebuild it.
* @returns {Promise<import('api').ApiReturn<'heartbeat'>>}
*/
heartbeat() {
return this._invoke('heartbeat', void 0);
}

/**
* @param {Transferable[]} transferables
*/
Expand All @@ -392,6 +409,7 @@
* @param {MessagePort} port
*/
connectToDatabaseWorker(port) {
console.log('pmInvoke connectToDatabaseWorker');

Check failure on line 412 in ext/js/comm/api.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
this._pmInvoke('connectToDatabaseWorker', void 0, [port]);
}

Expand Down
9 changes: 8 additions & 1 deletion ext/js/comm/shared-worker-bridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
*/

import {createApiMap, invokeApiMapHandler} from '../core/api-map.js';
import {ExtensionError} from '../core/extension-error.js';
import {log} from '../core/log.js';

/**
Expand Down Expand Up @@ -64,12 +65,18 @@
const {action, params} = event.data;
return invokeApiMapHandler(this._apiMap, action, params, [interlocutorPort, event.ports], () => {});
});
interlocutorPort.addEventListener('messageerror', (/** @type {MessageEvent} */ event) => {
const error = new ExtensionError('SharedWorkerBridge: Error receiving message from interlocutor port when establishing connection');
error.data = event;
log.error(error);
});
interlocutorPort.start();
});
}

/** @type {import('shared-worker').ApiHandler<'registerBackendPort'>} */
_onRegisterBackendPort(_params, interlocutorPort, _ports) {
console.log('SharedWorkerBridge: backend port registered');

Check failure on line 79 in ext/js/comm/shared-worker-bridge.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
this._backendPort = interlocutorPort;
}

Expand All @@ -78,7 +85,7 @@
if (this._backendPort !== null) {
this._backendPort.postMessage(void 0, [ports[0]]); // connectToBackend2
} else {
log.error('SharedWorkerBridge: backend port is not registered');
log.warn('SharedWorkerBridge: backend port is not registered; this can happen if one of the content scripts loads faster than the backend when extension is reloading');
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions ext/js/dictionary/dictionary-database-worker-handler.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/

import {ExtensionError} from '../core/extension-error.js';
import {log} from '../core/log.js';
import {DictionaryDatabase} from './dictionary-database.js';

Expand All @@ -35,6 +36,11 @@ export class DictionaryDatabaseWorkerHandler {
log.error(e);
}
self.addEventListener('message', this._onMessage.bind(this), false);
self.addEventListener('messageerror', (event) => {
const error = new ExtensionError('DictionaryDatabaseWorkerHandler: Error receiving message from main thread');
error.data = event;
log.error(error);
});
}
// Private

Expand Down
10 changes: 10 additions & 0 deletions ext/js/dictionary/dictionary-database.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@

import {initWasm, Resvg} from '../../lib/resvg-wasm.js';
import {createApiMap, invokeApiMapHandler} from '../core/api-map.js';
import {ExtensionError} from '../core/extension-error.js';
import {log} from '../core/log.js';
import {safePerformance} from '../core/safe-performance.js';
import {stringReverse} from '../core/utilities.js';
Expand Down Expand Up @@ -402,9 +403,11 @@
*/
async drawMedia(items, source) {
if (this._worker !== null) { // if a worker is available, offload the work to it
console.log('offloading to worker');

Check failure on line 406 in ext/js/dictionary/dictionary-database.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
this._worker.postMessage({action: 'drawMedia', params: {items}}, [source]);
return;
}
console.log('doing work in worker');

Check failure on line 410 in ext/js/dictionary/dictionary-database.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
// otherwise, you are the worker, so do the work
safePerformance.mark('drawMedia:start');

Expand Down Expand Up @@ -838,14 +841,21 @@
async connectToDatabaseWorker(port) {
if (this._worker !== null) {
// executes outside of worker
console.log('connecting to worker');

Check failure on line 844 in ext/js/dictionary/dictionary-database.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
this._worker.postMessage({action: 'connectToDatabaseWorker'}, [port]);
return;
}
// executes inside worker
console.log('connected to worker');

Check failure on line 849 in ext/js/dictionary/dictionary-database.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
port.onmessage = (/** @type {MessageEvent<import('dictionary-database').ApiMessageAny>} */event) => {
const {action, params} = event.data;
return invokeApiMapHandler(this._apiMap, action, params, [port], () => {});
};
port.onmessageerror = (event) => {
const error = new ExtensionError('DictionaryDatabase: Error receiving message from main thread');
error.data = event;
log.error(error);
};
}

/** @type {import('dictionary-database').ApiHandler<'drawMedia'>} */
Expand Down
15 changes: 15 additions & 0 deletions ext/js/display/media-drawing-worker.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@

import {API} from '../comm/api.js';
import {createApiMap, invokeApiMapHandler} from '../core/api-map.js';
import {ExtensionError} from '../core/extension-error.js';
import {log} from '../core/log.js';
import {WebExtension} from '../extension/web-extension.js';

Expand Down Expand Up @@ -59,15 +60,22 @@
const message = event.data;
return invokeApiMapHandler(this._fromApplicationApiMap, message.action, message.params, [event.ports], () => {});
});
addEventListener('messageerror', (event) => {
const error = new ExtensionError('MediaDrawingWorker: Error receiving message from application');
error.data = event;
log.error(error);
});
}

/** @type {import('api').PmApiHandler<'drawMedia'>} */
async _onDrawMedia({requests}) {
console.log('MediaDrawingWorker: drawMedia', requests);

Check failure on line 72 in ext/js/display/media-drawing-worker.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
this._generation++;
this._canvasesByGeneration.set(this._generation, requests.map((request) => request.canvas));
this._cleanOldGenerations();
const newRequests = requests.map((request, index) => ({...request, canvas: null, generation: this._generation, canvasIndex: index, canvasWidth: request.canvas.width, canvasHeight: request.canvas.height}));
if (this._dbPort !== null) {
console.log('MediaDrawingWorker: sending drawMedia to database worker', newRequests, this._dbPort);

Check failure on line 78 in ext/js/display/media-drawing-worker.js

View workflow job for this annotation

GitHub Actions / JavaScript

Unexpected console statement
this._dbPort.postMessage({action: 'drawMedia', params: {requests: newRequests}});
} else {
log.error('no database port available');
Expand Down Expand Up @@ -115,10 +123,17 @@
const dbPort = ports[0];
this._dbPort = dbPort;
dbPort.addEventListener('message', (/** @type {MessageEvent<import('api').PmApiMessageAny>} */ event) => {
console.log('MediaDrawingWorker: message from database worker', event.data);
const message = event.data;
return invokeApiMapHandler(this._fromDatabaseApiMap, message.action, message.params, [event.ports], () => {});
});
dbPort.addEventListener('messageerror', (event) => {
const error = new ExtensionError('MediaDrawingWorker: Error receiving message from database worker');
error.data = event;
log.error(error);
});
dbPort.start();
console.log('MediaDrawingWorker: connected to database worker');
}

/**
Expand Down
4 changes: 4 additions & 0 deletions types/ext/api.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -390,6 +390,10 @@ type ApiSurface = {
params: void;
return: Language.LanguageSummary[];
};
heartbeat: {
params: void;
return: void;
};
};

type ApiExtraArgs = [sender: chrome.runtime.MessageSender];
Expand Down