Issue 29832561: Issue 6774 - $document doesn't whitelist everything inside iframes in Edge (Closed)
|
Created:
July 17, 2018, 12:36 p.m. by geo Modified:
July 20, 2018, 3:42 p.m. CC:
Felix Dahlke Visibility:
Public. |
DescriptionIssue 6774 - $document doesn't whitelist everything inside iframes in Edge
Patch Set 1 #
Total comments: 14
Patch Set 2 : #
Total comments: 8
Patch Set 3 : #
Total comments: 2
Patch Set 4 : #MessagesTotal messages: 14
https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:572: let valueHref = value.url.href.replace("#", ""); This will only strip the # character itself but not the part that follows. Just in case, does value.url include the hash part anyway? https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:574: let rawSenderHref = rawSender.url ? Under which circumstances is url undefined or null? https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:575: rawSender.url.replace("#", "") : This will only strip the # character itself but not the part that follows. Just in case, does rawSender.url include the hash part anyway?
https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:563: // In Edge we don't have frameId Would be nice to add an Edge version number, so in the future it's easier to see if we can remove the workaround. https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:578: if (valueHref == rawSenderHref) I wonder what will happen if two frames have the same URL?
https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:563: // In Edge we don't have frameId Also please spell out "Microsoft Edge". https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:578: if (valueHref == rawSenderHref) On 2018/07/17 14:46:07, kzar wrote: > I wonder what will happen if two frames have the same URL? Then we assume that it's the first of the two frames. It's the best we can do.
https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:578: if (valueHref == rawSenderHref) On 2018/07/17 14:50:34, Sebastian Noack wrote: > On 2018/07/17 14:46:07, kzar wrote: > > I wonder what will happen if two frames have the same URL? > > Then we assume that it's the first of the two frames. It's the best we can do. Fair enough, but maybe we should leave a comment here to explain the limitation?
https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:563: // In Edge we don't have frameId On 2018/07/17 14:46:07, kzar wrote: > Would be nice to add an Edge version number, so in the future it's easier to see > if we can remove the workaround. Done. https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:563: // In Edge we don't have frameId On 2018/07/17 14:50:34, Sebastian Noack wrote: > Also please spell out "Microsoft Edge". Done. https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:572: let valueHref = value.url.href.replace("#", ""); On 2018/07/17 14:20:42, Sebastian Noack wrote: > This will only strip the # character itself but not the part that follows. > > Just in case, does value.url include the hash part anyway? I'm sorry. I've fixed it. Yes, both value.url and rawSender.url have the hash part. https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:574: let rawSenderHref = rawSender.url ? On 2018/07/17 14:20:42, Sebastian Noack wrote: > Under which circumstances is url undefined or null? I've based that off the comment at https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:575: rawSender.url.replace("#", "") : On 2018/07/17 14:20:41, Sebastian Noack wrote: > This will only strip the # character itself but not the part that follows. > > Just in case, does rawSender.url include the hash part anyway? Done. https://codereview.adblockplus.org/29832561/diff/29832562/ext/background.js#n... ext/background.js:578: if (valueHref == rawSenderHref) On 2018/07/17 15:41:53, kzar wrote: > On 2018/07/17 14:50:34, Sebastian Noack wrote: > > On 2018/07/17 14:46:07, kzar wrote: > > > I wonder what will happen if two frames have the same URL? > > > > Then we assume that it's the first of the two frames. It's the best we can do. > > Fair enough, but maybe we should leave a comment here to explain the limitation? Done.
https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:570: for (let [key, value] of frames) Nit: Would be nice to use some more descriptive variable names, e.g. something like `frameId` and `frame`. Oh, `frame` is already taken actually, but you get the idea. https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:572: let valueHref = value.url.href.replace(/#.*/, ""); Nit: I'd consider using `String.prototype.substring` to take the part of `value.url.href` up to the first "#", instead of using `String.prototype.replace` with a regular expression to mutate `value.url.href`. I'm not sure that it matters in practice though. https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:574: let rawSenderHref = rawSender.url ? Can't we do this outside of the for loop? https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:578: // if we have two frames with the same URL Nit: This is a good comment, but would you mind capitalising and adding a full stop?
https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:570: for (let [key, value] of frames) On 2018/07/17 16:54:35, kzar wrote: > Nit: Would be nice to use some more descriptive variable names, e.g. something > like `frameId` and `frame`. Oh, `frame` is already taken actually, but you get > the idea. Done. https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:572: let valueHref = value.url.href.replace(/#.*/, ""); On 2018/07/17 16:54:35, kzar wrote: > Nit: I'd consider using `String.prototype.substring` to take the part of > `value.url.href` up to the first "#", instead of using > `String.prototype.replace` with a regular expression to mutate `value.url.href`. > I'm not sure that it matters in practice though. I'm not sure either, but for `String.prototype.substring` or `String.prototype.substr` I would need to use `String.prototype.indexOf` and that returns `-1` if no hash is found, so I need an extra check. That's why I went with replace. https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:574: let rawSenderHref = rawSender.url ? On 2018/07/17 16:54:35, kzar wrote: > Can't we do this outside of the for loop? Done. https://codereview.adblockplus.org/29832561/diff/29832581/ext/background.js#n... ext/background.js:578: // if we have two frames with the same URL On 2018/07/17 16:54:35, kzar wrote: > Nit: This is a good comment, but would you mind capitalising and adding a full > stop? Done.
LGTM, nice work.
https://codereview.adblockplus.org/29832561/diff/29833595/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29833595/ext/background.js#n... ext/background.js:570: let rawSenderHref = rawSender.url ? Do we even want to enter this code path if we don't have a URL?
https://codereview.adblockplus.org/29832561/diff/29833595/ext/background.js File ext/background.js (right): https://codereview.adblockplus.org/29832561/diff/29833595/ext/background.js#n... ext/background.js:570: let rawSenderHref = rawSender.url ? On 2018/07/19 16:18:05, Sebastian Noack wrote: > Do we even want to enter this code path if we don't have a URL? No, you are right, there is no point to it. `frameInfo` will always have an url, so there will never be a match. Done.
LGTM
Even more LGTM
Pushed: https://hg.adblockplus.org/adblockpluschrome/rev/e062685e6bd6 Feel free to close this review. 
|
