Preload page contents with CSP as soon as rules have been applied [chromium/src : main]
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 1 comment![]( "Open in Gerrit")
NOTE: Current Patchset pushed for CI run only.
Related details
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard Townsend (Gerrit)
Richard Townsend voted Commit-Queue+1![]( "Open in Gerrit")
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 1 comment![]( "Open in Gerrit")
Hey, all!
As mentioned in the commit message, this CL changes the HTML document parser to resolve preloads affected by CSP meta tag as soon as the CSP rules are applied.
Please feel free to ask for any clarification!
Kind regards,
Tarcisio.
Related details
- Charlie Harrison
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 1 comment![]( "Open in Gerrit")
(Just noticed Mason Freed is OOO/Slow, so changing reviewer)
Related details
- Charlie Harrison
- Ian Kilpatrick
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 6 comments![]( "Open in Gerrit")
Thanks for fixing this. The issue is near and dear to my heart from https://codereview.chromium.org/2242223003 😊
Can you please figure out why the added test in that CL (/multiple-meta-csp.html) did not catch this regression, and fix the test if possible?
// yet). This is useful to check if preloads are allowed in the presence ofhow? Maybe mention what this is actually used for.
return !pending_preloads_->IsEmpty();😮. Can we make this a separate CL in case this has its own perf impact?
bool HTMLDocumentParser::AllowPreloadingConsideringCSPMetaTags() {Nit: suggest making this method more generic like `AllowPreloading` and include the following checks:
1. `!queued_preloads_.empty()`
2. `preloader_`
Additionally, we can just call this from within `FetchQueuedPreloads` (you could rename it `MaybeFetchQueuedPreloads` if you want). I think this would simplify call sites a bunch.
bool HTMLResourcePreloader::AllowPreloadConsideringCSP(Do we actually need all this? My memory was that we checked CSP for all requests later, e.g. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_request_utils.cc;drc=451093bbaf7fe812bf67d27d760f3bb64c92830b;l=334
Related details
- Ian Kilpatrick
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 1 comment![]( "Open in Gerrit")
Adding Kouhei for loading expertise too (feel free to reroute though)
Related details
- Ian Kilpatrick
- Kouhei Ueno
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 1 comment![]( "Open in Gerrit")
Thanks for fixing this. The issue is near and dear to my heart from https://codereview.chromium.org/2242223003 😊
Can you please figure out why the added test in that CL (/multiple-meta-csp.html) did not catch this regression, and fix the test if possible?
Interesting! To be honest, I didn't know about your "relation" to the issue, as I've added the reviewers in the order they were suggested, skipping anyone that was OOO - So I guess I was very lucky :)
`multiple-meta-csp.html` has been changed since you've added it, so there's no internals.isPreloaded assertions (assert_true) anymore - only the "negative" ones (assert_false). Culprit is here[1], but I didn't go into understanding the CL entirely yet.
Regardless of the reason, simply re-adding the assertions won't be enough, as I've just checked. This is because my CL isn't really "preloading" things, but just "early-loading", if that makes sense...? Let me try to explain:
Once a CSP meta tag has been seen, all preloads are deferred until CSP meta tag is processed.
But the CSP meta tags will only be processed later (e.g. when PumpTokenizer is called). So it's not really a "preloading", since it'll only happen after several calls for ConstructTreeFromToken. You can see the relevant change here: https://chromium-review.googlesource.com/c/chromium/src/+/5832754/5/third_party/blink/renderer/core/html/parser/html_document_parser.cc#782
So answering to your question: I don't know how to fix the test with the tools currently available :( I also tried adding a new test to it, but I couldn't do it unless I add a very specific observer variable to keep track of "preloads deferred due to CSP", which doesn't sound great (too specific). Do you have any suggestions?
[1] https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf2441e7f0686d53%5E%21/#F1
Related details
- Charlie Harrison
- Ian Kilpatrick
- Kouhei Ueno
- Richard Townsend
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 1 comment![]( "Open in Gerrit")
Tarcísio FischerThanks for fixing this. The issue is near and dear to my heart from https://codereview.chromium.org/2242223003 😊
Can you please figure out why the added test in that CL (/multiple-meta-csp.html) did not catch this regression, and fix the test if possible?
Interesting! To be honest, I didn't know about your "relation" to the issue, as I've added the reviewers in the order they were suggested, skipping anyone that was OOO - So I guess I was very lucky :)
`multiple-meta-csp.html` has been changed since you've added it, so there's no internals.isPreloaded assertions (assert_true) anymore - only the "negative" ones (assert_false). Culprit is here[1], but I didn't go into understanding the CL entirely yet.
Regardless of the reason, simply re-adding the assertions won't be enough, as I've just checked. This is because my CL isn't really "preloading" things, but just "early-loading", if that makes sense...? Let me try to explain:
Once a CSP meta tag has been seen, all preloads are deferred until CSP meta tag is processed.
But the CSP meta tags will only be processed later (e.g. when PumpTokenizer is called). So it's not really a "preloading", since it'll only happen after several calls for ConstructTreeFromToken. You can see the relevant change here: https://chromium-review.googlesource.com/c/chromium/src/+/5832754/5/third_party/blink/renderer/core/html/parser/html_document_parser.cc#782So answering to your question: I don't know how to fix the test with the tools currently available :( I also tried adding a new test to it, but I couldn't do it unless I add a very specific observer variable to keep track of "preloads deferred due to CSP", which doesn't sound great (too specific). Do you have any suggestions?
[1] https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf2441e7f0686d53%5E%21/#F1
Hm... would the following test work:
```
<meta tag>
<script>
// Poll the server using a looped sync XHR to see if to_preload has been fetched.
...
</script>
<img src="to_preload.png">
```
It's been a while for me in this code base so I forget the precise parsing semantics, maybe Kouhei has another suggestion though.
Related details
- Ian Kilpatrick
- Kouhei Ueno
- Richard Townsend
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Ian Kilpatrick (Gerrit)
Ian Kilpatrick added 1 comment![]( "Open in Gerrit")
Sorry I'm not the right reviewer - Mason is likely best. Added jarhar@ as a backup.
Related details
- Joey Arhar
- Kouhei Ueno
- Mason Freed
- Richard Townsend
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 6 comments![]( "Open in Gerrit")
if (seen_csp_meta_tags_ > 0 && !queued_preloads_.empty() &&nit: you don't need `>0`
(side-nit: I actually like making this `unsigned` as you have it, so that there's no question about what `<0` means here?)
return !pending_preloads_->IsEmpty();😮. Can we make this a separate CL in case this has its own perf impact?
????? How long has that been there? I'm sure I'm the reviewer, whenever it was.
bool HTMLDocumentParser::AllowPreloadingConsideringCSPMetaTags() {Nit: suggest making this method more generic like `AllowPreloading` and include the following checks:
1. `!queued_preloads_.empty()`
2. `preloader_`Additionally, we can just call this from within `FetchQueuedPreloads` (you could rename it `MaybeFetchQueuedPreloads` if you want). I think this would simplify call sites a bunch.
This would also be a good place to add a flag guarding this new behavior.
int* csp_meta_tag_counter) {two nits:
- this should be `unsigned` also, or at least should match the other two
- Perhaps `csp_meta_tag_count` rather than `counter`?
*csp_meta_tag_counter += 1;nit: `++(*csp_meta_tag_counter)`
const IntegrityMetadataSet& IntegrityMetadataForTestingOnly() const {Without further context, it always worries me to remove `ForTestingOnly` for something. Do you know why this was for testing only previously, and why it's safe now?
Related details
- Charlie Harrison
- Joey Arhar
- Kouhei Ueno
- Richard Townsend
- Tarcísio Fischer
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 2 comments![]( "Open in Gerrit")
return !pending_preloads_->IsEmpty();Mason Freed😮. Can we make this a separate CL in case this has its own perf impact?
????? How long has that been there? I'm sure I'm the reviewer, whenever it was.
Originally here: https://chromium-review.googlesource.com/c/chromium/src/+/5125772/5/third_party/blink/renderer/core/html/parser/html_document_parser.cc#1596
This CL was reverted but then re-landed here: https://chromium-review.googlesource.com/c/chromium/src/+/5132495
I'll move this fix to a separate CL.
const IntegrityMetadataSet& IntegrityMetadataForTestingOnly() const {Without further context, it always worries me to remove `ForTestingOnly` for something. Do you know why this was for testing only previously, and why it's safe now?
I didn't check the history behind it, I thought "ForTestingOnly" just meant it was only being used on test code up to now.
Regardless of the reason, I'll try to revert this change with Charlie's hypothesis that `HTMLResourcePreloader::AllowPreloadConsideringCSP` is doing duplicated work (Because that's where I was using this).
Related details
- Charlie Harrison
- Joey Arhar
- Kouhei Ueno
- Mason Freed
- Richard Townsend
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 8 comments![]( "Open in Gerrit")
Hey, all.
Thank you for all the review.
It looks like Charlie's hypothesis that HTMLResourcePreloader::AllowPreloadConsideringCSP was doing duplicated work was correct, since all tests passed and also I didn't have any issues, in local manual experiments. - So I managed to clean up the CL a bit (Thank you).
There are some remaining questions that I'd be happy to hear your opinions.
Done
// yet). This is useful to check if preloads are allowed in the presence ofhow? Maybe mention what this is actually used for.
Done
if (seen_csp_meta_tags_ > 0 && !queued_preloads_.empty() &&nit: you don't need `>0`
(side-nit: I actually like making this `unsigned` as you have it, so that there's no question about what `<0` means here?)
Since this is just a nit, I'll prefer following the Chromium Guidelines and just use `int` whenever possible, if that's ok?
TRACE_EVENT_WITH_FLOW0("blink,devtools.timeline",I wonder if I should move this up (before the introduced `if`), since the `if` is trying to decide if the function will actually run. Asking here to see if anyone has any opinions on that.
bool HTMLDocumentParser::AllowPreloadingConsideringCSPMetaTags() {Mason FreedNit: suggest making this method more generic like `AllowPreloading` and include the following checks:
1. `!queued_preloads_.empty()`
2. `preloader_`Additionally, we can just call this from within `FetchQueuedPreloads` (you could rename it `MaybeFetchQueuedPreloads` if you want). I think this would simplify call sites a bunch.
This would also be a good place to add a flag guarding this new behavior.
Now that `FetchQueuedPreloads` has been renamed to `MaybeFetchQueuedPreloads`, I wonder if there's any issue in renaming the `UmaHistogram`. I did rename it, but please let me know if that's the best approach in this case.
int* csp_meta_tag_counter) {two nits:
- this should be `unsigned` also, or at least should match the other two
- Perhaps `csp_meta_tag_count` rather than `counter`?
I've renamed it to `csp_meta_tag_count`, but will keep it as `int` just to follow Chromium's guidelines, if that's ok?
*csp_meta_tag_counter += 1;Tarcísio Fischernit: `++(*csp_meta_tag_counter)`
Done
Related details
- Charlie Harrison
- Joey Arhar
- Kouhei Ueno
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 4 comments![]( "Open in Gerrit")
if (seen_csp_meta_tags_ > 0 && !queued_preloads_.empty() &&Tarcísio Fischernit: you don't need `>0`
(side-nit: I actually like making this `unsigned` as you have it, so that there's no question about what `<0` means here?)
Since this is just a nit, I'll prefer following the Chromium Guidelines and just use `int` whenever possible, if that's ok?
Wow I actually went and read the guidelines for that, which I hadn't noticed before. For reference, it says this:
In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.
I feel like we violate this all over the place. But anyway, I'm ok with an `int` in these cases, but I do want the second part: please add `CHECK(int>=0)` everywhere.
TRACE_EVENT_WITH_FLOW0("blink,devtools.timeline",I wonder if I should move this up (before the introduced `if`), since the `if` is trying to decide if the function will actually run. Asking here to see if anyone has any opinions on that.
Since the function is "Maybe", I think I'd log it before the `if()` so you see that the "maybe" got evaluated. Just my opinion.
Mason Freed😮. Can we make this a separate CL in case this has its own perf impact?
Tarcísio Fischer????? How long has that been there? I'm sure I'm the reviewer, whenever it was.
Originally here: https://chromium-review.googlesource.com/c/chromium/src/+/5125772/5/third_party/blink/renderer/core/html/parser/html_document_parser.cc#1596
This CL was reverted but then re-landed here: https://chromium-review.googlesource.com/c/chromium/src/+/5132495
I'll move this fix to a separate CL.
Yep, I reviewed it. Thanks for the archaeology.
int* csp_meta_tag_counter) {Tarcísio Fischertwo nits:
- this should be `unsigned` also, or at least should match the other two
- Perhaps `csp_meta_tag_count` rather than `counter`?
I've renamed it to `csp_meta_tag_count`, but will keep it as `int` just to follow Chromium's guidelines, if that's ok?
- Charlie Harrison
- Joey Arhar
- Kouhei Ueno
- Tarcísio Fischer
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 2 comments![]( "Open in Gerrit")
Tarcísio FischerThanks for fixing this. The issue is near and dear to my heart from https://codereview.chromium.org/2242223003 😊
Can you please figure out why the added test in that CL (/multiple-meta-csp.html) did not catch this regression, and fix the test if possible?
Charlie HarrisonInteresting! To be honest, I didn't know about your "relation" to the issue, as I've added the reviewers in the order they were suggested, skipping anyone that was OOO - So I guess I was very lucky :)
`multiple-meta-csp.html` has been changed since you've added it, so there's no internals.isPreloaded assertions (assert_true) anymore - only the "negative" ones (assert_false). Culprit is here[1], but I didn't go into understanding the CL entirely yet.
Regardless of the reason, simply re-adding the assertions won't be enough, as I've just checked. This is because my CL isn't really "preloading" things, but just "early-loading", if that makes sense...? Let me try to explain:
Once a CSP meta tag has been seen, all preloads are deferred until CSP meta tag is processed.
But the CSP meta tags will only be processed later (e.g. when PumpTokenizer is called). So it's not really a "preloading", since it'll only happen after several calls for ConstructTreeFromToken. You can see the relevant change here: https://chromium-review.googlesource.com/c/chromium/src/+/5832754/5/third_party/blink/renderer/core/html/parser/html_document_parser.cc#782So answering to your question: I don't know how to fix the test with the tools currently available :( I also tried adding a new test to it, but I couldn't do it unless I add a very specific observer variable to keep track of "preloads deferred due to CSP", which doesn't sound great (too specific). Do you have any suggestions?
[1] https://chromium.googlesource.com/chromium/src/+/bb924e1897f5b4c67325dbfbdf2441e7f0686d53%5E%21/#F1
Hm... would the following test work:
```
<meta tag>
<script>
// Poll the server using a looped sync XHR to see if to_preload has been fetched.
...
</script>
<img src="to_preload.png">
```It's been a while for me in this code base so I forget the precise parsing semantics, maybe Kouhei has another suggestion though.
The issue is (unless I am missing something), when things get loaded, the script can't be sure if they were "late-preloaded" or just "regular-loaded".
There is a `internals.isPreloaded`, but it won't be recognized as preloaded in this case (because my late-preloading is being done later in the `PumpTokenizer`, only after I make sure CSP rules are applied).
The only way I can think of would be to have a bunch of CSP-dependent resources in a HTML file and then measure how long it takes to load the whole page. For 20 resources, if it's less than ~5 seconds, then things got late-preloaded. If more than ~15 seconds, then things didn't got late-preloaded. If it's anything in between 5-20seconds, it's hard to say. (That's how I manually tested, and that's what's in the bug report). But that's a very bad idea for an automated testing, since it'll probably be flaky.
bool HTMLResourcePreloader::AllowPreloadConsideringCSP(Do we actually need all this? My memory was that we checked CSP for all requests later, e.g. in https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/loader/fetch/resource_request_utils.cc;drc=451093bbaf7fe812bf67d27d760f3bb64c92830b;l=334
We don't need it. Thanks for letting me know!
Related details
- Charlie Harrison
- Joey Arhar
- Kouhei Ueno
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 1 comment![]( "Open in Gerrit")
const IntegrityMetadataSet& IntegrityMetadataForTestingOnly() const {Tarcísio FischerWithout further context, it always worries me to remove `ForTestingOnly` for something. Do you know why this was for testing only previously, and why it's safe now?
I didn't check the history behind it, I thought "ForTestingOnly" just meant it was only being used on test code up to now.
Regardless of the reason, I'll try to revert this change with Charlie's hypothesis that `HTMLResourcePreloader::AllowPreloadConsideringCSP` is doing duplicated work (Because that's where I was using this).
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 2 comments![]( "Open in Gerrit")
Hm yeah I think I understand the point, but I would have thought my proposed test would catch that, since the script blocks "regular" loading of the <img> tag, so if the tag ever gets loaded, it must have been preloaded / early-loaded.
if (seen_csp_meta_tags_ > 0 && !queued_preloads_.empty() &&Tarcísio Fischernit: you don't need `>0`
(side-nit: I actually like making this `unsigned` as you have it, so that there's no question about what `<0` means here?)
Mason FreedSince this is just a nit, I'll prefer following the Chromium Guidelines and just use `int` whenever possible, if that's ok?
Wow I actually went and read the guidelines for that, which I hadn't noticed before. For reference, it says this:
In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.
I feel like we violate this all over the place. But anyway, I'm ok with an `int` in these cases, but I do want the second part: please add `CHECK(int>=0)` everywhere.
(FWIW in the past blink and webkit did not follow the google style guide, so it makes sense there is tons of divergence in the code base. I am not sure if we have specific guidance when there is divergence within one file)
Related details
- Mason Freed
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 1 comment![]( "Open in Gerrit")
if (seen_csp_meta_tags_ > 0 && !queued_preloads_.empty() &&Tarcísio Fischernit: you don't need `>0`
(side-nit: I actually like making this `unsigned` as you have it, so that there's no question about what `<0` means here?)
Mason FreedSince this is just a nit, I'll prefer following the Chromium Guidelines and just use `int` whenever possible, if that's ok?
Charlie HarrisonWow I actually went and read the guidelines for that, which I hadn't noticed before. For reference, it says this:
In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.
I feel like we violate this all over the place. But anyway, I'm ok with an `int` in these cases, but I do want the second part: please add `CHECK(int>=0)` everywhere.
(FWIW in the past blink and webkit did not follow the google style guide, so it makes sense there is tons of divergence in the code base. I am not sure if we have specific guidance when there is divergence within one file)
Yeah, makes sense.
If we're sticking with `int`, that's fine. But I do want `CHECK(int>=0)` added everywhere we use it. Accidentally forgetting negatives is a bug waiting to happen.
Related details
- Charlie Harrison
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 4 comments![]( "Open in Gerrit")
Ohhh- ok, I see your point. I'll try with that specific scenario, sorry I didn't really understood before.
if (seen_csp_meta_tags_ > 0 && !queued_preloads_.empty() &&Tarcísio Fischernit: you don't need `>0`
(side-nit: I actually like making this `unsigned` as you have it, so that there's no question about what `<0` means here?)
Mason FreedSince this is just a nit, I'll prefer following the Chromium Guidelines and just use `int` whenever possible, if that's ok?
Charlie HarrisonWow I actually went and read the guidelines for that, which I hadn't noticed before. For reference, it says this:
In particular, do not use unsigned types to say a number will never be negative. Instead, use assertions for this.
I feel like we violate this all over the place. But anyway, I'm ok with an `int` in these cases, but I do want the second part: please add `CHECK(int>=0)` everywhere.
Mason Freed(FWIW in the past blink and webkit did not follow the google style guide, so it makes sense there is tons of divergence in the code base. I am not sure if we have specific guidance when there is divergence within one file)
Yeah, makes sense.
If we're sticking with `int`, that's fine. But I do want `CHECK(int>=0)` added everywhere we use it. Accidentally forgetting negatives is a bug waiting to happen.
Done
Mason FreedI wonder if I should move this up (before the introduced `if`), since the `if` is trying to decide if the function will actually run. Asking here to see if anyone has any opinions on that.
Since the function is "Maybe", I think I'd log it before the `if()` so you see that the "maybe" got evaluated. Just my opinion.
Done
bool HTMLDocumentParser::AllowPreloadingConsideringCSPMetaTags() {Mason FreedNit: suggest making this method more generic like `AllowPreloading` and include the following checks:
1. `!queued_preloads_.empty()`
2. `preloader_`Additionally, we can just call this from within `FetchQueuedPreloads` (you could rename it `MaybeFetchQueuedPreloads` if you want). I think this would simplify call sites a bunch.
Tarcísio FischerThis would also be a good place to add a flag guarding this new behavior.
Now that `FetchQueuedPreloads` has been renamed to `MaybeFetchQueuedPreloads`, I wonder if there's any issue in renaming the `UmaHistogram`. I did rename it, but please let me know if that's the best approach in this case.
- Charlie Harrison
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 4 comments![]( "Open in Gerrit")
A few small nits, but this LGTM at this point.
if (base::FeatureList::IsEnabled(features::kAllowPreloadingWithCSPMetaTag)) {Any reason not to use a normal runtime enabled feature here?
That's the normal way to gate features within blink.
if (csp_meta_tag_count != 0) {nit: just `if (csp_meta_tag_count)`
Related details
- Charlie Harrison
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 2 comments![]( "Open in Gerrit")
Change mostly LGTM other than the test if it's possible.
// Early return in case preloads are not allowed.nit: comment is redundant with the code.
Related details
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 5 comments![]( "Open in Gerrit")
Apparently I was wrong before, and `internals.isPreloaded` works just fine. Sorry for my big (flawed) explanation. I don't know what exactly I did different before that wasn't working, to be honest. I am thinking that _maybe_ the implementation changed a bit since I tried. Regardless of the reason, apparently it is now working.
I've added an `early_load_with_csp.html` web test that ensures that an image is preloaded in the presence of a meta CSP tag.
nit: comment is redundant with the code.
Done
if (base::FeatureList::IsEnabled(features::kAllowPreloadingWithCSPMetaTag)) {Any reason not to use a normal runtime enabled feature here?
That's the normal way to gate features within blink.
Only because I didn't know about this. I changed to use the RuntimeEnabledFeatures. Thanks for suggesting.
if (csp_meta_tag_count != 0) {Tarcísio Fischernit: just `if (csp_meta_tag_count)`
Done
Related details
- Charlie Harrison
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison added 1 comment![]( "Open in Gerrit")
/* Sync wait acts as a barrier to give the time for the image to be preloaded and avoid loading it */Now that I'm thinking about it... I wonder if this is either flaky or not needed.
IIUC we need the main thread free to initiate a preload request. This means if when the JS has started running we _haven't_ yet sent off the preload request, we will not while the JS is running because the JS is taking control of the main thread.
Does the `isPreloaded` function work if we just check after the page is finished loading?
Related details
- Mason Freed
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 1 comment![]( "Open in Gerrit")
/* Sync wait acts as a barrier to give the time for the image to be preloaded and avoid loading it */Now that I'm thinking about it... I wonder if this is either flaky or not needed.
IIUC we need the main thread free to initiate a preload request. This means if when the JS has started running we _haven't_ yet sent off the preload request, we will not while the JS is running because the JS is taking control of the main thread.Does the `isPreloaded` function work if we just check after the page is finished loading?
Looks like you are right (I just tested it) - syncWait is not necessary. I started from the looped sync XHR idea and thought I'd just need any synchronous barrier, so I never tried removing it completely. `isPreloaded` works if we check after the page is finished loading, so the test passes.
I'll remove that bit from the code. By the way, is this a good location for this test file, or should I move it to `blink/web_tests/http/tests/preload/`? (Or somewhere else?)
Thanks for all the help so far!
Related details
- Charlie Harrison
- Mason Freed
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed added 2 comments![]( "Open in Gerrit")
I think this LGTM, with the other test-related questions resolved. Just one more thing about the unit test.
[this](bool enable_allow_preloading_with_csp_meta_tag) {I think `INSTANTIATE_TEST_SUITE_P` might be a better (and more standard) approach here. You can have a Param that is true/false and use that to set the scoped feature flag. Here's a rough example:
Related details
- Charlie Harrison
- Tarcísio Fischer
Code-Coverage
Code-Review
No-Unresolved-Comments
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tarcísio Fischer (Gerrit)
Tarcísio Fischer added 3 comments![]( "Open in Gerrit")
Hey all.
I believe this is good to go, but please feel free to further review, in case there's anything else you'd like me to fix/improve.
Thank you for the review and discussions!
[this](bool enable_allow_preloading_with_csp_meta_tag) {I think `INSTANTIATE_TEST_SUITE_P` might be a better (and more standard) approach here. You can have a Param that is true/false and use that to set the scoped feature flag. Here's a rough example:
Done
/* Sync wait acts as a barrier to give the time for the image to be preloaded and avoid loading it */Tarcísio FischerNow that I'm thinking about it... I wonder if this is either flaky or not needed.
IIUC we need the main thread free to initiate a preload request. This means if when the JS has started running we _haven't_ yet sent off the preload request, we will not while the JS is running because the JS is taking control of the main thread.Does the `isPreloaded` function work if we just check after the page is finished loading?
Looks like you are right (I just tested it) - syncWait is not necessary. I started from the looped sync XHR idea and thought I'd just need any synchronous barrier, so I never tried removing it completely. `isPreloaded` works if we check after the page is finished loading, so the test passes.
I'll remove that bit from the code. By the way, is this a good location for this test file, or should I move it to `blink/web_tests/http/tests/preload/`? (Or somewhere else?)
Thanks for all the help so far!
I've removed the syncWait and moved the file to a (IMO) more suitable place.
Related details
- Charlie Harrison
- Mason Freed
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
Nice! Thanks for the test changes. LGTM from my end.
Related details
- Charlie Harrison
- Tarcísio Fischer
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Charlie Harrison (Gerrit)
Charlie Harrison voted Code-Review+1![]( "Open in Gerrit")
- Tarcísio Fischer
Code-Coverage
Code-Review
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Richard Townsend (Gerrit)
Richard Townsend voted and added 1 comment![]( "Open in Gerrit")
Votes added by Richard Townsend
| Commit-Queue | +2 |
1 comment
Thanks for the reviews all, great to see this one resolved. Sending to CQ on Tarcisio's behalf.
Best
Richard
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Preload page contents with CSP as soon as rules have been applied
Today, Chromium will stop preloading on the HTML document parser as soon
as it reaches a CSP meta tag. This CL will change that to re-enable
preloading as soon as CSP have been applied.
- M third_party/blink/renderer/core/html/parser/html_document_parser.cc
- M third_party/blink/renderer/core/html/parser/html_document_parser.h
- M third_party/blink/renderer/core/html/parser/html_document_parser_state.h
- M third_party/blink/renderer/core/html/parser/html_preload_scanner.cc
- M third_party/blink/renderer/core/html/parser/html_preload_scanner.h
- M third_party/blink/renderer/core/html/parser/html_preload_scanner_test.cc
- M third_party/blink/renderer/platform/runtime_enabled_features.json5
- A third_party/blink/web_tests/http/tests/preload/early_load_with_csp.html
Code-Review: +1 by Mason Freed, +1 by Charlie Harrison
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |