Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2350813003:
NoState Prefetch: CSP test (Closed)
DescriptionNoState Prefetch: CSP test
We currently skip prefetching entirely if there is a CSP meta tag. This is
probably not something we want to do long term; this test will document that and
make it clear when we've changed it.
BUG=643652
Committed: https://crrev.com/5b870b676ffb62117e37fa64ee4a76b8182e099b
Cr-Commit-Position: refs/heads/master@{#425666}
Patch Set 1 #Patch Set 2 : response headers #
Total comments: 2
Patch Set 3 : comment/typo #
Total comments: 13
Patch Set 4 : comments #
Total comments: 6
Patch Set 5 : Move HttpsServer changes from dependency to this CL #Patch Set 6 : comments/rebase #Patch Set 7 : fix merge line ordering problem #Patch Set 8 : alphabetizing #Patch Set 9 : rebase #
Total comments: 3
Patch Set 10 : filled in bug #
Messages
Total messages: 26 (8 generated)
The CQ bit was checked by mattcary@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
mattcary@chromium.org changed reviewers: + droger@chromium.org, pasko@chromium.org
David and Egor: CSP tests. Thanks for the response-header hint, David.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
lgtm https://codereview.chromium.org/2350813003/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:246: // The second scrip is in the correct domain for CSP, but the first script is s/scrip/script/
https://codereview.chromium.org/2350813003/diff/20001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/20001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:246: // The second scrip is in the correct domain for CSP, but the first script is On 2016/09/21 11:05:39, droger wrote: > s/scrip/script/ Done.
I cannot remember how we decided to support CSP in the current iteration of prefetch, so I though I should ask a few questions first .. https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:71: base::FilePath url_file = can this function call the one below to avoid repetitions of the PostTask(Bind) spagetti? https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:80: void CountRequestFor(const GURL& url, CountRequestForUrl() looks better than this overload, to make grepping easier. https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:250: first_script.WaitForCount(0); does WaitForCount actually check that it won't count up to 1 at some point later? https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:254: // TODO(mattcary): probably we don't want to cancel this so we're consistent by 'this' do you mean the prefetch or the request? Why should the csp meta tag cancel the whole prefetch and not just a request? Is it this particular csp meta tag in the test that prevents the page from loading or you want it this way to be on extremely safe side? (I know, stupid questions in the evening..) https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:269: // the preload scanner if we see the CSP meta tag I am confused. What does it mean to 'bail out on the preload scanner'? Are you describing intentional behavior and the current one? What are they?
https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:71: base::FilePath url_file = On 2016/09/26 17:05:10, pasko wrote: > can this function call the one below to avoid repetitions of the PostTask(Bind) > spagetti? Yes, strange, that was my intention... https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:80: void CountRequestFor(const GURL& url, On 2016/09/26 17:05:10, pasko wrote: > CountRequestForUrl() looks better than this overload, to make grepping easier. Done. https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:250: first_script.WaitForCount(0); On 2016/09/26 17:05:10, pasko wrote: > does WaitForCount actually check that it won't count up to 1 at some point > later? No. There doesn't seem to be an easy way to confirm that, especially since we don't currently have any hooks into the prerenderer (for instance, it's not obvious to me how to tell that the prefetch scanner had finished, so even if we did hook into the queue of requests (which also seems hard), we couldn't be sure it was finished. Heuristically, by doing the WaitForCount(0) a the end, we ensure that the message loop has at least been pumped, which has in my experience caught when a request for the first script was actually made. But I suspect it's not reliable. https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:254: // TODO(mattcary): probably we don't want to cancel this so we're consistent On 2016/09/26 17:05:10, pasko wrote: > by 'this' do you mean the prefetch or the request? Why should the csp meta tag > cancel the whole prefetch and not just a request? Is it this particular csp meta > tag in the test that prevents the page from loading or you want it this way to > be on extremely safe side? > > (I know, stupid questions in the evening..) Clarified the comment. By "this" I mean the prerender. As soon as the prefetch scanner sees a meta csp tag in the main resource, it bails, which means no prefetching is done at all. In contrast, if there are CSP directives in the response headers, then the appropriate tests are done. I'm not sure what we want, this test is just documenting behavior for now. I think the issue is that plumbing the CSP stuff from the background parser up to where ever in the net stack it should be is hard. Probably we should put this on the list to fix, but due to the apparent rarity of the meta CSP tag I don't think this should block initial testing. I figured I'd make a bug, etc, to do the right thing with CSP, as soon as we figured out what that was, depending on comments from this CL. https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:269: // the preload scanner if we see the CSP meta tag On 2016/09/26 17:05:10, pasko wrote: > I am confused. What does it mean to 'bail out on the preload scanner'? Are you > describing intentional behavior and the current one? What are they? Clarified the comment. See above.
found that I forgot to publish a few drafts (see below) once you rebase this on top of the main testing page, I'd be glad to jump in for another round https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:250: first_script.WaitForCount(0); On 2016/09/27 08:13:18, mattcary wrote: > On 2016/09/26 17:05:10, pasko wrote: > > does WaitForCount actually check that it won't count up to 1 at some point > > later? > > No. There doesn't seem to be an easy way to confirm that, especially since we > don't currently have any hooks into the prerenderer (for instance, it's not > obvious to me how to tell that the prefetch scanner had finished, so even if we > did hook into the queue of requests (which also seems hard), we couldn't be sure > it was finished. > > Heuristically, by doing the WaitForCount(0) a the end, we ensure that the > message loop has at least been pumped, which has in my experience caught when a > request for the first script was actually made. But I suspect it's not reliable. Agreed. I am working on signalling from the scanner on when it is done (needed anyway to kill the renderer or give green light to a new prefetch, etc), it will open the opportunity to check our counter after that signal arrives. For now I think it is appropriate to add TODO(pasko): Wait for prefetch to be finished before checking the counts, once the signal is available. https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:254: // TODO(mattcary): probably we don't want to cancel this so we're consistent On 2016/09/27 08:13:18, mattcary wrote: > On 2016/09/26 17:05:10, pasko wrote: > > by 'this' do you mean the prefetch or the request? Why should the csp meta tag > > cancel the whole prefetch and not just a request? Is it this particular csp > meta > > tag in the test that prevents the page from loading or you want it this way to > > be on extremely safe side? > > > > (I know, stupid questions in the evening..) > > Clarified the comment. > > By "this" I mean the prerender. As soon as the prefetch scanner sees a meta csp > tag in the main resource, it bails, which means no prefetching is done at all. Ah, good to know. It is indeed good to have a test for this early stop, to get notified it changes in the parser. > In contrast, if there are CSP directives in the response headers, then the > appropriate tests are done. > > I'm not sure what we want, this test is just documenting behavior for now. I > think the issue is that plumbing the CSP stuff from the background parser up to > where ever in the net stack it should be is hard. Yes, I think it is hard to plumb it. And in any case, the recommended way to do CSP seems to be over headers, not <meta>, so hopefully we get good coverage even without this complexity. > Probably we should put this on the list to fix, but due to the apparent rarity > of the meta CSP tag I don't think this should block initial testing. I figured > I'd make a bug, etc, to do the right thing with CSP, as soon as we figured out > what that was, depending on comments from this CL. Agreed to track it as low pri. Before going this rote let's see if it affects performance. I added a task for this. https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:43: const char kPrefetchMetaCSP[] = "prerender/prefetch_meta_csp.html"; I liked how you sorted these before, what made you undo it? https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:267: // Check that response header CSP is handled correctly. Preferable: s/is handled correctly/prevents fetching a script from disallowed domain/.
https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/40001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:250: first_script.WaitForCount(0); On 2016/10/05 14:09:55, pasko wrote: > On 2016/09/27 08:13:18, mattcary wrote: > > On 2016/09/26 17:05:10, pasko wrote: > > > does WaitForCount actually check that it won't count up to 1 at some point > > > later? > > > > No. There doesn't seem to be an easy way to confirm that, especially since we > > don't currently have any hooks into the prerenderer (for instance, it's not > > obvious to me how to tell that the prefetch scanner had finished, so even if > we > > did hook into the queue of requests (which also seems hard), we couldn't be > sure > > it was finished. > > > > Heuristically, by doing the WaitForCount(0) a the end, we ensure that the > > message loop has at least been pumped, which has in my experience caught when > a > > request for the first script was actually made. But I suspect it's not > reliable. > > Agreed. I am working on signalling from the scanner on when it is done (needed > anyway to kill the renderer or give green light to a new prefetch, etc), it will > open the opportunity to check our counter after that signal arrives. For now I > think it is appropriate to add TODO(pasko): Wait for prefetch to be finished > before checking the counts, once the signal is available. Done. https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:43: const char kPrefetchMetaCSP[] = "prerender/prefetch_meta_csp.html"; On 2016/10/05 14:09:55, pasko wrote: > I liked how you sorted these before, what made you undo it? There may be a merge thing here: the names are sorted alphabetically, and I (hopefully) maintained that. https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:267: // Check that response header CSP is handled correctly. On 2016/10/05 14:09:55, pasko wrote: > Preferable: s/is handled correctly/prevents fetching a script from disallowed > domain/. > Done.
https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:43: const char kPrefetchMetaCSP[] = "prerender/prefetch_meta_csp.html"; On 2016/10/12 15:09:10, mattcary wrote: > On 2016/10/05 14:09:55, pasko wrote: > > I liked how you sorted these before, what made you undo it? > > There may be a merge thing here: the names are sorted alphabetically, and I > (hopefully) maintained that. kPrefetchLoopPage[] looks misordered ..
Note there is some merge confusion around the FilePath windows wchar issues that will be resolved once the dependent CL is sorted. https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... File chrome/browser/prerender/prerender_nostate_prefetch_test.cc (right): https://codereview.chromium.org/2350813003/diff/60001/chrome/browser/prerende... chrome/browser/prerender/prerender_nostate_prefetch_test.cc:43: const char kPrefetchMetaCSP[] = "prerender/prefetch_meta_csp.html"; On 2016/10/12 15:48:55, pasko wrote: > On 2016/10/12 15:09:10, mattcary wrote: > > On 2016/10/05 14:09:55, pasko wrote: > > > I liked how you sorted these before, what made you undo it? > > > > There may be a merge thing here: the names are sorted alphabetically, and I > > (hopefully) maintained that. > > kPrefetchLoopPage[] looks misordered .. Gah, thanks. Rebasing is hard.
On 2016/10/13 08:44:27, mattcary wrote: > Note there is some merge confusion around the FilePath windows wchar issues that > will be resolved once the dependent CL is sorted. OK, let me know. I am not any expert in windows wchar file paths, curious what it will end up being.
On 2016/10/13 10:05:30, pasko wrote: > On 2016/10/13 08:44:27, mattcary wrote: > > Note there is some merge confusion around the FilePath windows wchar issues > that > > will be resolved once the dependent CL is sorted. > > OK, let me know. I am not any expert in windows wchar file paths, curious what > it will end up being. All merged, PTAL
lgtm with a nit note: mkwst@ is an expertin in CSP, should we add him as a reviewer to confirm how headers should be different from <meta>? Can do for the next change where we try to address the TODO. https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:305: // response-header CSP. See crbug/XXX. nit: please substitute the XXX appropriately
https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:305: // response-header CSP. See crbug/XXX. On 2016/10/17 11:23:18, pasko wrote: > nit: please substitute the XXX appropriately Oops, bug created and this comment updated, thanks.
https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:305: // response-header CSP. See crbug/XXX. On 2016/10/17 11:32:08, mattcary wrote: > On 2016/10/17 11:23:18, pasko wrote: > > nit: please substitute the XXX appropriately > > Oops, bug created and this comment updated, thanks. .. and uploaded?
On 2016/10/17 11:39:26, pasko wrote: > https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... > File chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc (right): > > https://codereview.chromium.org/2350813003/diff/160001/chrome/browser/prerend... > chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc:305: // > response-header CSP. See crbug/XXX. > On 2016/10/17 11:32:08, mattcary wrote: > > On 2016/10/17 11:23:18, pasko wrote: > > > nit: please substitute the XXX appropriately > > > > Oops, bug created and this comment updated, thanks. > > .. and uploaded? yes, once enter is hit a sufficient number of times in the terminal :)
The CQ bit was checked by mattcary@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from droger@chromium.org, pasko@chromium.org Link to the patchset: https://codereview.chromium.org/2350813003/#ps180001 (title: "filled in bug")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #10 (id:180001)
Message was sent while issue was closed.
Description was changed from ========== NoState Prefetch: CSP test We currently skip prefetching entirely if there is a CSP meta tag. This is probably not something we want to do long term; this test will document that and make it clear when we've changed it. BUG=643652 ========== to ========== NoState Prefetch: CSP test We currently skip prefetching entirely if there is a CSP meta tag. This is probably not something we want to do long term; this test will document that and make it clear when we've changed it. BUG=643652 Committed: https://crrev.com/5b870b676ffb62117e37fa64ee4a76b8182e099b Cr-Commit-Position: refs/heads/master@{#425666} ==========
Message was sent while issue was closed.
Patchset 10 (id:??) landed as https://crrev.com/5b870b676ffb62117e37fa64ee4a76b8182e099b Cr-Commit-Position: refs/heads/master@{#425666} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
