Issue 30002601: Issue 7284 - Move CSS escaping to createStyleSheet (Closed)
|
Created:
Feb. 9, 2019, 12:41 a.m. by hub Modified:
Feb. 19, 2019, 1:03 p.m. Reviewers:
Manish Jethani Base URL:
https://hg.adblockplus.org/adblockpluscore/ Visibility:
Public. |
DescriptionIssue 7284 - Move CSS escaping to createStyleSheet
Patch Set 1 #
Total comments: 1
Patch Set 2 : Reworked the escaping of selectors #
Total comments: 17
Patch Set 3 : Updated addressing most comments #
Total comments: 1
Patch Set 4 : Added test #
Total comments: 8
Patch Set 5 : Nits addressed #Patch Set 6 : more nits #
MessagesTotal messages: 21
and our current testsuite for elemHidingEmulation doesn't trigger the bug because we don't go through the code path that escapes the curly braces. I'd love to get this in for the release if that's possible.
The issue I have with this fix is that it would no longer be possible to search for the literal string "\7B" in a node. For a solution, see my comment here: https://issues.adblockplus.org/ticket/7268#comment:3
On 2019/02/11 00:56:30, Manish Jethani wrote: > The issue I have with this fix is that it would no longer be possible to search > for the literal string "\7B" in a node. > > For a solution, see my comment here: > https://issues.adblockplus.org/ticket/7268#comment:3 I am pretty sure `:-abp-properties()` has the exact same issue.
On 2019/02/11 17:19:09, hub wrote: > On 2019/02/11 00:56:30, Manish Jethani wrote: > > The issue I have with this fix is that it would no longer be possible to > search > > for the literal string "\7B" in a node. > > > > For a solution, see my comment here: > > https://issues.adblockplus.org/ticket/7268#comment:3 > > I am pretty sure `:-abp-properties()` has the exact same issue. Just to be clear, the patch here was meant to quickly fix an issue that break the feature seriously, making it on par with the rest - in hope to get it in a release soon. `:-abp-properties()` also will suffer from the same limitation described above.
Will update the patch.
updated patch
https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:485: function escapedSelector(selector) while we expect a string, if `selector` is undefined it will throw an error. The bug in test I fixed was triggering it. So I'm pondering if we shouldn't check for undefined (and null?). https://codereview.adblockplus.org/30002601/diff/30005555/test/browser/elemHi... File test/browser/elemHideEmulation.js (left): https://codereview.adblockplus.org/30002601/diff/30005555/test/browser/elemHi... test/browser/elemHideEmulation.js:305: exports.testPropertySelectorWithEscapedBrace = async function(test) these two tests are no longer relevant. https://codereview.adblockplus.org/30002601/diff/30005555/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30005555/test/elemHide.js#ne... test/elemHide.js:302: selectors[index] = ".s" + index; The previous version of the code produced an Array that was empty as it seems the iterator returned nothing.
https://codereview.adblockplus.org/30002601/diff/30005555/lib/content/elemHid... File lib/content/elemHideEmulation.js (left): https://codereview.adblockplus.org/30002601/diff/30005555/lib/content/elemHid... lib/content/elemHideEmulation.js:351: Unrelated? https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:480: * Escape curly braces to prevent CSS rule injection. s/Escape/Escapes/ https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:483: * @return {string} s/@return/@returns/ https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:485: function escapedSelector(selector) On 2019/02/12 23:55:49, hub wrote: > while we expect a string, if `selector` is undefined it will throw an error. The > bug in test I fixed was triggering it. > So I'm pondering if we shouldn't check for undefined (and null?). If it's always going to be a string in practice then we should not check for null. The tests should not try to pass a non-string. https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:485: function escapedSelector(selector) Let's rename to `escapeSelector` https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:504: " {display: none !important;}\n"; Nit: The double quote here should be in the same column as the 'e' of 'escapedSelector' above. https://codereview.adblockplus.org/30002601/diff/30005555/test/browser/elemHi... File test/browser/elemHideEmulation.js (left): https://codereview.adblockplus.org/30002601/diff/30005555/test/browser/elemHi... test/browser/elemHideEmulation.js:305: exports.testPropertySelectorWithEscapedBrace = async function(test) On 2019/02/12 23:55:49, hub wrote: > these two tests are no longer relevant. Shouldn't we add some tests now to test/elemHide.js to make sure the braces are being escaped in the generated style sheet? https://codereview.adblockplus.org/30002601/diff/30005555/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30005555/test/elemHide.js#ne... test/elemHide.js:302: selectors[index] = ".s" + index; On 2019/02/12 23:55:49, hub wrote: > The previous version of the code produced an Array that was empty as it seems > the iterator returned nothing. I see, `Array.prototype.map` skips empty items. This could also be fixed by using fill() new Array(50000).fill().map((element, index) => ".s" + index)
On 2019/02/12 18:14:17, hub wrote: > Just to be clear, the patch here was meant to quickly fix an issue that break > the feature seriously, making it on par with the rest - in hope to get it in a > release soon. OK, I see that this patch is getting bigger than I had thought. Let's land your original fix now and we can keep this one in review (because we have to check the performance of createStyleSheet() now). Can you submit a new patch with your original fix?
https://codereview.adblockplus.org/30002601/diff/30002602/lib/content/elemHid... File lib/content/elemHideEmulation.js (right): https://codereview.adblockplus.org/30002601/diff/30002602/lib/content/elemHid... lib/content/elemHideEmulation.js:351: this._regexp = makeRegExpParameter( Nit: I think it helps to put the "depends on" variables together, that's why I had the blank line.
https://codereview.adblockplus.org/30002601/diff/30005555/lib/content/elemHid... File lib/content/elemHideEmulation.js (left): https://codereview.adblockplus.org/30002601/diff/30005555/lib/content/elemHid... lib/content/elemHideEmulation.js:351: On 2019/02/13 12:27:25, Manish Jethani wrote: > Unrelated? This shouldn't have been here, indeed. Done. https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:480: * Escape curly braces to prevent CSS rule injection. On 2019/02/13 12:27:25, Manish Jethani wrote: > s/Escape/Escapes/ Done. https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:483: * @return {string} On 2019/02/13 12:27:25, Manish Jethani wrote: > s/@return/@returns/ Done. https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:485: function escapedSelector(selector) On 2019/02/13 12:27:25, Manish Jethani wrote: > Let's rename to `escapeSelector` Done. https://codereview.adblockplus.org/30002601/diff/30005555/lib/elemHide.js#new... lib/elemHide.js:504: " {display: none !important;}\n"; On 2019/02/13 12:27:25, Manish Jethani wrote: > Nit: The double quote here should be in the same column as the 'e' of > 'escapedSelector' above. Done. https://codereview.adblockplus.org/30002601/diff/30005555/test/browser/elemHi... File test/browser/elemHideEmulation.js (left): https://codereview.adblockplus.org/30002601/diff/30005555/test/browser/elemHi... test/browser/elemHideEmulation.js:305: exports.testPropertySelectorWithEscapedBrace = async function(test) On 2019/02/13 12:27:25, Manish Jethani wrote: > On 2019/02/12 23:55:49, hub wrote: > > these two tests are no longer relevant. > > Shouldn't we add some tests now to test/elemHide.js to make sure the braces are > being escaped in the generated style sheet? Yes
On 2019/02/13 14:58:24, Manish Jethani wrote: > On 2019/02/12 18:14:17, hub wrote: > > Just to be clear, the patch here was meant to quickly fix an issue that break > > the feature seriously, making it on par with the rest - in hope to get it in a > > release soon. > > OK, I see that this patch is getting bigger than I had thought. Let's land your > original fix now and we can keep this one in review (because we have to check > the performance of createStyleSheet() now). > > Can you submit a new patch with your original fix? https://codereview.adblockplus.org/30006562
https://codereview.adblockplus.org/30002601/diff/30006555/lib/elemHide.js File lib/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30006555/lib/elemHide.js#new... lib/elemHide.js:501: rule += escapeSelector(selectors[i]) + ", "; Performance-wise this additional code seems to be OK.
This is looking good except we could add some tests to test/elemHide.js to make sure that createStyleSheet escapes the CSS selectors containing braces.
On 2019/02/15 14:21:49, Manish Jethani wrote: > This is looking good except we could add some tests to test/elemHide.js to make > sure that createStyleSheet escapes the CSS selectors containing braces. Added the tests now.
https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:312: "html, #foo, .bar, #foo .bar, #foo > .bar, #foo[data-bar='\\7B foo: 1\\7D '] " + Nit: This is the only line in the file that exceeds 80 characters. Can we split this into ""html, #foo, .bar, #foo .bar, #foo > .bar," and the rest? https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:314: "Style sheet creation should work" "Braces should be escaped" https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:316: test.done(); Nit: Can we leave a blank line above test.done?
updated patch https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:312: "html, #foo, .bar, #foo .bar, #foo > .bar, #foo[data-bar='\\7B foo: 1\\7D '] " + On 2019/02/16 04:34:04, Manish Jethani wrote: > Nit: This is the only line in the file that exceeds 80 characters. Can we split > this into ""html, #foo, .bar, #foo .bar, #foo > .bar," and the rest? Done. https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:314: "Style sheet creation should work" On 2019/02/16 04:34:04, Manish Jethani wrote: > "Braces should be escaped" Done. https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:316: test.done(); On 2019/02/16 04:34:04, Manish Jethani wrote: > Nit: Can we leave a blank line above test.done? Done.
The summary of this patch should be "Issue 7284 - Move CSS escaping to createStyleSheet". https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:312: "html, #foo, .bar, #foo .bar, #foo > .bar, #foo[data-bar='\\7B foo: 1\\7D '] " + On 2019/02/18 12:56:20, hub wrote: > On 2019/02/16 04:34:04, Manish Jethani wrote: > > Nit: This is the only line in the file that exceeds 80 characters. Can we > split > > this into ""html, #foo, .bar, #foo .bar, #foo > .bar," and the rest? > > Done. Nit: When wrapping an expression `a + b` the break should be after `+` and `b` should be in the same column as `a`.
updated patch https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js File test/elemHide.js (right): https://codereview.adblockplus.org/30002601/diff/30008555/test/elemHide.js#ne... test/elemHide.js:312: "html, #foo, .bar, #foo .bar, #foo > .bar, #foo[data-bar='\\7B foo: 1\\7D '] " + On 2019/02/18 13:07:21, Manish Jethani wrote: > On 2019/02/18 12:56:20, hub wrote: > > On 2019/02/16 04:34:04, Manish Jethani wrote: > > > Nit: This is the only line in the file that exceeds 80 characters. Can we > > split > > > this into ""html, #foo, .bar, #foo .bar, #foo > .bar," and the rest? > > > > Done. > > Nit: When wrapping an expression `a + b` the break should be after `+` and `b` > should be in the same column as `a`. Done.
On 2019/02/18 13:07:21, Manish Jethani wrote: > The summary of this patch should be "Issue 7284 - Move CSS escaping to > createStyleSheet". Aside from the above, LGTM 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
