Issue 29761612: Issue 6538, 6781 - Implement script compilation for snippets (Closed)
|
Created:
April 25, 2018, 8:03 p.m. by Manish Jethani Modified:
July 18, 2018, 10:43 p.m. CC:
sergei, hub, Erik Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionThis builds on https://codereview.adblockplus.org/29761597/
Patch Set 1 #Patch Set 2 : Clean up test #Patch Set 3 : Test script execution as well #
Total comments: 5
Patch Set 4 : Move compileScript to top level #Patch Set 5 : Rebase #
Total comments: 20
MessagesTotal messages: 16
Patch Set 1 This builds on https://codereview.adblockplus.org/29737558/ and https://codereview.adblockplus.org/29761597/ See issue description for specification.
Patch Set 2: Clean up test Patch Set 3: Test script execution as well
https://codereview.adblockplus.org/29761612/diff/29761623/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29761623/lib/snippets.js#new... lib/snippets.js:149: let imports = Object.create(null); There's an imports object and it gets populated by the "library". The library is just a JavaScript module like the following: // my-lib.js exports.foo = function() { console.log("foo"); }; Where does my-lib.js get the exports object? We pass our "imports" object to it as an argument, which becomes its exports object. This way we load multiple libraries and all of them populate the same exports object. The reason for supporting multiple libraries is that we want to have an initial local library (bundled with the extension) and then have a remote library loaded and updated at regular intervals, which overrides the local library. Also at some point we might want to split up our snippets into multiple libraries for organization purposes (and to make the downloads smaller so you only download what you need). https://codereview.adblockplus.org/29761612/diff/29761623/lib/snippets.js#new... lib/snippets.js:157: let value = imports[name]; Note that the function must exist directly on the imports object, hence no chance of calling window.eval and things like that. https://codereview.adblockplus.org/29761612/diff/29761623/lib/snippets.js#new... lib/snippets.js:159: value(...args); Also note that the function is called without a "this". It still has access to its file scope. //my-lib.js let foo = 0; exports.setFoo = function(value) { foo = value; } Calling setFoo here without a "this" will still set the value of foo. This is explicitly tested in the unit tests. Each script execution happens with a new instance of the library so the value of foo won't be retained between script executions. This is also explicitly tested in the unit tests. Basically we want things to be as stateless as possible; script execution on one site should not have any effect on other sites where the same script executes. State can be shared within the same script (e.g. "setFoo 123; checkFoo 123") but this is only incidental and not by design. We won't have any global variables in our snippets library so there will be nothing to share in practice. https://codereview.adblockplus.org/29761612/diff/29761623/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29761623/test/snippets.js#ne... test/snippets.js:18: /* eslint no-new-func: "off" */ ESLint won't let me use new Function(...) even though it's not the same as eval [1] [1]: https://stackoverflow.com/a/4599946 https://codereview.adblockplus.org/29761612/diff/29761623/test/snippets.js#ne... test/snippets.js:240: function verifyExecutable(script) I don't know how valuable this is because "template" is essentially copy-and-paste from the implementation.
Patch Set 5: Rebase
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); If I got it right, `compileScript` has a string representation of the library function, which we then inject into the generated code as a string using JSON.stringify, before evaling that string using the Function constructor. Couldn't we instead omit the JSON.stringify step, so that the library function was injected into the code as a function? https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); I wonder why we take include all libraries, instead of the ones that are being used? https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:178: if (Object.prototype.hasOwnProperty.call(imports, name)) Why not `name in imports`, you created the `imports` Object with no prototype. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:181: if (typeof value == "function") Could this check ever fail?
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/10 14:35:15, kzar wrote: > I wonder why we take include all libraries, instead of the ones that are being > used? The idea is that there will be a local library, which is shipped with the extension, and then there will be a remote version downloaded periodically. The remote version will contain overrides of functions in the local library. If there is no override, the local version of the function will be used. Anyway right now there is only one library. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/10 14:35:15, kzar wrote: > If I got it right, `compileScript` has a string representation of the library > function, which we then inject into the generated code as a string using > JSON.stringify, before evaling that string using the Function constructor. > Couldn't we instead omit the JSON.stringify step, so that the library function > was injected into the code as a function? libraries is an array of strings. The only way to safely encode a string into another string is using JSON.stringify. This function returns a string; it needs to include the original library code, but the code must be a string literal which we pass to the Function constructor. As for why we need the Function constructor: 1. It is safer, because the library code runs in global context and therefore cannot affect the surrounding variables 2. It's a neat way for the library to export functions into our imports object here https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:178: if (Object.prototype.hasOwnProperty.call(imports, name)) On 2018/07/10 14:35:15, kzar wrote: > Why not `name in imports`, you created the `imports` Object with no prototype. It's just safer this way. Right now it's a null prototype, maybe later somebody modifies the code so it's a regular object or an instance of some class. I'm trying to avoid accidental security issues as much as possible. There's no harm in doing it this way, on the other hand. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:181: if (typeof value == "function") On 2018/07/10 14:35:15, kzar wrote: > Could this check ever fail? Yes, a library could export a "constant" for whatever misguided reason. exports.MAX_INTERVAL = 100; So it's worth having the check. Basically this function places as little trust in the library code as it must. If a library is malicious it can still do anything, but at least we can try to avoid accidental failures.
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/12 09:48:32, Manish Jethani wrote: > On 2018/07/10 14:35:15, kzar wrote: > > If I got it right, `compileScript` has a string representation of the library > > function, which we then inject into the generated code as a string using > > JSON.stringify, before evaling that string using the Function constructor. > > Couldn't we instead omit the JSON.stringify step, so that the library function > > was injected into the code as a function? > > libraries is an array of strings. The only way to safely encode a string into > another string is using JSON.stringify. This function returns a string; it needs > to include the original library code, but the code must be a string literal > which we pass to the Function constructor. > > As for why we need the Function constructor: > > 1. It is safer, because the library code runs in global context and therefore > cannot affect the surrounding variables > 2. It's a neat way for the library to export functions into our imports object > here I still don't see the point of stringifying it, just to eval it again. Unless I'm mistaken we're doing something like this (contrived and untested, but you get the idea): let libraries = ["console.log("example");"]; eval(` let codeString = ${JSON.stringify(libraries)}[0]; eval(codeString); `); But I figure we could juts do something like this: let libraries = ["console.log("example");"]; eval(libraries[0]); https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:178: if (Object.prototype.hasOwnProperty.call(imports, name)) On 2018/07/12 09:48:31, Manish Jethani wrote: > On 2018/07/10 14:35:15, kzar wrote: > > Why not `name in imports`, you created the `imports` Object with no prototype. > > It's just safer this way. Right now it's a null prototype, maybe later somebody > modifies the code so it's a regular object or an instance of some class. I'm > trying to avoid accidental security issues as much as possible. There's no harm > in doing it this way, on the other hand. Alright, I disagree but won't insist. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:181: if (typeof value == "function") On 2018/07/12 09:48:31, Manish Jethani wrote: > On 2018/07/10 14:35:15, kzar wrote: > > Could this check ever fail? > > Yes, a library could export a "constant" for whatever misguided reason. > > exports.MAX_INTERVAL = 100; > > So it's worth having the check. > > Basically this function places as little trust in the library code as it must. > If a library is malicious it can still do anything, but at least we can try to > avoid accidental failures. Acknowledged.
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/12 10:21:43, kzar wrote: > On 2018/07/12 09:48:32, Manish Jethani wrote: > > On 2018/07/10 14:35:15, kzar wrote: > > > If I got it right, `compileScript` has a string representation of the > library > > > function, which we then inject into the generated code as a string using > > > JSON.stringify, before evaling that string using the Function constructor. > > > Couldn't we instead omit the JSON.stringify step, so that the library > function > > > was injected into the code as a function? > > > > libraries is an array of strings. The only way to safely encode a string into > > another string is using JSON.stringify. This function returns a string; it > needs > > to include the original library code, but the code must be a string literal > > which we pass to the Function constructor. > > > > As for why we need the Function constructor: > > > > 1. It is safer, because the library code runs in global context and > therefore > > cannot affect the surrounding variables > > 2. It's a neat way for the library to export functions into our imports > object > > here > > I still don't see the point of stringifying it, just to eval it again. Unless > I'm mistaken we're doing something like this (contrived and untested, but you > get the idea): > > let libraries = ["console.log("example");"]; > eval(` > let codeString = ${JSON.stringify(libraries)}[0]; > eval(codeString); > `); > > But I figure we could juts do something like this: > > let libraries = ["console.log("example");"]; > eval(libraries[0]); > The JSON.stringify is what converts your first snippet into your second snippet. The return value of this function is exactly what you are suggesting we should do, but to do that we need to insert the library code into the string as a literal. i.e. we are not actually calling eval, we are constructing a string that contains code that calls eval. The eval call is elsewhere, not in this function. Also an important point regarding eval: eval would execute the code in the local scope (so it could mess with local variables), which is what we don't want, that's why we use the Function constructor. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:178: if (Object.prototype.hasOwnProperty.call(imports, name)) On 2018/07/12 10:21:43, kzar wrote: > On 2018/07/12 09:48:31, Manish Jethani wrote: > > On 2018/07/10 14:35:15, kzar wrote: > > > Why not `name in imports`, you created the `imports` Object with no > prototype. > > > > It's just safer this way. Right now it's a null prototype, maybe later > somebody > > modifies the code so it's a regular object or an instance of some class. I'm > > trying to avoid accidental security issues as much as possible. There's no > harm > > in doing it this way, on the other hand. > > Alright, I disagree but won't insist. Acknowledged.
LGTM, would you mind also taking a look at `compileScript` Sebastian? https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/12 11:05:29, Manish Jethani wrote: > The JSON.stringify is what converts your first snippet into your second snippet. It's really not, but nevermind. > Also an important point regarding eval... Yes, I just used `eval` in my example for brevity, I don't have a strong opinion to use `eval` vs the `Function` constructor.
Thanks! I'll wait for Sebastian's feedback. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/12 11:19:55, kzar wrote: > On 2018/07/12 11:05:29, Manish Jethani wrote: > > The JSON.stringify is what converts your first snippet into your second > snippet. > > It's really not, but nevermind. OK, perhaps I am misunderstanding. If you can rewrite the function to not use JSON.stringify, it would help me understand what you mean. > > Also an important point regarding eval... > > Yes, I just used `eval` in my example for brevity, I don't have a strong opinion > to use `eval` vs the `Function` constructor. Acknowledged.
Can someone explain to me the concept of libraries, and how/where it will be used in practice? https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js#ne... test/snippets.js:223: test.equal(expected, actual); Instead of copying the boilerplate over to the tests and verifying that the output of the generated code matches, I wonder whether we should rather run the generated code and verify, that it behaves as expected?
https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); So we are generating code that generates code just for the matter that we can use the Function constructor? Are there any (significant) benefits over inlining the library code here? I could imagine that this would be somewhat faster, and might make things slightly easier to debug.
On 2018/07/16 16:37:36, Sebastian Noack wrote: > Can someone explain to me the concept of libraries, and how/where it will be > used in practice? A snippets library is just a JavaScript module that exports some functions. Here's what it looks like right now: https://github.com/adblockplus/adblockpluscore/blob/master/lib/content/snippe... The idea is that there will be at least two libraries, a local library that is shipped with the extension, and a remote library that is downloaded periodically and overrides exports from the local library. https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js File lib/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/lib/snippets.js#new... lib/snippets.js:174: new Function("exports", library)(imports); On 2018/07/16 19:14:53, Sebastian Noack wrote: > So we are generating code that generates code just for the matter that we can > use the Function constructor? Are there any (significant) benefits over inlining > the library code here? I could imagine that this would be somewhat faster, and > might make things slightly easier to debug. The library looks like this: https://github.com/adblockplus/adblockpluscore/blob/master/lib/content/snippe... It's a JavaScript module that exports functions. I suppose we could write something like this: return ` let exports = Object.create(null); ${libraries.map(library => `(function(){${library}})()`).join(";")}; // use exports `; But this is both uglier and more prone to error, the latter because the library would be able to access and modify state in the surrounding context. https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js#ne... test/snippets.js:223: test.equal(expected, actual); On 2018/07/16 16:37:35, Sebastian Noack wrote: > Instead of copying the boilerplate over to the tests and verifying that the > output of the generated code matches, I wonder whether we should rather run the > generated code and verify, that it behaves as expected? Let me try something. Yeah I agree that the current approach probably isn't the best way to do it.
https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js File test/snippets.js (right): https://codereview.adblockplus.org/29761612/diff/29787586/test/snippets.js#ne... test/snippets.js:223: test.equal(expected, actual); On 2018/07/17 19:57:15, Manish Jethani wrote: > On 2018/07/16 16:37:35, Sebastian Noack wrote: > > Instead of copying the boilerplate over to the tests and verifying that the > > output of the generated code matches, I wonder whether we should rather run > the > > generated code and verify, that it behaves as expected? > > Let me try something. > > Yeah I agree that the current approach probably isn't the best way to do it. Wait, this is what the next test does! It tests the _execution_ of the snippet. setFoo is supposed to set the value of an internal variable, and assertFoo is supposed to verify the value. We are already doing this.
It looks like Sebastian won't have time for this. Since Dave and I are both satisfied with this change, I'll close this now. If we need to make any improvements, we can still do it before the next release as a separate issue/patch. Thanks! 
|
