hackademix / nscl

NoScript Commons Library
GNU General Public License v3.0
23 stars 9 forks source link

Optimize patchWindow injection on Chromium #6

Closed Martet closed 1 year ago

Martet commented 1 year ago

Hello, I am currently working on optimizing JShelter's performance as part of my final thesis and noticed a possible optimization for Chromium injection.

The patchingCallback code is inserted as a string into the page on Chromium, but directly called as a function on Firefox. Currently, a function is constructed from the code at the beginning of patchWindow, however, this is unnecessary on Chromium as the function will never be called, but converted back into a string. Replacing the function constructor with wrapping the code with the correct signature in string form accomplishes the same task, only much faster.

From my measurements, the function constructor call needlessly spends around 20ms parsing code generated by JShelter's "Recommended" level with FPD enabled (~560kB) on every frame load, so this is a nice speedup.

laniakea64 commented 1 year ago

Hi @hackademix , Please don't accept this pull request.

For one thing, it hides the source of error messages that originate from patch code, making such errors harder to catch and debug. To illustrate, consider the following content.js:

patchWindow('console.log("typo"");');

Current state:

Uncaught SyntaxError: missing ) after argument list
    at new Function (<anonymous>)
    at patchWindow (patchWindow.js:60:24)
    at content.js:1:1

where patchWindow.js:60:24 and content.js:1:1 are clickable links that take you to the relevant code. :+1:

WIth this pull request, it's just:

Uncaught SyntaxError: missing ) after argument list

... and this appears to be coming not from our test extension, but from the webpage - and the clickable link that's supposed to take you to the code goes nowhere.

Worse yet, this pull request makes the following code valid (!):

patchWindow('},{});let h=function(a,b){globalThis.nscl_env=b};h({');

Even if patchWindow isn't ever fed any potentially-untrusted input, this sort of thing being possible would be bad optics for a security- and privacy-oriented library like nscl, to the point of potentially undermining trust.

A 20ms performance boost isn't worth this.