Issue 29886700: Issue 6969 - Implement abort-on-property-read snippet (Closed)
|
Created:
Sept. 20, 2018, 10:12 p.m. by hub Modified:
Nov. 14, 2018, 7:12 p.m. Reviewers:
Manish Jethani Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 6969 - Implement abort-on-property-read snippet
Patch Set 1 #Patch Set 2 : eslint fixes #Patch Set 3 : Allow multiple properties as arguments. #
Total comments: 23
Patch Set 4 : Addressed all review but the snippets parameters. #
Total comments: 2
Patch Set 5 : Revert to having one single property per call. #Patch Set 6 : Rebase #Patch Set 7 : Minor style changes #
Total comments: 4
Patch Set 8 : Split out the property wrap #
Total comments: 40
Patch Set 9 : Review changes #
Total comments: 6
Patch Set 10 : More changes from review #
Total comments: 2
Patch Set 11 : Bigger random ID #
Total comments: 5
Patch Set 12 : Update from review #
Total comments: 12
Patch Set 13 : Fixed jsdoc #
Total comments: 1
Patch Set 14 : Fix wrap property #
Total comments: 2
Patch Set 15 : Rework wrapPropertyAccess #
Total comments: 9
Patch Set 16 : Another round of changes #MessagesTotal messages: 38
This currently only handle single properties. not properties of properties.
I need to pass eslint first.
fixed eslint
now support multiple argument syntax.
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 chars [a-z0-9] Since we're not zero-padding the return value, does the number of characters here have any significance at all? I mean: (123).toString(36) Gives `3f`. It's not 6 characters. I think we should just do something like `Math.random().toString(36).substring(2)` https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) The problem with accepting a variable number of arguments here is that we can't extend the function to add more arguments that specify some conditions. It would also break compatibility with uBO, so that if uBO adds an additional argument we won't be able to add it for compatibility. So I think we should just keep it compatible with uBO and take only one argument. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:467: window.onerror = (msg, ...args) => Let's rename this to `message` and `...rest`. About the `...rest` bit, I think we have a convention that if it's "rest of the arguments" then we use the name `rest`. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:469: if (typeof msg === "string" && msg.indexOf(magic) != -1) Strict equality is unnecessary here. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:469: if (typeof msg === "string" && msg.indexOf(magic) != -1) We could use `String.includes` instead of `String.indexOf`. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:471: if (onerror) Just to be safe, we should check `typeof onerror == "function"` here. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:472: return onerror(msg, ...args); Let's just also set the `this` for this call, so `onerror.call(this, message, ...args)`. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:479: if (!d || d.get !== abort) Strict inequality is unnecessary here.
updated patch https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 chars [a-z0-9] On 2018/09/24 11:08:35, Manish Jethani wrote: > Since we're not zero-padding the return value, does the number of characters > here have any significance at all? > > I mean: > > (123).toString(36) > > Gives `3f`. It's not 6 characters. > > I think we should just do something like > `Math.random().toString(36).substring(2)` Modifying the formula so that if Math.random() returns 0 we still get 6 characters. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/09/24 11:08:35, Manish Jethani wrote: > The problem with accepting a variable number of arguments here is that we can't > extend the function to add more arguments that specify some conditions. It would > also break compatibility with uBO, so that if uBO adds an additional argument we > won't be able to add it for compatibility. > > So I think we should just keep it compatible with uBO and take only one > argument. I didn't have uBO compatibility in mind. I just allow a list of properties as an argument as to be able to just group all in one filter. This was a suggestion from Amr. But I'm ok to revert to one propertie per invocation if you think it's better. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:467: window.onerror = (msg, ...args) => On 2018/09/24 11:08:35, Manish Jethani wrote: > Let's rename this to `message` and `...rest`. About the `...rest` bit, I think > we have a convention that if it's "rest of the arguments" then we use the name > `rest`. Done. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:469: if (typeof msg === "string" && msg.indexOf(magic) != -1) On 2018/09/24 11:08:35, Manish Jethani wrote: > Strict equality is unnecessary here. Done. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:469: if (typeof msg === "string" && msg.indexOf(magic) != -1) On 2018/09/24 11:08:35, Manish Jethani wrote: > We could use `String.includes` instead of `String.indexOf`. Done. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:471: if (onerror) On 2018/09/24 11:08:35, Manish Jethani wrote: > Just to be safe, we should check `typeof onerror == "function"` here. Done. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:472: return onerror(msg, ...args); On 2018/09/24 11:08:35, Manish Jethani wrote: > Let's just also set the `this` for this call, so `onerror.call(this, message, > ...args)`. Done. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:479: if (!d || d.get !== abort) On 2018/09/24 11:08:35, Manish Jethani wrote: > Strict inequality is unnecessary here. Done.
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 chars [a-z0-9] On 2018/09/25 04:28:52, hub wrote: > On 2018/09/24 11:08:35, Manish Jethani wrote: > > Since we're not zero-padding the return value, does the number of characters > > here have any significance at all? > > > > I mean: > > > > (123).toString(36) > > > > Gives `3f`. It's not 6 characters. > > > > I think we should just do something like > > `Math.random().toString(36).substring(2)` > > Modifying the formula so that if Math.random() returns 0 we still get 6 > characters. The updated version looks alright to me, it always generates a string that is 6 characters long, but I still wonder why it must be 6 characters only. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/09/25 04:28:52, hub wrote: > On 2018/09/24 11:08:35, Manish Jethani wrote: > > The problem with accepting a variable number of arguments here is that we > can't > > extend the function to add more arguments that specify some conditions. It > would > > also break compatibility with uBO, so that if uBO adds an additional argument > we > > won't be able to add it for compatibility. > > > > So I think we should just keep it compatible with uBO and take only one > > argument. > > I didn't have uBO compatibility in mind. I just allow a list of properties as an > argument as to be able to just group all in one filter. > > This was a suggestion from Amr. But I'm ok to revert to one propertie per > invocation if you think it's better. The thing is that the moment ABP starts using the technique that works for uBO, they will circumvent it, and uBO may respond with an updated snippet. Then we'll also have to implement the updated technique, which might have additional parameters. Since the snippet has the same name as the uBO version, it would be better to just stay fully compatible. So I think at least for now it's better to just keep it at a single parameter. https://codereview.adblockplus.org/29886700/diff/29890637/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29890637/lib/content/snippet... lib/content/snippets.js:446: return (Math.floor(Math.random() * 2116316160 + 60466176)).toString(36); Nit: The parentheses around `Math.foor(...)` are unnecessary.
Updated patch https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:442: // 2176782336 is 36^6 which mean 6 chars [a-z0-9] On 2018/09/25 12:25:22, Manish Jethani wrote: > On 2018/09/25 04:28:52, hub wrote: > > On 2018/09/24 11:08:35, Manish Jethani wrote: > > > Since we're not zero-padding the return value, does the number of characters > > > here have any significance at all? > > > > > > I mean: > > > > > > (123).toString(36) > > > > > > Gives `3f`. It's not 6 characters. > > > > > > I think we should just do something like > > > `Math.random().toString(36).substring(2)` > > > > Modifying the formula so that if Math.random() returns 0 we still get 6 > > characters. > > The updated version looks alright to me, it always generates a string that is 6 > characters long, but I still wonder why it must be 6 characters only. 6 is arbitrary. Long enough to be unique, but still short enough. https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/09/25 12:25:22, Manish Jethani wrote: > On 2018/09/25 04:28:52, hub wrote: > > On 2018/09/24 11:08:35, Manish Jethani wrote: > > > The problem with accepting a variable number of arguments here is that we > > can't > > > extend the function to add more arguments that specify some conditions. It > > would > > > also break compatibility with uBO, so that if uBO adds an additional > argument > > we > > > won't be able to add it for compatibility. > > > > > > So I think we should just keep it compatible with uBO and take only one > > > argument. > > > > I didn't have uBO compatibility in mind. I just allow a list of properties as > an > > argument as to be able to just group all in one filter. > > > > This was a suggestion from Amr. But I'm ok to revert to one propertie per > > invocation if you think it's better. > > The thing is that the moment ABP starts using the technique that works for uBO, > they will circumvent it, and uBO may respond with an updated snippet. Then we'll > also have to implement the updated technique, which might have additional > parameters. Since the snippet has the same name as the uBO version, it would be > better to just stay fully compatible. > > So I think at least for now it's better to just keep it at a single parameter. fair enough. will fix it. https://codereview.adblockplus.org/29886700/diff/29890637/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29890637/lib/content/snippet... lib/content/snippets.js:446: return (Math.floor(Math.random() * 2116316160 + 60466176)).toString(36); On 2018/09/25 12:25:22, Manish Jethani wrote: > Nit: The parentheses around `Math.foor(...)` are unnecessary. Done.
Are these inline scripts, by the way? If they are scripts loaded off the network, why don't we just block them with a regular blocking filter?
On 2018/10/02 08:51:03, Manish Jethani wrote: > Are these inline scripts, by the way? If they are scripts loaded off the > network, why don't we just block them with a regular blocking filter? It is more granular than block the script which: 1. could have more code some being actually relevant to some feature 2. script could be first-party and with 1. might be a big issue to block it. Also it is a similar technique than used by uBlock Origin, since we insist on being compatible it fit right in place. Note: this snippet is usable for a lot of other ad providers too.
On 2018/10/02 17:01:50, hub wrote: > On 2018/10/02 08:51:03, Manish Jethani wrote: > > Are these inline scripts, by the way? If they are scripts loaded off the > > network, why don't we just block them with a regular blocking filter? > > It is more granular than block the script which: > > 1. could have more code some being actually relevant to some feature > 2. script could be first-party and with 1. might be a big issue to block it. > > Also it is a similar technique than used by uBlock Origin, since we insist on > being compatible it fit right in place. > > Note: this snippet is usable for a lot of other ad providers too. I see, thanks. Makes sense.
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/09/25 15:40:43, hub wrote: > On 2018/09/25 12:25:22, Manish Jethani wrote: > > On 2018/09/25 04:28:52, hub wrote: > > > On 2018/09/24 11:08:35, Manish Jethani wrote: > > > > The problem with accepting a variable number of arguments here is that we > > > can't > > > > extend the function to add more arguments that specify some conditions. It > > > would > > > > also break compatibility with uBO, so that if uBO adds an additional > > argument > > > we > > > > won't be able to add it for compatibility. > > > > > > > > So I think we should just keep it compatible with uBO and take only one > > > > argument. > > > > > > I didn't have uBO compatibility in mind. I just allow a list of properties > as > > an > > > argument as to be able to just group all in one filter. > > > > > > This was a suggestion from Amr. But I'm ok to revert to one propertie per > > > invocation if you think it's better. > > > > The thing is that the moment ABP starts using the technique that works for > uBO, > > they will circumvent it, and uBO may respond with an updated snippet. Then > we'll > > also have to implement the updated technique, which might have additional > > parameters. Since the snippet has the same name as the uBO version, it would > be > > better to just stay fully compatible. > > > > So I think at least for now it's better to just keep it at a single parameter. > > fair enough. will fix it. Happy to proceed whenever you're ready.
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/10/22 14:21:01, Manish Jethani wrote: > On 2018/09/25 15:40:43, hub wrote: > > On 2018/09/25 12:25:22, Manish Jethani wrote: > > > On 2018/09/25 04:28:52, hub wrote: > > > > On 2018/09/24 11:08:35, Manish Jethani wrote: > > > > > The problem with accepting a variable number of arguments here is that > we > > > > can't > > > > > extend the function to add more arguments that specify some conditions. > It > > > > would > > > > > also break compatibility with uBO, so that if uBO adds an additional > > > argument > > > > we > > > > > won't be able to add it for compatibility. > > > > > > > > > > So I think we should just keep it compatible with uBO and take only one > > > > > argument. > > > > > > > > I didn't have uBO compatibility in mind. I just allow a list of properties > > as > > > an > > > > argument as to be able to just group all in one filter. > > > > > > > > This was a suggestion from Amr. But I'm ok to revert to one propertie per > > > > invocation if you think it's better. > > > > > > The thing is that the moment ABP starts using the technique that works for > > uBO, > > > they will circumvent it, and uBO may respond with an updated snippet. Then > > we'll > > > also have to implement the updated technique, which might have additional > > > parameters. Since the snippet has the same name as the uBO version, it would > > be > > > better to just stay fully compatible. > > > > > > So I think at least for now it's better to just keep it at a single > parameter. > > > > fair enough. will fix it. > > Happy to proceed whenever you're ready. This was fixed in Patch Set 5
https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29887600/lib/content/snippet... lib/content/snippets.js:454: function abortOnPropertyRead(...props) On 2018/10/22 14:52:49, hub wrote: > On 2018/10/22 14:21:01, Manish Jethani wrote: > > On 2018/09/25 15:40:43, hub wrote: > > > On 2018/09/25 12:25:22, Manish Jethani wrote: > > > > On 2018/09/25 04:28:52, hub wrote: > > > > > On 2018/09/24 11:08:35, Manish Jethani wrote: > > > > > > The problem with accepting a variable number of arguments here is that > > we > > > > > can't > > > > > > extend the function to add more arguments that specify some > conditions. > > It > > > > > would > > > > > > also break compatibility with uBO, so that if uBO adds an additional > > > > argument > > > > > we > > > > > > won't be able to add it for compatibility. > > > > > > > > > > > > So I think we should just keep it compatible with uBO and take only > one > > > > > > argument. > > > > > > > > > > I didn't have uBO compatibility in mind. I just allow a list of > properties > > > as > > > > an > > > > > argument as to be able to just group all in one filter. > > > > > > > > > > This was a suggestion from Amr. But I'm ok to revert to one propertie > per > > > > > invocation if you think it's better. > > > > > > > > The thing is that the moment ABP starts using the technique that works for > > > uBO, > > > > they will circumvent it, and uBO may respond with an updated snippet. Then > > > we'll > > > > also have to implement the updated technique, which might have additional > > > > parameters. Since the snippet has the same name as the uBO version, it > would > > > be > > > > better to just stay fully compatible. > > > > > > > > So I think at least for now it's better to just keep it at a single > > parameter. > > > > > > fair enough. will fix it. > > > > Happy to proceed whenever you're ready. > > This was fixed in Patch Set 5 Sorry, I missed this. I'll look at this patch tomorrow for sure.
https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippet... lib/content/snippets.js:530: return onerror(this, message, ...rest); This needs to be onerror.call https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippet... lib/content/snippets.js:533: (function(o, p) I think this should be a separate function: function wrapPropertyAccess(object, property, descriptor) If the descriptor does not contain a specific accessor (get or set), it should be replaced with a passthrough (forward the call to the original descriptor). If it contains a specific accessor (get or set) but the value is null, it should use the null value (meaning don't include that accessor).
https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippet... lib/content/snippets.js:530: return onerror(this, message, ...rest); On 2018/10/23 15:31:24, Manish Jethani wrote: > This needs to be onerror.call Done. https://codereview.adblockplus.org/29886700/diff/29900555/lib/content/snippet... lib/content/snippets.js:533: (function(o, p) On 2018/10/23 15:31:24, Manish Jethani wrote: > I think this should be a separate function: > > function wrapPropertyAccess(object, property, descriptor) > > If the descriptor does not contain a specific accessor (get or set), it should > be replaced with a passthrough (forward the call to the original descriptor). If > it contains a specific accessor (get or set) but the value is null, it should > use the null value (meaning don't include that accessor). Done.
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:500: // for when Math.random() returns 0.0 The last line of the comment here, it's a bit misleading because it's not just for when Math.random() returns 0.0. Right? I think we could leave this part out. We could add that the range of values in base 36 is 100000-zzzzzz (both inclusive). https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:504: function wrapPropertyAccess(object, property, desc) At least for function parameters, let's stick with the current practice throughout this file, which is to use fuller names (descriptor rather than desc). https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:507: let d = Object.getOwnPropertyDescriptor(object, property); Maybe we should call it "currentDescriptor" instead of just "d"? https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:522: /** Nit: We should leave a blank line here. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:523: * Will patch a property on the window object to abort when read. Regarding the JSDoc comment for this snippet, my thinking was that we should not take on the burden of documenting uBlock Origin's snippets and then keeping the documentation up to date. Instead, we can simply point to the original implementation, like I have done here: https://hg.adblockplus.org/adblockpluscore/file/7126b8cc2fad/lib/content/snip... This would make it easier to maintain these snippets over time (one less thing to worry about), while also clearly distinguishing them from our own ideas. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:530: function abortOnPropertyRead(prop) Let's rename "prop" to "property" since we are using fuller names throughout this file (e.g. "element" rather than "elem"). https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:537: function canceller() How about we just call this function "abort", which matches the name of the snippet? https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:547: if (onerror && typeof onerror == "function") I don't think the first condition here is necessary. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); I just realized that you can have a function that has no call method (if the prototype has been changed). We should change this to `(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:551: let d = {get: canceller, set() {}}; We don't need this temporary variable, in my opinion. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:551: let d = {get: canceller, set() {}}; I feel like we're going to need this noop function a lot. What do you think about declaring it as a separate function in the file? /** * Does nothing. */ function noop() { } Then I think this would look like: wrapPropertyAccess(window, property, {get: abort, set: noop});
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:542: let {onerror} = window; Hmm ... I think we're going to need to repeat this code as well, so maybe this could also be a separate function: function makeAborter() { let magic = randomId(); window.onerror = ...; return function() { throw new ReferenceError(magic); }; } Then abortOnPropertyRead would be just this: if (!property) return; wrapPropertyAccess(window, property, {get: makeAborter(), set: noop}); What do you think? Or is this overkill? I feel like we might need to do this eventually.
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:542: let {onerror} = window; On 2018/10/24 21:01:48, Manish Jethani wrote: > Hmm ... I think we're going to need to repeat this code as well, so maybe this > could also be a separate function: > > function makeAborter() > { > let magic = randomId(); > > window.onerror = ...; > > return function() > { > throw new ReferenceError(magic); > }; > } > > Then abortOnPropertyRead would be just this: > > if (!property) > return; > > wrapPropertyAccess(window, property, {get: makeAborter(), set: noop}); > > What do you think? Or is this overkill? I feel like we might need to do this > eventually. Looking at the other patch, yeah I think we should do this, but makeAborter should take an optional callback as an argument. function makeAborter(shouldAbort = () => true) { ... return function() { if (shouldAbort()) throw new ReferenceError(magic); }; }
Updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:500: // for when Math.random() returns 0.0 On 2018/10/24 20:47:45, Manish Jethani wrote: > The last line of the comment here, it's a bit misleading because it's not just > for when Math.random() returns 0.0. Right? I think we could leave this part out. > > We could add that the range of values in base 36 is 100000-zzzzzz (both > inclusive). reworded the comments. Also added a proper JSDoc at the top. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:504: function wrapPropertyAccess(object, property, desc) On 2018/10/24 20:47:44, Manish Jethani wrote: > At least for function parameters, let's stick with the current practice > throughout this file, which is to use fuller names (descriptor rather than > desc). Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:507: let d = Object.getOwnPropertyDescriptor(object, property); On 2018/10/24 20:47:45, Manish Jethani wrote: > Maybe we should call it "currentDescriptor" instead of just "d"? Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:522: /** On 2018/10/24 20:47:45, Manish Jethani wrote: > Nit: We should leave a blank line here. Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:523: * Will patch a property on the window object to abort when read. On 2018/10/24 20:47:44, Manish Jethani wrote: > Regarding the JSDoc comment for this snippet, my thinking was that we should not > take on the burden of documenting uBlock Origin's snippets and then keeping the > documentation up to date. Instead, we can simply point to the original > implementation, like I have done here: > > https://hg.adblockplus.org/adblockpluscore/file/7126b8cc2fad/lib/content/snip... > > This would make it easier to maintain these snippets over time (one less thing > to worry about), while also clearly distinguishing them from our own ideas. I disagree. This is documenting this code. It is easier to read local documentation than something somewhere else. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:530: function abortOnPropertyRead(prop) On 2018/10/24 20:47:45, Manish Jethani wrote: > Let's rename "prop" to "property" since we are using fuller names throughout > this file (e.g. "element" rather than "elem"). Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:537: function canceller() On 2018/10/24 20:47:45, Manish Jethani wrote: > How about we just call this function "abort", which matches the name of the > snippet? Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:542: let {onerror} = window; On 2018/10/24 21:12:28, Manish Jethani wrote: > On 2018/10/24 21:01:48, Manish Jethani wrote: > > Hmm ... I think we're going to need to repeat this code as well, so maybe this > > could also be a separate function: > > > > function makeAborter() > > { > > let magic = randomId(); > > > > window.onerror = ...; > > > > return function() > > { > > throw new ReferenceError(magic); > > }; > > } > > > > Then abortOnPropertyRead would be just this: > > > > if (!property) > > return; > > > > wrapPropertyAccess(window, property, {get: makeAborter(), set: noop}); > > > > What do you think? Or is this overkill? I feel like we might need to do this > > eventually. > > Looking at the other patch, yeah I think we should do this, but makeAborter > should take an optional callback as an argument. > > function makeAborter(shouldAbort = () => true) > { > ... > > return function() > { > if (shouldAbort()) > throw new ReferenceError(magic); > }; > } All of this make the whole thing more complicated, more subject to side effects. Simple contained pieces of code are IMHO cleaner. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:542: let {onerror} = window; On 2018/10/24 21:01:48, Manish Jethani wrote: > What do you think? Or is this overkill? I feel like we might need to do this > eventually. I think it is. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:547: if (onerror && typeof onerror == "function") On 2018/10/24 20:47:45, Manish Jethani wrote: > I don't think the first condition here is necessary. Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/24 20:47:45, Manish Jethani wrote: > I just realized that you can have a function that has no call method (if the > prototype has been changed). We should change this to > `(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion. Actually I think I went a bit overboard. I don't even need `this`. In this case `onerror` is a function that will just be called as is. So I'll change this to `return onerror(message, ...rest);`. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:551: let d = {get: canceller, set() {}}; On 2018/10/24 20:47:44, Manish Jethani wrote: > We don't need this temporary variable, in my opinion. Done. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:551: let d = {get: canceller, set() {}}; On 2018/10/24 20:47:45, Manish Jethani wrote: > I feel like we're going to need this noop function a lot. What do you think > about declaring it as a separate function in the file? > > /** > * Does nothing. > */ > function noop() > { > } > > Then I think this would look like: > > wrapPropertyAccess(window, property, {get: abort, set: noop}); > I don't think this make it any clearer.
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:523: * Will patch a property on the window object to abort when read. On 2018/10/24 22:17:37, hub wrote: > On 2018/10/24 20:47:44, Manish Jethani wrote: > > Regarding the JSDoc comment for this snippet, my thinking was that we should > not > > take on the burden of documenting uBlock Origin's snippets and then keeping > the > > documentation up to date. Instead, we can simply point to the original > > implementation, like I have done here: > > > > > https://hg.adblockplus.org/adblockpluscore/file/7126b8cc2fad/lib/content/snip... > > > > This would make it easier to maintain these snippets over time (one less thing > > to worry about), while also clearly distinguishing them from our own ideas. > > I disagree. This is documenting this code. It is easier to read local > documentation than something somewhere else. Alright, so we need to credit uBlock Origin for the idea. How do you propose we do this? https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:542: let {onerror} = window; On 2018/10/24 22:17:38, hub wrote: > On 2018/10/24 21:12:28, Manish Jethani wrote: > > On 2018/10/24 21:01:48, Manish Jethani wrote: > > > Hmm ... I think we're going to need to repeat this code as well, so maybe > this > > > could also be a separate function: > > > > > > function makeAborter() > > > { > > > let magic = randomId(); > > > > > > window.onerror = ...; > > > > > > return function() > > > { > > > throw new ReferenceError(magic); > > > }; > > > } > > > > > > Then abortOnPropertyRead would be just this: > > > > > > if (!property) > > > return; > > > > > > wrapPropertyAccess(window, property, {get: makeAborter(), set: noop}); > > > > > > What do you think? Or is this overkill? I feel like we might need to do this > > > eventually. > > > > Looking at the other patch, yeah I think we should do this, but makeAborter > > should take an optional callback as an argument. > > > > function makeAborter(shouldAbort = () => true) > > { > > ... > > > > return function() > > { > > if (shouldAbort()) > > throw new ReferenceError(magic); > > }; > > } > > All of this make the whole thing more complicated, more subject to side effects. > Simple contained pieces of code are IMHO cleaner. So you intend to repeat this window.onerror boilerplate in every snippet that does something like this (and you already have another one in review)? I don't really understand how that's better, but OK. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:545: if (typeof message == "string" && message.includes(rid)) I think we should check specifically for "ReferenceError: [id]" here. I can imagine some other errors getting ignored because of this by accident. So `new RegExp("\\bReferenceError: " + id "\\b").test(message)` https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/24 22:17:38, hub wrote: > On 2018/10/24 20:47:45, Manish Jethani wrote: > > I just realized that you can have a function that has no call method (if the > > prototype has been changed). We should change this to > > `(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion. > > Actually I think I went a bit overboard. I don't even need `this`. In this case > `onerror` is a function that will just be called as is. So I'll change this to > `return onerror(message, ...rest);`. You did not go overboard, because in strict mode the `this` for the function points to the window object, and if you don't pass it then it will be undefined, which could cause the function to fail (if it accesses `this`). We have to faithfully pass on `this` to the original handler. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:551: let d = {get: canceller, set() {}}; On 2018/10/24 22:17:38, hub wrote: > On 2018/10/24 20:47:45, Manish Jethani wrote: > > I feel like we're going to need this noop function a lot. What do you think > > about declaring it as a separate function in the file? > > > > /** > > * Does nothing. > > */ > > function noop() > > { > > } > > > > Then I think this would look like: > > > > wrapPropertyAccess(window, property, {get: abort, set: noop}); > > > > I don't think this make it any clearer. Acknowledged. https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... lib/content/snippets.js:496: * Generate an alpha-numeric id 6 chars long, in the range 100000..zzzzzz. Some nits: s/Generate/Generates/ s/an alpha-numeric id 6 chars long .../a random alphanumeric ID consisting of 6 base-36 digits from the range 100000..zzzzzz (both inclusive)./ s/{string} the random id./{string} The random ID./ https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... lib/content/snippets.js:511: // simple property Do we need this comment? The code is quite straightforward in what it does. https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... lib/content/snippets.js:517: if (currentDescriptor.get != descriptor.get) I'm not seeing the point of this check.
Updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:523: * Will patch a property on the window object to abort when read. On 2018/10/24 23:02:09, Manish Jethani wrote: > On 2018/10/24 22:17:37, hub wrote: > > On 2018/10/24 20:47:44, Manish Jethani wrote: > > > Regarding the JSDoc comment for this snippet, my thinking was that we should > > not > > > take on the burden of documenting uBlock Origin's snippets and then keeping > > the > > > documentation up to date. Instead, we can simply point to the original > > > implementation, like I have done here: > > > > > > > > > https://hg.adblockplus.org/adblockpluscore/file/7126b8cc2fad/lib/content/snip... > > > > > > This would make it easier to maintain these snippets over time (one less > thing > > > to worry about), while also clearly distinguishing them from our own ideas. > > > > I disagree. This is documenting this code. It is easier to read local > > documentation than something somewhere else. > > Alright, so we need to credit uBlock Origin for the idea. How do you propose we > do this? Added a link to uBlock. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:545: if (typeof message == "string" && message.includes(rid)) On 2018/10/24 23:02:09, Manish Jethani wrote: > I think we should check specifically for "ReferenceError: [id]" here. I can > imagine some other errors getting ignored because of this by accident. > > So `new RegExp("\\bReferenceError: " + id "\\b").test(message)` No. Because the message format isn't part of any standard, so it would break at any implementation change. I saw a bug recently in Mozilla's bugzilla where some framework assumed this and broke when change were made in Firefox error reporting. The chances to match the random id to another message are quite thin. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/24 23:02:09, Manish Jethani wrote: > On 2018/10/24 22:17:38, hub wrote: > > On 2018/10/24 20:47:45, Manish Jethani wrote: > > > I just realized that you can have a function that has no call method (if the > > > prototype has been changed). We should change this to > > > `(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion. > > > > Actually I think I went a bit overboard. I don't even need `this`. In this > case > > `onerror` is a function that will just be called as is. So I'll change this to > > `return onerror(message, ...rest);`. > > You did not go overboard, because in strict mode the `this` for the function > points to the window object, and if you don't pass it then it will be undefined, > which could cause the function to fail (if it accesses `this`). We have to > faithfully pass on `this` to the original handler. No. If the original handler is bound to something, that bind is retained. https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror clearly show there is not expectation to be bound to anything. https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... lib/content/snippets.js:496: * Generate an alpha-numeric id 6 chars long, in the range 100000..zzzzzz. On 2018/10/24 23:02:10, Manish Jethani wrote: > Some nits: > > s/Generate/Generates/ > > s/an alpha-numeric id 6 chars long .../a random alphanumeric ID consisting of 6 > base-36 digits from the range 100000..zzzzzz (both inclusive)./ > > s/{string} the random id./{string} The random ID./ Done. https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... lib/content/snippets.js:511: // simple property On 2018/10/24 23:02:10, Manish Jethani wrote: > Do we need this comment? The code is quite straightforward in what it does. was carried over from other changes. removed it. https://codereview.adblockplus.org/29886700/diff/29923592/lib/content/snippet... lib/content/snippets.js:517: if (currentDescriptor.get != descriptor.get) On 2018/10/24 23:02:10, Manish Jethani wrote: > I'm not seeing the point of this check. Done.
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:545: if (typeof message == "string" && message.includes(rid)) On 2018/10/25 14:32:26, hub wrote: > On 2018/10/24 23:02:09, Manish Jethani wrote: > > I think we should check specifically for "ReferenceError: [id]" here. I can > > imagine some other errors getting ignored because of this by accident. > > > > So `new RegExp("\\bReferenceError: " + id "\\b").test(message)` > > No. Because the message format isn't part of any standard, so it would break at > any implementation change. > I saw a bug recently in Mozilla's bugzilla where some framework assumed this and > broke when change were made in Firefox error reporting. > > The chances to match the random id to another message are quite thin. Well it's OK for this to not work and for an error to be thrown instead, the opposite it not OK (catching and ignoring errors from other people's code). The "magic" string is literally just a string of 6 digits, that can occur in any error that deals with numbers. Another idea is to use 30 digits instead of 6, which would make it more unique. The current information content of the magic string is `log2(36) * 6 = ~31 bits` (this assumes Math.random() is cryptographically random, which is also not the case). We could just add more base-36 digits to make it more unique. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/25 14:32:25, hub wrote: > On 2018/10/24 23:02:09, Manish Jethani wrote: > > On 2018/10/24 22:17:38, hub wrote: > > > On 2018/10/24 20:47:45, Manish Jethani wrote: > > > > I just realized that you can have a function that has no call method (if > the > > > > prototype has been changed). We should change this to > > > > `(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion. > > > > > > Actually I think I went a bit overboard. I don't even need `this`. In this > > case > > > `onerror` is a function that will just be called as is. So I'll change this > to > > > `return onerror(message, ...rest);`. > > > > You did not go overboard, because in strict mode the `this` for the function > > points to the window object, and if you don't pass it then it will be > undefined, > > which could cause the function to fail (if it accesses `this`). We have to > > faithfully pass on `this` to the original handler. > > No. If the original handler is bound to something, that bind is retained. How can we assume that the original handler will be bound to anything? Take this simple case: <script> "use strict"; window.onerror = function() { this.location = "/error-page"; }; </script> This is going to bomb if you call it the way you intend to. > https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/onerror > clearly show there is not expectation to be bound to anything. It says nothing about `this`. In practice the function is called by default, as I said, with `this` set to the window object. https://codereview.adblockplus.org/29886700/diff/29924564/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29924564/lib/content/snippet... lib/content/snippets.js:496: * Generates a random alphanumeric ID consisting of 6 chars base-36 digits s/chars//
Updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:545: if (typeof message == "string" && message.includes(rid)) On 2018/10/25 16:12:15, Manish Jethani wrote: > We could just add more base-36 digits to make it more unique. I'll make 5 times these. Should be random enough. https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/25 16:12:15, Manish Jethani wrote: > On 2018/10/25 14:32:25, hub wrote: > > On 2018/10/24 23:02:09, Manish Jethani wrote: > > > On 2018/10/24 22:17:38, hub wrote: > > > > On 2018/10/24 20:47:45, Manish Jethani wrote: > > > > > I just realized that you can have a function that has no call method (if > > the > > > > > prototype has been changed). We should change this to > > > > > `(() => {}).call.call(onerror, this, message, ...rest)`, in my opinion. > > > > > > > > Actually I think I went a bit overboard. I don't even need `this`. In this > > > case > > > > `onerror` is a function that will just be called as is. So I'll change > this > > to > > > > `return onerror(message, ...rest);`. > > > > > > You did not go overboard, because in strict mode the `this` for the function > > > points to the window object, and if you don't pass it then it will be > > undefined, > > > which could cause the function to fail (if it accesses `this`). We have to > > > faithfully pass on `this` to the original handler. > > > > No. If the original handler is bound to something, that bind is retained. > > How can we assume that the original handler will be bound to anything? > > Take this simple case: > > <script> > "use strict"; > > window.onerror = function() > { > this.location = "/error-page"; > }; > </script> > > This is going to bomb if you call it the way you intend to. So call it is. BTW if the function object has no `call` method, how does it get called? https://codereview.adblockplus.org/29886700/diff/29924564/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29924564/lib/content/snippet... lib/content/snippets.js:496: * Generates a random alphanumeric ID consisting of 6 chars base-36 digits On 2018/10/25 16:12:16, Manish Jethani wrote: > s/chars// Done.
https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/25 20:26:09, hub wrote: > On 2018/10/25 16:12:15, Manish Jethani wrote: > > On 2018/10/25 14:32:25, hub wrote: > > > On 2018/10/24 23:02:09, Manish Jethani wrote: > > > > On 2018/10/24 22:17:38, hub wrote: > > > > > On 2018/10/24 20:47:45, Manish Jethani wrote: > > > > > > I just realized that you can have a function that has no call method > (if > > > the > > > > > > prototype has been changed). We should change this to > > > > > > `(() => {}).call.call(onerror, this, message, ...rest)`, in my > opinion. > > > > > > > > > > Actually I think I went a bit overboard. I don't even need `this`. In > this > > > > case > > > > > `onerror` is a function that will just be called as is. So I'll change > > this > > > to > > > > > `return onerror(message, ...rest);`. > > > > > > > > You did not go overboard, because in strict mode the `this` for the > function > > > > points to the window object, and if you don't pass it then it will be > > > undefined, > > > > which could cause the function to fail (if it accesses `this`). We have to > > > > faithfully pass on `this` to the original handler. > > > > > > No. If the original handler is bound to something, that bind is retained. > > > > How can we assume that the original handler will be bound to anything? > > > > Take this simple case: > > > > <script> > > "use strict"; > > > > window.onerror = function() > > { > > this.location = "/error-page"; > > }; > > </script> > > > > This is going to bomb if you call it the way you intend to. > > > So call it is. BTW if the function object has no `call` method, how does it get > called? The call method is a feature of the API, it is not required for the JS runtime for calling the function. For example: function foo() { console.log("foo"); } foo.__proto__ = {} typeof foo // "function" foo.call // undefined foo() // prints "foo"
https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... lib/content/snippets.js:501: function randomId() Do you like this version of the function? I don't understand why we're using base-36 here. If we need a random number of up to N bits, let's say N being 64, then we can just do this: Math.floor(Math.random() * (2 ** 32)).toString(16).padStart(8, "0") + Math.floor(Math.random() * (2 ** 32)).toString(16).padStart(8, "0") This would be straightforward to document, as in "Generates a 64-bit psuedo-random number in hexadecimal format". There's no reason to even mention a range, the range is whatever can fit in 64 bits (16 hexadecimal digits). Anyway, if you don't like the idea then let's just revert to the previous version, because now the JSDoc and the implementation are both getting weird. I think you'll agree that it was better before. https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... lib/content/snippets.js:556: return onerror.call(this, message, ...rest); OK, this one I already explained why there's no guarantee that there will be a call method on the object.
https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... lib/content/snippets.js:501: function randomId() Just to clarify, I'm sorry about leading you down this path and I now realize that it was not such a great idea. Let's revert to the previous version of randomId() (and accept the tiny chance of a collision with a genuine error message).
updated patch https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29921567/lib/content/snippet... lib/content/snippets.js:548: return onerror.call(this, message, ...rest); On 2018/10/25 21:32:25, Manish Jethani wrote: > On 2018/10/25 20:26:09, hub wrote: > > On 2018/10/25 16:12:15, Manish Jethani wrote: > > > On 2018/10/25 14:32:25, hub wrote: > > > > On 2018/10/24 23:02:09, Manish Jethani wrote: > > > > > On 2018/10/24 22:17:38, hub wrote: > > > > > > On 2018/10/24 20:47:45, Manish Jethani wrote: > > > > > > > I just realized that you can have a function that has no call method > > (if > > > > the > > > > > > > prototype has been changed). We should change this to > > > > > > > `(() => {}).call.call(onerror, this, message, ...rest)`, in my > > opinion. > > > > > > > > > > > > Actually I think I went a bit overboard. I don't even need `this`. In > > this > > > > > case > > > > > > `onerror` is a function that will just be called as is. So I'll change > > > this > > > > to > > > > > > `return onerror(message, ...rest);`. > > > > > > > > > > You did not go overboard, because in strict mode the `this` for the > > function > > > > > points to the window object, and if you don't pass it then it will be > > > > undefined, > > > > > which could cause the function to fail (if it accesses `this`). We have > to > > > > > faithfully pass on `this` to the original handler. > > > > > > > > No. If the original handler is bound to something, that bind is retained. > > > > > > How can we assume that the original handler will be bound to anything? > > > > > > Take this simple case: > > > > > > <script> > > > "use strict"; > > > > > > window.onerror = function() > > > { > > > this.location = "/error-page"; > > > }; > > > </script> > > > > > > This is going to bomb if you call it the way you intend to. > > > > > > So call it is. BTW if the function object has no `call` method, how does it > get > > called? > > The call method is a feature of the API, it is not required for the JS runtime > for calling the function. > > For example: > > function foo() { console.log("foo"); } > foo.__proto__ = {} > typeof foo // "function" > foo.call // undefined > foo() // prints "foo" Done. https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... lib/content/snippets.js:501: function randomId() On 2018/10/25 22:59:57, Manish Jethani wrote: > Just to clarify, I'm sorry about leading you down this path and I now realize > that it was not such a great idea. Let's revert to the previous version of > randomId() (and accept the tiny chance of a collision with a genuine error > message). Done. https://codereview.adblockplus.org/29886700/diff/29926555/lib/content/snippet... lib/content/snippets.js:556: return onerror.call(this, message, ...rest); On 2018/10/25 22:22:52, Manish Jethani wrote: > OK, this one I already explained why there's no guarantee that there will be a > call method on the object. Done.
https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:496: * Generates a random alphanumeric ID consisting 6 base-36 digits Nit: consisting *of* https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:524: * Will patch a property on the window object to abort when read. Suggestion: Patches a property on the window object to abort execution when the property is read. (Even if you don't like the entire suggestion, I think it should be "patches" instead of "will patch" because this is the tense we use in the rest of the JSDoc.) https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:525: * It will intercept the onerror callback and block it if tagged. I don't think the onerror part is necessary, it really looks like an implementation detail. https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:526: * The idea originates from Since we are now using JSDoc, how about using a link? The idea originates from {@link [url] uBlock Origin}. https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:529: * @todo handle properties of properties. I searched adblockpluscore, adblockpluschrome, and adblockplusui, and couldn't find a single instance of @todo. I don't think we leave todo comments in the code (but you can file an issue if you like). https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:531: * @param {string} property the name of the property. Suggestion: @param {string} property The name of the property. (The description is a complete sentence, starting with a capital letter.)
updated patch https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:496: * Generates a random alphanumeric ID consisting 6 base-36 digits On 2018/10/28 21:34:06, Manish Jethani wrote: > Nit: consisting *of* Done. https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:524: * Will patch a property on the window object to abort when read. On 2018/10/28 21:34:05, Manish Jethani wrote: > Suggestion: Patches a property on the window object to abort execution when the > property is read. > > (Even if you don't like the entire suggestion, I think it should be "patches" > instead of "will patch" because this is the tense we use in the rest of the > JSDoc.) Done. https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:525: * It will intercept the onerror callback and block it if tagged. On 2018/10/28 21:34:05, Manish Jethani wrote: > I don't think the onerror part is necessary, it really looks like an > implementation detail. I'll rephrase as "no error will be printed in the console". https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:526: * The idea originates from On 2018/10/28 21:34:05, Manish Jethani wrote: > Since we are now using JSDoc, how about using a link? > > The idea originates from {@link [url] uBlock Origin}. Done. https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:529: * @todo handle properties of properties. On 2018/10/28 21:34:05, Manish Jethani wrote: > I searched adblockpluscore, adblockpluschrome, and adblockplusui, and couldn't > find a single instance of @todo. I don't think we leave todo comments in the > code (but you can file an issue if you like). I'll remove it. https://codereview.adblockplus.org/29886700/diff/29928555/lib/content/snippet... lib/content/snippets.js:531: * @param {string} property the name of the property. On 2018/10/28 21:34:05, Manish Jethani wrote: > Suggestion: > > @param {string} property The name of the property. > > (The description is a complete sentence, starting with a capital letter.) Done.
updated patch https://codereview.adblockplus.org/29886700/diff/29930573/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29930573/lib/content/snippet... lib/content/snippets.js:520: Object.defineProperty(object, property, currentDescriptor); Here the problem is that if `currentDescriptor.value` is set or `currentDescriptor.writable` is false then `defineProperty` will throw an error. I'll update the patch.
https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippet... lib/content/snippets.js:515: { OK, so here's the thing: We don't need to use the previous descriptor at all. The new descriptor can only overwrite the properties given in the descriptor, any other properties are not overwritten. For example: let obj = {}; Object.defineProperty(obj, "foo", {value: 1, writable: true, configurable: true}); Object.defineProperty(obj, "foo", {get: function(){return 2}}); Object.defineProperty(obj, "foo", {configurable: false}); console.log(Object.getOwnPropertyDescriptor(obj, "foo")); console.log(obj.foo); But also note that we can't redefine a property that isn't configurable, so we need to check that: if (currentDescriptor && !currentDescriptor.configurable) return false; Other than this, we can just remove everything else and go straight to the last line. Also I think this function should return true if it wrapped the property and false otherwise, so that if the property wasn't wrapped then the calling code can skip some other stuff (like adding a onerror handler).
Updated patch https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29931197/lib/content/snippet... lib/content/snippets.js:515: { On 2018/10/30 22:57:28, Manish Jethani wrote: > OK, so here's the thing: We don't need to use the previous descriptor at all. > The new descriptor can only overwrite the properties given in the descriptor, > any other properties are not overwritten. > > For example: > > let obj = {}; > > Object.defineProperty(obj, "foo", {value: 1, writable: true, configurable: > true}); > Object.defineProperty(obj, "foo", {get: function(){return 2}}); > Object.defineProperty(obj, "foo", {configurable: false}); > > console.log(Object.getOwnPropertyDescriptor(obj, "foo")); > > console.log(obj.foo); > > But also note that we can't redefine a property that isn't configurable, so we > need to check that: > > if (currentDescriptor && !currentDescriptor.configurable) > return false; > > Other than this, we can just remove everything else and go straight to the last > line. > > Also I think this function should return true if it wrapped the property and > false otherwise, so that if the property wasn't wrapped then the calling code > can skip some other stuff (like adding a onerror handler). Done.
https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:544: let {onerror} = window; Can we move this line outside the if block? In theory somebody could want to wrap "window.onerror" and then this would bomb.
Leave nits aside, LGTM https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:524: * No error will be printed in the console. Nit: Again, is the "No error will be printed" part really relevant to a filter list author or anyone working on this code? If you think so, that's fine, but in that case we could keep the tense consistent (i.e. "No error is printed to the console"). https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:524: * No error will be printed in the console. Nit: Even though it doesn't matter to the generated documentation (since whitespace is not relevant for HTML), I leave a blank line between paragraphs here. We could do the same in this case (i.e. a blank line before and after "No error ...") https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:525: * The idea originates from Nit: Period at the end of the sentence.
Updated patch https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:524: * No error will be printed in the console. On 2018/10/31 19:54:49, Manish Jethani wrote: > Nit: Again, is the "No error will be printed" part really relevant to a filter > list author or anyone working on this code? If you think so, that's fine, but in > that case we could keep the tense consistent (i.e. "No error is printed to the > console"). Done. https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:524: * No error will be printed in the console. On 2018/10/31 19:54:49, Manish Jethani wrote: > Nit: Even though it doesn't matter to the generated documentation (since > whitespace is not relevant for HTML), I leave a blank line between paragraphs > here. We could do the same in this case (i.e. a blank line before and after "No > error ...") Done. https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:525: * The idea originates from On 2018/10/31 19:54:49, Manish Jethani wrote: > Nit: Period at the end of the sentence. Done. https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:544: let {onerror} = window; On 2018/10/31 19:38:16, Manish Jethani wrote: > Can we move this line outside the if block? In theory somebody could want to > wrap "window.onerror" and then this would bomb. It can be moved. But it would still bomb if it was wrapped, on the line below.
LGTM https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... File lib/content/snippets.js (right): https://codereview.adblockplus.org/29886700/diff/29932555/lib/content/snippet... lib/content/snippets.js:544: let {onerror} = window; On 2018/10/31 20:44:16, hub wrote: > On 2018/10/31 19:38:16, Manish Jethani wrote: > > Can we move this line outside the if block? In theory somebody could want to > > wrap "window.onerror" and then this would bomb. > > It can be moved. > > But it would still bomb if it was wrapped, on the line below. Alright, fair enough. 
|
