Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1512783002:
disable fetches for parser blocking scripts inserted via doc.write (Closed)
|
Created:
5 years ago by Bryan McQuade Modified:
5 years ago CC:
chromium-reviews, Yoav Weiss, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, Nate Chapin, kinuko Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Descriptiondisable fetches for parser blocking scripts inserted via doc.write
BUG=
Committed: https://crrev.com/d1864bd3ffc06096c2f5b64d9123059c28aa3aa5
Cr-Commit-Position: refs/heads/master@{#366218}
Patch Set 1 #Patch Set 2 : remove newline #Patch Set 3 : #Patch Set 4 : #Patch Set 5 : add a few comments #
Total comments: 6
Patch Set 6 : #
Total comments: 5
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : #
Total comments: 2
Patch Set 10 : #Patch Set 11 : #Patch Set 12 : #Patch Set 13 : #Patch Set 14 : #Patch Set 15 : #Patch Set 16 : #
Messages
Total messages: 18 (7 generated)
japhet@chromium.org changed reviewers: + japhet@chromium.org
When do scripts that trigger this case get re-requested if they're not in the cache already? https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/fetch/ScriptResource.cpp (right): https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/fetch/ScriptResource.cpp:47: request.mutableResourceRequest().setCachePolicy(ReturnCacheDataDontLoad); This would ideally go in FrameFetchContext::resourceRequestCachePolicy(), to minimize the number of places we set ResourceRequestCachePolicy. That also removes the need to expose allowScriptNetworkFetch() on FetchContext. https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:774: // tokenized later. The document.write intervention, as currently Describing this simply as 'the document.write intervention' is probably insufficient for this comment to make sense to an arbitrary observer. A link to a bug would probably help. https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:353: if (frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && isMainFrame() && FetchRequest::NoDefer == request.defer() && frame()->document() && frame()->document()->activeParserCount() > 1) activeParserCount() is on my list of things to delete :) It might be worth doing it right the first time, just to avoid propagating activeParserCount() usage, especially since its only non-assert usage at the moment is a kooky legacy function.
Thanks! RE: fetching these resources later, I'm not sure yet if we want to do that, but will resolve that question before we try to move this change forward. https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp (right): https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLDocumentParser.cpp:774: // tokenized later. The document.write intervention, as currently On 2015/12/09 18:03:21, Nate Chapin wrote: > Describing this simply as 'the document.write intervention' is probably > insufficient for this comment to make sense to an arbitrary observer. A link to > a bug would probably help. Yeah - sorry - I wasn't planning to check this comment in as is but wanted to document the current state of the implementation. https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:353: if (frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && isMainFrame() && FetchRequest::NoDefer == request.defer() && frame()->document() && frame()->document()->activeParserCount() > 1) On 2015/12/09 18:03:21, Nate Chapin wrote: > activeParserCount() is on my list of things to delete :) > > It might be worth doing it right the first time, just to avoid propagating > activeParserCount() usage, especially since its only non-assert usage at the > moment is a kooky legacy function. Turns out there is Document::isInDocumentWrite() which is more in line with what I need, so I've switched to it. RE: it being a possible abstraction violation to call isInDocumentWrite from FrameFetchContext, do you think that's the case? Should I try to plumb the fact that a script element was document.written through HTMLScriptElement to ScriptLoader and then down through? Or do you think it's better to check isInDocumentWrite from FrameFetchContext::resourceRequestCachePolicy? It's certainly easier to do the latter. If we do the former, it's not clear to me how to properly communicate that the script element was inserted during document.write in this call stack: ScriptLoader::fetchScript ScriptResource::fetch(FetchRequest&, ResourceFetcher*) ResourceFetcher::requestResource(FetchRequest&, const ResourceFactory&, const SubstituteData&) ResourceFetcher::createResourceForLoading(FetchRequest&, const String& charset, const ResourceFactory&) ResourceFetcher::initializeResourceRequest(ResourceRequest& request, Resource::Type type) FetchContext::resourceRequestCachePolicy(const ResourceRequest&, Resource::Type) Maybe we could add an annotation on FetchRequest noting something about it being inserted during document.write, but it's not clear how that gets mapped onto a ResourceRequest later in the call stack. This is part of the reason I do like explicitly setting the cache policy directly in ScriptResource::fetch - we have all the context we need at that point, and it's hard to pass it forward to FetchContext::resourceRequestCachePolicy, unless we are comfortable calling Document::isInDocumentWrite directly from FetchContext::resourceRequestCachePolicy, in which case we're fine. What do you think?
https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:353: if (frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && isMainFrame() && FetchRequest::NoDefer == request.defer() && frame()->document() && frame()->document()->activeParserCount() > 1) On 2015/12/09 20:30:55, Bryan McQuade wrote: > On 2015/12/09 18:03:21, Nate Chapin wrote: > > activeParserCount() is on my list of things to delete :) > > > > It might be worth doing it right the first time, just to avoid propagating > > activeParserCount() usage, especially since its only non-assert usage at the > > moment is a kooky legacy function. > > Turns out there is Document::isInDocumentWrite() which is more in line with what > I need, so I've switched to it. > > RE: it being a possible abstraction violation to call isInDocumentWrite from > FrameFetchContext, do you think that's the case? Should I try to plumb the fact > that a script element was document.written through HTMLScriptElement to > ScriptLoader and then down through? Or do you think it's better to check > isInDocumentWrite from FrameFetchContext::resourceRequestCachePolicy? It's > certainly easier to do the latter. > > If we do the former, it's not clear to me how to properly communicate that the > script element was inserted during document.write in this call stack: > ScriptLoader::fetchScript > ScriptResource::fetch(FetchRequest&, ResourceFetcher*) > ResourceFetcher::requestResource(FetchRequest&, const ResourceFactory&, const > SubstituteData&) > ResourceFetcher::createResourceForLoading(FetchRequest&, const String& charset, > const ResourceFactory&) > ResourceFetcher::initializeResourceRequest(ResourceRequest& request, > Resource::Type type) > FetchContext::resourceRequestCachePolicy(const ResourceRequest&, Resource::Type) > > Maybe we could add an annotation on FetchRequest noting something about it being > inserted during document.write, but it's not clear how that gets mapped onto a > ResourceRequest later in the call stack. > > This is part of the reason I do like explicitly setting the cache policy > directly in ScriptResource::fetch - we have all the context we need at that > point, and it's hard to pass it forward to > FetchContext::resourceRequestCachePolicy, unless we are comfortable calling > Document::isInDocumentWrite directly from > FetchContext::resourceRequestCachePolicy, in which case we're fine. > > What do you think? FrameFetchContext is allowed to read pretty much anything it can get to from a Frame* or a DocumentLoader*. Document is very close to both of those, so I'm inclined to say it's not a layering violation.
Description was changed from ========== disable fetches for parser blocking scripts inserted via doc.write BUG= ========== to ========== disable fetches for parser blocking scripts inserted via doc.write BUG= ==========
On 2015/12/09 21:33:30, Nate Chapin wrote: > https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): > > https://codereview.chromium.org/1512783002/diff/80001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:353: if > (frame()->settings() && > frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && > isMainFrame() && FetchRequest::NoDefer == request.defer() && frame()->document() > && frame()->document()->activeParserCount() > 1) > On 2015/12/09 20:30:55, Bryan McQuade wrote: > > On 2015/12/09 18:03:21, Nate Chapin wrote: > > > activeParserCount() is on my list of things to delete :) > > > > > > It might be worth doing it right the first time, just to avoid propagating > > > activeParserCount() usage, especially since its only non-assert usage at the > > > moment is a kooky legacy function. > > > > Turns out there is Document::isInDocumentWrite() which is more in line with > what > > I need, so I've switched to it. > > > > RE: it being a possible abstraction violation to call isInDocumentWrite from > > FrameFetchContext, do you think that's the case? Should I try to plumb the > fact > > that a script element was document.written through HTMLScriptElement to > > ScriptLoader and then down through? Or do you think it's better to check > > isInDocumentWrite from FrameFetchContext::resourceRequestCachePolicy? It's > > certainly easier to do the latter. > > > > If we do the former, it's not clear to me how to properly communicate that the > > script element was inserted during document.write in this call stack: > > ScriptLoader::fetchScript > > ScriptResource::fetch(FetchRequest&, ResourceFetcher*) > > ResourceFetcher::requestResource(FetchRequest&, const ResourceFactory&, const > > SubstituteData&) > > ResourceFetcher::createResourceForLoading(FetchRequest&, const String& > charset, > > const ResourceFactory&) > > ResourceFetcher::initializeResourceRequest(ResourceRequest& request, > > Resource::Type type) > > FetchContext::resourceRequestCachePolicy(const ResourceRequest&, > Resource::Type) > > > > Maybe we could add an annotation on FetchRequest noting something about it > being > > inserted during document.write, but it's not clear how that gets mapped onto a > > ResourceRequest later in the call stack. > > > > This is part of the reason I do like explicitly setting the cache policy > > directly in ScriptResource::fetch - we have all the context we need at that > > point, and it's hard to pass it forward to > > FetchContext::resourceRequestCachePolicy, unless we are comfortable calling > > Document::isInDocumentWrite directly from > > FetchContext::resourceRequestCachePolicy, in which case we're fine. > > > > What do you think? > > FrameFetchContext is allowed to read pretty much anything it can get to from a > Frame* or a DocumentLoader*. Document is very close to both of those, so I'm > inclined to say it's not a layering violation. Thanks Nate! I made these changes. The code is nice and simple now. :) There are still some open issues I'd need to address before I can commit (do we fetch the script in the background? etc), but I'm glad to get your early feedback to make sure I'm on the right track for this part of the change.
yoav@yoav.ws changed reviewers: + yoav@yoav.ws
A couple of nits :) https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:168: { Could you ASSERT(frame()) here to make sure this is always called when frame() is around? https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:200: if (Resource::Script == type && frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && isMainFrame() && frame()->document() && frame()->document()->isInDocumentWrite()) The yoda condition style is atypical - although if the style guide allows it, I don't particularly mind. Also, breaking out some of these comparisons to variables may be better for readability. e.g. bool isInDocumentWrite = frame()->document && frame()->document->isInDocumentWrite(); bool disallowDocWriteScripts = frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame();
https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:168: { On 2015/12/10 13:20:37, Yoav Weiss wrote: > Could you ASSERT(frame()) here to make sure this is always called when frame() > is around? Done. https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:200: if (Resource::Script == type && frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && isMainFrame() && frame()->document() && frame()->document()->isInDocumentWrite()) On 2015/12/10 13:20:37, Yoav Weiss wrote: > The yoda condition style is atypical - although if the style guide allows it, I > don't particularly mind. > Also, breaking out some of these comparisons to variables may be better for > readability. e.g. > bool isInDocumentWrite = frame()->document && > frame()->document->isInDocumentWrite(); > bool disallowDocWriteScripts = frame()->settings() && > frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame(); Thanks for the suggestion to break out the comparisons. Done. Much more readable now. :) https://codereview.chromium.org/1512783002/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:200: if (Resource::Script == type && frame()->settings() && frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame() && isMainFrame() && frame()->document() && frame()->document()->isInDocumentWrite()) On 2015/12/10 13:20:37, Yoav Weiss wrote: > The yoda condition style is atypical - although if the style guide allows it, I > don't particularly mind. > Also, breaking out some of these comparisons to variables may be better for > readability. e.g. > bool isInDocumentWrite = frame()->document && > frame()->document->isInDocumentWrite(); > bool disallowDocWriteScripts = frame()->settings() && > frame()->settings()->disallowFetchForDocWrittenScriptsInMainFrame(); reversed the yoda conditional.
LGTM https://codereview.chromium.org/1512783002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:203: const bool isInDocumentWrite = frame()->document() && frame()->document()->isInDocumentWrite(); Consistency nit: I was forgetting what pointers FrameFetchContext holds. This should probably check m_document instead of frame()->document().
https://codereview.chromium.org/1512783002/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/loader/FrameFetchContext.cpp (right): https://codereview.chromium.org/1512783002/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/loader/FrameFetchContext.cpp:203: const bool isInDocumentWrite = frame()->document() && frame()->document()->isInDocumentWrite(); On 2015/12/10 19:18:22, Nate Chapin wrote: > Consistency nit: I was forgetting what pointers FrameFetchContext holds. This > should probably check m_document instead of frame()->document(). done, thanks!
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from japhet@chromium.org Link to the patchset: https://codereview.chromium.org/1512783002/#ps300001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1512783002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1512783002/300001
Message was sent while issue was closed.
Description was changed from ========== disable fetches for parser blocking scripts inserted via doc.write BUG= ========== to ========== disable fetches for parser blocking scripts inserted via doc.write BUG= ==========
Message was sent while issue was closed.
Committed patchset #16 (id:300001)
Message was sent while issue was closed.
Description was changed from ========== disable fetches for parser blocking scripts inserted via doc.write BUG= ========== to ========== disable fetches for parser blocking scripts inserted via doc.write BUG= Committed: https://crrev.com/d1864bd3ffc06096c2f5b64d9123059c28aa3aa5 Cr-Commit-Position: refs/heads/master@{#366218} ==========
Message was sent while issue was closed.
Patchset 16 (id:??) landed as https://crrev.com/d1864bd3ffc06096c2f5b64d9123059c28aa3aa5 Cr-Commit-Position: refs/heads/master@{#366218} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
