Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2242223003:
Preload tokens even if a <meta> csp tag is found (Closed)
|
Created:
4 years, 4 months ago by Charlie Harrison Modified:
4 years, 3 months ago CC:
blink-reviews, blink-reviews-html_chromium.org, chromium-reviews, dglazkov+blink, kinuko+watch, loading-reviews+parser_chromium.org, Yoav Weiss Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPreload tokens even if a <meta> csp tag is found
This patch adds logic to the preload scanner to keep track of csp
declarations in the markup and sends that data to the main thread in a
TokenizedChunk. The main thread will defer preload fetches if any chunk
indicates that it contains a <meta> csp tag, until that tag is parsed and
applied.
The current code simply stops preload scanning if this markup if ever found.
This patch also refactors some logic in HTMLDocumentParser now that we have
two conditions in which preloads can be deferred (previously we only
deferred preloads while waiting for AppCache to be initialized).
BUG=434230
Committed: https://crrev.com/6873913202e7304e4eb1da070554dba88236a71a
Cr-Commit-Position: refs/heads/master@{#413748}
Patch Set 1 #Patch Set 2 : less refactoring #
Total comments: 11
Patch Set 3 : Added another layout test. Use index into |tokens| in TokenizedChunk #
Total comments: 6
Patch Set 4 : use -1 instead of WTF::Optional #
Total comments: 4
Patch Set 5 : kouhei review #Patch Set 6 : Fix up layout test #
Total comments: 1
Messages
Total messages: 51 (28 generated)
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + kouhei@chromium.org, mkwst@chromium.org
kouhei, mkwst, wdyt? I believe the current PS is at least as safe as the current behavior. Failure points are of course anything dynamically adding <meta> csp from a script, which will fail today.
The CQ bit was checked by csharrison@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...
csharrison@chromium.org changed reviewers: + yoav@yoav.ws - kouhei@chromium.org
Moving yoav to review since kouhei is OOO.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
kouhei@chromium.org changed reviewers: + kouhei@chromium.org
https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html:17: assert_greater_than(boundedStart, r.startTime); Should we use internals.isPreloaded? https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:135: CompactHTMLToken* m_pendingCSPMetaTag; Can we use index here? CompactHTMLToken* scares me. https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:118: CompactHTMLToken* pendingCSPMetaTag; Let's use index as in "likelyDocumentWriteScriptIndices"
https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html:2: <meta http-equiv="Content-Security-Policy" content="script-src 'self' 'unsafe-inline'; font-src 'none'"> Can you add a more complicated test as well? Something like: ``` <meta [policy1]> <script> <meta [policy2]> <script> <meta [policy3]> <link> ```
The concept LGTM, but I'll defer to yoav@ and kouhei@ on the details of the preload scanner interaction.
https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:118: CompactHTMLToken* pendingCSPMetaTag; On 2016/08/19 04:56:15, kouhei wrote: > Let's use index as in "likelyDocumentWriteScriptIndices" SGTM but we'll have to pull the pointer out when the chunk reaches the main thread, unless we have some complicated logic to index into m_speculations.
The CQ bit was checked by csharrison@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...
Thanks for the reviews. The current PS uses indices in the BackgroundHTMLParser but converts them to pointers in the HTMLDocumentParser. I think the alternative would be pretty ugly (i.e. keeping track of indices in m_speculations). I had a bold idea to keep a map of CSPToken -> [queued preloads], but it requires adding state to PreloadRequest indicating where in the Chunk the preload actually is. If multiple meta tokens are common, or the conservative way of queuing preloads in this CL is too conservative, we can consider doing something like that. https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html:17: assert_greater_than(boundedStart, r.startTime); On 2016/08/19 04:56:15, kouhei wrote: > Should we use internals.isPreloaded? Done. isPreloaded will return true even if the resource wasn't used, right? https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:135: CompactHTMLToken* m_pendingCSPMetaTag; On 2016/08/19 04:56:15, kouhei wrote: > Can we use index here? CompactHTMLToken* scares me. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by csharrison@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...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/http/tests/preload/meta-csp.html:17: assert_greater_than(boundedStart, r.startTime); On 2016/08/19 15:13:40, Charlie Harrison wrote: > On 2016/08/19 04:56:15, kouhei wrote: > > Should we use internals.isPreloaded? > > Done. isPreloaded will return true even if the resource wasn't used, right? Yes. https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:118: CompactHTMLToken* pendingCSPMetaTag; On 2016/08/19 12:33:33, Charlie Harrison wrote: > On 2016/08/19 04:56:15, kouhei wrote: > > Let's use index as in "likelyDocumentWriteScriptIndices" > > SGTM but we'll have to pull the pointer out when the chunk reaches the main > thread, unless we have some complicated logic to index into m_speculations. I'm thinking of keeping a defer counter on the main thread side. Let me try that in a separate CL. https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:331: chunk->pendingCSPMetaTokenIndex.swap(m_pendingCSPMetaTokenIndex); chunk->pendingCSPMetaTokenIndex = m_pendingCSPMetaTokenIndex; https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:135: WTF::Optional<int> m_pendingCSPMetaTokenIndex; Checked with TOK C++ experts. I think we should go with a reserved value (-1) for invalid token index. WTF::Optional is cute, but is an overkill here. https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:319: for (auto& chunk : pendingChunks) { Can we skip this if(m_pendingCSPToken)? or do we need to update to the next meta tag?
Thanks! https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:118: CompactHTMLToken* pendingCSPMetaTag; On 2016/08/22 05:34:03, kouhei wrote: > On 2016/08/19 12:33:33, Charlie Harrison wrote: > > On 2016/08/19 04:56:15, kouhei wrote: > > > Let's use index as in "likelyDocumentWriteScriptIndices" > > > > SGTM but we'll have to pull the pointer out when the chunk reaches the main > > thread, unless we have some complicated logic to index into m_speculations. > > I'm thinking of keeping a defer counter on the main thread side. Let me try that > in a separate CL. Do you mean a followup? I'd be happy to implement this in the current CL if you'd like. https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:331: chunk->pendingCSPMetaTokenIndex.swap(m_pendingCSPMetaTokenIndex); On 2016/08/22 05:34:03, kouhei wrote: > chunk->pendingCSPMetaTokenIndex = m_pendingCSPMetaTokenIndex; I used swap to also clear the m_pendingCSPMetaTokenIndex member. That ensures it only tracks data for the current chunk. Updated this to reset the member to -1. https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:135: WTF::Optional<int> m_pendingCSPMetaTokenIndex; On 2016/08/22 05:34:03, kouhei wrote: > Checked with TOK C++ experts. I think we should go with a reserved value (-1) > for invalid token index. WTF::Optional is cute, but is an overkill here. SGTM. Changed it. https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/2242223003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:319: for (auto& chunk : pendingChunks) { On 2016/08/22 05:34:03, kouhei wrote: > Can we skip this if(m_pendingCSPToken)? or do we need to update to the next meta > tag? I chose to be conservative in this patch and always update the CSP tag. This ensures that any token after any <meta> csp tag is guaranteed to have its policies applied. If we wanted to be looser, we would need to have multiple defer lists.
lgtm % static constexpr https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h (right): https://codereview.chromium.org/2242223003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.h:118: CompactHTMLToken* pendingCSPMetaTag; On 2016/08/22 14:08:05, Charlie Harrison wrote: > On 2016/08/22 05:34:03, kouhei wrote: > > On 2016/08/19 12:33:33, Charlie Harrison wrote: > > > On 2016/08/19 04:56:15, kouhei wrote: > > > > Let's use index as in "likelyDocumentWriteScriptIndices" > > > > > > SGTM but we'll have to pull the pointer out when the chunk reaches the main > > > thread, unless we have some complicated logic to index into m_speculations. > > > > I'm thinking of keeping a defer counter on the main thread side. Let me try > that > > in a separate CL. > > Do you mean a followup? I'd be happy to implement this in the current CL if > you'd like. I mean a follow up. I'm not sure if its really possible (esp with conservative check where last token wins), so let not that block you. https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:332: chunk->pendingCSPMetaTokenIndex = m_pendingCSPMetaTokenIndex; single space after = https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:136: int m_pendingCSPMetaTokenIndex; static constexpr int noPendingToken = -1;
Thanks! Can you let me know if you mind adding the noPendingToken int on TokenizedChunk? If so, feel free to CQ. https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp (right): https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.cpp:332: chunk->pendingCSPMetaTokenIndex = m_pendingCSPMetaTokenIndex; On 2016/08/23 02:18:39, kouhei wrote: > single space after = Done. https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h (right): https://codereview.chromium.org/2242223003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/BackgroundHTMLParser.h:136: int m_pendingCSPMetaTokenIndex; On 2016/08/23 02:18:39, kouhei wrote: > static constexpr int noPendingToken = -1; Hm, that member should be public somehow, right? Otherwise this could change and the main thread will have to be updated as well. I'll add this to the TokenizedChunk.
lgtm cq
The CQ bit was checked by kouhei@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org Link to the patchset: https://codereview.chromium.org/2242223003/#ps80001 (title: "kouhei review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by csharrison@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...
Looks like there's one layout tests that assumes we won't be preloading. Updated it.
The CQ bit was unchecked by csharrison@chromium.org
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mkwst@chromium.org, kouhei@chromium.org Link to the patchset: https://codereview.chromium.org/2242223003/#ps100001 (title: "Fix up layout test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by csharrison@chromium.org
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 #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Preload tokens even if a <meta> csp tag is found This patch adds logic to the preload scanner to keep track of csp declarations in the markup and sends that data to the main thread in a TokenizedChunk. The main thread will defer preload fetches if any chunk indicates that it contains a <meta> csp tag, until that tag is parsed and applied. The current code simply stops preload scanning if this markup if ever found. This patch also refactors some logic in HTMLDocumentParser now that we have two conditions in which preloads can be deferred (previously we only deferred preloads while waiting for AppCache to be initialized). BUG=434230 ========== to ========== Preload tokens even if a <meta> csp tag is found This patch adds logic to the preload scanner to keep track of csp declarations in the markup and sends that data to the main thread in a TokenizedChunk. The main thread will defer preload fetches if any chunk indicates that it contains a <meta> csp tag, until that tag is parsed and applied. The current code simply stops preload scanning if this markup if ever found. This patch also refactors some logic in HTMLDocumentParser now that we have two conditions in which preloads can be deferred (previously we only deferred preloads while waiting for AppCache to be initialized). BUG=434230 Committed: https://crrev.com/6873913202e7304e4eb1da070554dba88236a71a Cr-Commit-Position: refs/heads/master@{#413748} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/6873913202e7304e4eb1da070554dba88236a71a Cr-Commit-Position: refs/heads/master@{#413748}
Message was sent while issue was closed.
Apologies for failing to review in time... I think the tests can be improved (One of them fails on https://codereview.chromium.org/2275493002/ and I suspect it's a spurious failure) https://codereview.chromium.org/2242223003/diff/100001/third_party/WebKit/Lay... File third_party/WebKit/LayoutTests/http/tests/preload/multiple-meta-csp.html (right): https://codereview.chromium.org/2242223003/diff/100001/third_party/WebKit/Lay... third_party/WebKit/LayoutTests/http/tests/preload/multiple-meta-csp.html:21: assert_true(internals.isPreloaded('../resources/testharnessreport.js')); isPreloaded is not very reliable when used after the resources were already discovered. You probably want to run the test in a blocking script that runs before the parser discovers all the resources.
Message was sent while issue was closed.
On 2016/09/02 10:23:53, Yoav Weiss wrote: > Apologies for failing to review in time... I think the tests can be improved > (One of them fails on https://codereview.chromium.org/2275493002/ and I suspect > it's a spurious failure) > > https://codereview.chromium.org/2242223003/diff/100001/third_party/WebKit/Lay... > File third_party/WebKit/LayoutTests/http/tests/preload/multiple-meta-csp.html > (right): > > https://codereview.chromium.org/2242223003/diff/100001/third_party/WebKit/Lay... > third_party/WebKit/LayoutTests/http/tests/preload/multiple-meta-csp.html:21: > assert_true(internals.isPreloaded('../resources/testharnessreport.js')); > isPreloaded is not very reliable when used after the resources were already > discovered. You probably want to run the test in a blocking script that runs > before the parser discovers all the resources. hiroshige@ just started working on this: crbug.com/643621
Message was sent while issue was closed.
Ideally I'd like to fix isPreloaded for this change, as the preloads are to be sent out immediately after the csp is applied. This makes tests where preloads come right after csp in the markup more easily understood, without going into complications w/ scripts about where the parsed chunk stops/starts and whether the preload is discovered by the time the csp is processed. |
