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

Add safePerformance to guard against bad performance calls #1677

Merged
merged 9 commits into from
Dec 17, 2024
Merged

Add safePerformance to guard against bad performance calls #1677

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
4e3daa6
Disallow the use of performance
Kuuuube Dec 17, 2024
35c130f
Add safePerformance
Kuuuube Dec 17, 2024
4cbb2b2
Replace all performance with safePerformance
Kuuuube Dec 17, 2024
36aae7f
Disconnect safePerformance from Performance due to constructor issues
Kuuuube Dec 17, 2024
2eff89d
Fix missing import
Kuuuube Dec 17, 2024
6fc2914
Add safe-performance to eslintrc
Kuuuube Dec 17, 2024
7416f12
Ignore errors for using performance in dev directory
Kuuuube Dec 17, 2024
882ebc4
Merge remote-tracking branch 'upstream/master' into add-safe-perf
Kuuuube Dec 17, 2024
7b6dd22
Add missing imports
Kuuuube Dec 17, 2024
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
6 changes: 6 additions & 0 deletions .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,6 +79,10 @@
{
"message": "Avoid using Response.json(), prefer readResponseJson.",
"selector": "MemberExpression[property.name=json]"
},
{
"message": "Avoid using performance, prefer safePerformance.",
"selector": "MemberExpression[object.name=performance]"
}
],
"no-self-compare": "error",
Expand Down Expand Up @@ -582,6 +586,7 @@
"ext/js/core/extension-error.js",
"ext/js/core/json.js",
"ext/js/core/log.js",
"ext/js/core/safe-performance.js",
"ext/js/core/to-error.js",
"ext/js/core/utilities.js",
"ext/js/data/database.js",
Expand Down Expand Up @@ -619,6 +624,7 @@
"ext/js/core/log-utilities.js",
"ext/js/core/log.js",
"ext/js/core/object-utilities.js",
"ext/js/core/safe-performance.js",
"ext/js/core/to-error.js",
"ext/js/core/utilities.js",
"ext/js/data/anki-util.js",
Expand Down
3 changes: 3 additions & 0 deletions dev/bin/schema-validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,18 @@ function main() {
const schema = parseJson(schemaSource);

for (const dataFileName of args.slice(1)) {
// eslint-disable-next-line no-restricted-syntax
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible to have something specific to the performance custom rule?

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think so. I can't find anything in the eslint docs. But the name is a bit misleading. It does not disable all restricted syntax, it only disables the no-restricted-syntax rule. For Yomitan this means it only disables this:

"no-restricted-syntax": [
    "error",
    {
        "message": "Avoid using JSON.parse(), prefer parseJson.",
        "selector": "MemberExpression[object.name=JSON][property.name=parse]"
    },
    {
        "message": "Avoid using Response.json(), prefer readResponseJson.",
        "selector": "MemberExpression[property.name=json]"
    },
    {
        "message": "Avoid using performance, prefer safePerformance.",
        "selector": "MemberExpression[object.name=performance]"
    }
],

const start = performance.now();
try {
console.log(`Validating ${dataFileName}...`);
const dataSource = fs.readFileSync(dataFileName, {encoding: 'utf8'});
const data = parseJson(dataSource);
createJsonSchema(mode, schema).validate(data);
// eslint-disable-next-line no-restricted-syntax
const end = performance.now();
console.log(`No issues detected (${((end - start) / 1000).toFixed(2)}s)`);
} catch (e) {
// eslint-disable-next-line no-restricted-syntax
const end = performance.now();
console.log(`Encountered an error (${((end - start) / 1000).toFixed(2)}s)`);
console.warn(e);
Expand Down
3 changes: 3 additions & 0 deletions dev/dictionary-validate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,14 +131,17 @@ export async function testDictionaryFiles(mode, dictionaryFileNames) {
const schemas = getSchemas();

for (const dictionaryFileName of dictionaryFileNames) {
// eslint-disable-next-line no-restricted-syntax
const start = performance.now();
try {
console.log(`Validating ${dictionaryFileName}...`);
const source = fs.readFileSync(dictionaryFileName);
await validateDictionary(mode, source.buffer, schemas);
// eslint-disable-next-line no-restricted-syntax
const end = performance.now();
console.log(`No issues detected (${((end - start) / 1000).toFixed(2)}s)`);
} catch (e) {
// eslint-disable-next-line no-restricted-syntax
const end = performance.now();
console.log(`Encountered an error (${((end - start) / 1000).toFixed(2)}s)`);
console.warn(e);
Expand Down
7 changes: 4 additions & 3 deletions ext/js/app/frontend.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ import {createApiMap, invokeApiMapHandler} from '../core/api-map.js';
import {EventListenerCollection} from '../core/event-listener-collection.js';
import {log} from '../core/log.js';
import {promiseAnimationFrame} from '../core/promise-animation-frame.js';
import {safePerformance} from '../core/safe-performance.js';
import {setProfile} from '../data/profiles-util.js';
import {addFullscreenChangeEventListener, getFullscreenElement} from '../dom/document-util.js';
import {TextSourceElement} from '../dom/text-source-element.js';
Expand Down Expand Up @@ -952,13 +953,13 @@ export class Frontend {
* @returns {Promise<boolean>}
*/
async _scanSelectedText(allowEmptyRange, disallowExpandSelection, showEmpty = false) {
performance.mark('frontend:scanSelectedText:start');
safePerformance.mark('frontend:scanSelectedText:start');
const range = this._getFirstSelectionRange(allowEmptyRange);
if (range === null) { return false; }
const source = disallowExpandSelection ? TextSourceRange.createLazy(range) : TextSourceRange.create(range);
await this._textScanner.search(source, {focus: true, restoreSelection: true}, showEmpty);
performance.mark('frontend:scanSelectedText:end');
performance.measure('frontend:scanSelectedText', 'frontend:scanSelectedText:start', 'frontend:scanSelectedText:end');
safePerformance.mark('frontend:scanSelectedText:end');
safePerformance.measure('frontend:scanSelectedText', 'frontend:scanSelectedText:start', 'frontend:scanSelectedText:end');
return true;
}

Expand Down
3 changes: 2 additions & 1 deletion ext/js/app/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ import {DynamicProperty} from '../core/dynamic-property.js';
import {EventDispatcher} from '../core/event-dispatcher.js';
import {EventListenerCollection} from '../core/event-listener-collection.js';
import {ExtensionError} from '../core/extension-error.js';
import {safePerformance} from '../core/safe-performance.js';
import {deepEqual} from '../core/utilities.js';
import {addFullscreenChangeEventListener, computeZoomScale, convertRectZoomCoordinates, getFullscreenElement} from '../dom/document-util.js';
import {loadStyle} from '../dom/style-util.js';
Expand Down Expand Up @@ -302,7 +303,7 @@ export class Popup extends EventDispatcher {
await this._show(sourceRects, writingMode);

if (displayDetails !== null) {
performance.mark('invokeDisplaySetContent:start');
safePerformance.mark('invokeDisplaySetContent:start');
void this._invokeSafe('displaySetContent', {details: displayDetails});
}
}
Expand Down
3 changes: 2 additions & 1 deletion ext/js/comm/cross-frame-api.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import {EventListenerCollection} from '../core/event-listener-collection.js';
import {ExtensionError} from '../core/extension-error.js';
import {parseJson} from '../core/json.js';
import {log} from '../core/log.js';
import {safePerformance} from '../core/safe-performance.js';

/**
* @augments EventDispatcher<import('cross-frame-api').CrossFrameAPIPortEvents>
Expand Down Expand Up @@ -106,7 +107,7 @@ export class CrossFrameAPIPort extends EventDispatcher {
return;
}
}
performance.mark(`cross-frame-api:invoke:${action}`);
safePerformance.mark(`cross-frame-api:invoke:${action}`);
try {
this._port.postMessage(/** @type {import('cross-frame-api').InvokeMessage} */ ({type: 'invoke', id, data: {action, params}}));
} catch (e) {
Expand Down
4 changes: 3 additions & 1 deletion ext/js/core/promise-animation-frame.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/

import {safePerformance} from './safe-performance.js';

/**
* Creates a promise that will resolve after the next animation frame, using `requestAnimationFrame`.
* @param {number} [timeout] A maximum duration (in milliseconds) to wait until the promise resolves. If null or omitted, no timeout is used.
Expand Down Expand Up @@ -51,7 +53,7 @@ export function promiseAnimationFrame(timeout) {
cancelAnimationFrame(frameRequest);
frameRequest = null;
}
resolve({time: performance.now(), timeout: true});
resolve({time: safePerformance.now(), timeout: true});
};

frameRequest = requestAnimationFrame(onFrame);
Expand Down
68 changes: 68 additions & 0 deletions ext/js/core/safe-performance.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
/*
* Copyright (C) 2024 Yomitan Authors
*
* This program is free software: you can redistribute it and/or modify
* it under the terms of the GNU General Public License as published by
* the Free Software Foundation, either version 3 of the License, or
* (at your option) any later version.
*
* This program is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
* GNU General Public License for more details.
*
* You should have received a copy of the GNU General Public License
* along with this program. If not, see <https://www.gnu.org/licenses/>.
*/

import {log} from './log.js';

/**
* This class safely handles performance methods.
*/
class SafePerformance {
constructor() {}

/**
* @param {string} markName
* @param {PerformanceMarkOptions} [markOptions]
* @returns {PerformanceMark | undefined}
*/
mark(markName, markOptions) {
try {
// eslint-disable-next-line no-restricted-syntax
return performance.mark(markName, markOptions);
} catch (e) {
log.error(e);
}
}

/**
*
* @param {string} measureName
* @param {string | PerformanceMeasureOptions} [startOrMeasureOptions]
* @param {string} [endMark]
* @returns {PerformanceMeasure | undefined}
*/
measure(measureName, startOrMeasureOptions, endMark) {
try {
// eslint-disable-next-line no-restricted-syntax
return performance.measure(measureName, startOrMeasureOptions, endMark);
} catch (e) {
log.error(e);
}
}

/**
* @returns {DOMHighResTimeStamp}
*/
now() {
// eslint-disable-next-line no-restricted-syntax
return performance.now();
}
}

/**
* This object is the default performance measurer used by the runtime.
*/
export const safePerformance = new SafePerformance();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can we shadow performance instead of using a new variable and changing calls?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes but this would break the eslint rule. Checked to see if there's any way to ban the use of a specific class so we could ban Performance but I don't see any way to select that. Selecting performance will end up disallowing both the shadowed and not shadowed.

If the eslint rule is removed, this could work if you are suggesting making the new performance a global. It can't not be a global since any files that haven't imported the shadowing performance will be able to access the normal performance due to the missing eslint rule.

7 changes: 4 additions & 3 deletions ext/js/dictionary/dictionary-database.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/

import {log} from '../core/log.js';
import {safePerformance} from '../core/safe-performance.js';
import {stringReverse} from '../core/utilities.js';
import {Database} from '../data/database.js';

Expand Down Expand Up @@ -455,16 +456,16 @@ export class DictionaryDatabase {
* @returns {Promise<TResult[]>}
*/
_findMultiBulk(objectStoreName, indexNames, items, createQuery, predicate, createResult) {
performance.mark('findMultiBulk:start');
safePerformance.mark('findMultiBulk:start');
return new Promise((resolve, reject) => {
const itemCount = items.length;
const indexCount = indexNames.length;
/** @type {TResult[]} */
const results = [];
if (itemCount === 0 || indexCount === 0) {
resolve(results);
performance.mark('findMultiBulk:end');
performance.measure('findMultiBulk', 'findMultiBulk:start', 'findMultiBulk:end');
safePerformance.mark('findMultiBulk:end');
safePerformance.measure('findMultiBulk', 'findMultiBulk:start', 'findMultiBulk:end');
return;
}

Expand Down
13 changes: 7 additions & 6 deletions ext/js/display/display-generator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
*/

import {ExtensionError} from '../core/extension-error.js';
import {safePerformance} from '../core/safe-performance.js';
import {getDisambiguations, getGroupedPronunciations, getTermFrequency, groupKanjiFrequencies, groupTermFrequencies, groupTermTags, isNonNounVerbOrAdjective} from '../dictionary/dictionary-data-util.js';
import {HtmlTemplateCollection} from '../dom/html-template-collection.js';
import {distributeFurigana, getKanaMorae, getPitchCategory, isCodePointKanji} from '../language/ja/japanese.js';
Expand Down Expand Up @@ -111,23 +112,23 @@ export class DisplayGenerator {
node.dataset.groupedFrequencyCount = `${groupedFrequencies.length}`;
node.dataset.primaryMatchTypes = [...primaryMatchTypes].join(' ');

performance.mark('displayGenerator:createTermEntry:createTermHeadword:start');
safePerformance.mark('displayGenerator:createTermEntry:createTermHeadword:start');
for (let i = 0, ii = headwords.length; i < ii; ++i) {
const node2 = this._createTermHeadword(headwords[i], i, pronunciations);
node2.dataset.index = `${i}`;
headwordsContainer.appendChild(node2);
}
headwordsContainer.dataset.count = `${headwords.length}`;
performance.mark('displayGenerator:createTermEntry:createTermHeadword:end');
performance.measure('displayGenerator:createTermEntry:createTermHeadword', 'displayGenerator:createTermEntry:createTermHeadword:start', 'displayGenerator:createTermEntry:createTermHeadword:end');
safePerformance.mark('displayGenerator:createTermEntry:createTermHeadword:end');
safePerformance.measure('displayGenerator:createTermEntry:createTermHeadword', 'displayGenerator:createTermEntry:createTermHeadword:start', 'displayGenerator:createTermEntry:createTermHeadword:end');

performance.mark('displayGenerator:createTermEntry:promises:start');
safePerformance.mark('displayGenerator:createTermEntry:promises:start');
this._appendMultiple(inflectionRuleChainsContainer, this._createInflectionRuleChain.bind(this), inflectionRuleChainCandidates);
this._appendMultiple(frequencyGroupListContainer, this._createFrequencyGroup.bind(this), groupedFrequencies, false);
this._appendMultiple(groupedPronunciationsContainer, this._createGroupedPronunciation.bind(this), groupedPronunciations);
this._appendMultiple(headwordTagsContainer, this._createTermTag.bind(this), termTags, headwords.length);
performance.mark('displayGenerator:createTermEntry:promises:end');
performance.measure('displayGenerator:createTermEntry:promises', 'displayGenerator:createTermEntry:promises:start', 'displayGenerator:createTermEntry:promises:end');
safePerformance.mark('displayGenerator:createTermEntry:promises:end');
safePerformance.measure('displayGenerator:createTermEntry:promises', 'displayGenerator:createTermEntry:promises:start', 'displayGenerator:createTermEntry:promises:end');

for (const term of uniqueTerms) {
headwordTagsContainer.appendChild(this._createSearchTag(term));
Expand Down
Loading
Loading