Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2440803002:
Filter out data urls in the preload scanner (Closed)
|
Created:
4 years, 2 months ago by Charlie Harrison Modified:
4 years, 1 month ago Reviewers:
Yoav Weiss CC:
chromium-reviews, blink-reviews-html_chromium.org, loading-reviews+parser_chromium.org, dglazkov+blink, blink-reviews, kinuko+watch Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFilter out data urls in the preload scanner
Previously, we happily constructed PreloadRequests with data URLs, only
to cancel them once they reached the HTMLResourcePreloader.
This entails a whole song and dance including copying the resource url.
Since data URLs are huge this is not ideal.
BUG=657978
Committed: https://crrev.com/ac38e9cdd60cf37902e040d78f8749b0e35c52a5
Cr-Commit-Position: refs/heads/master@{#427349}
Patch Set 1 #Patch Set 2 : also block ref urls #
Total comments: 1
Patch Set 3 : fix up #
Total comments: 9
Patch Set 4 : yoav comments #
Total comments: 9
Patch Set 5 : nits and test #
Messages
Total messages: 34 (20 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: + yoav@yoav.ws
Yoav, am I missing something here? When could a base url override result in a data URL, when the relative component is not already a data URL?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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_...)
On 2016/10/20 20:37:32, Charlie Harrison wrote: > Yoav, am I missing something here? When could a base url override result in a > data URL, when the relative component is not already a data URL? No need for base URL here, I think I was just not aware of the static string base protocolIs() when I reviewed this, and didn't want to KURL the string just in order to test the protocol. LGTM assuming the test failures are not related (they don't seem to be)
Actually, some of the test failures are related. Mostly the DCHECK I added is triggering. I think I am not properly filtering out CSS preloads. Let me amend the patch, sorry!
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...
So it looks like we're making preload requests for ref urls (i.e. src="#") which parse to data urls when they are paired with their associated base url. I *think* this is the only case where this can happen. In any case, I've fudged the code a bit more so that the preload request constructor has some logic. Let's see what bots think. https://codereview.chromium.org/2440803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/2440803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:487: will remove
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...
Yoav, would you mind taking another looks, since the implementation has changed a bit since your l-g-t-m?
https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (left): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:234: if (!url.isEmpty()) { Why create a request for empty URLs? https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:48: if (resourceURL.isEmpty() || resourceURL.startsWith("#") || Are absolute ref URLs an issue? What would happen with them?
Thanks https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (left): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:234: if (!url.isEmpty()) { On 2016/10/24 14:43:54, Yoav Weiss wrote: > Why create a request for empty URLs? I moved the isEmpty check to the PreloadRequest class. https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:48: if (resourceURL.isEmpty() || resourceURL.startsWith("#") || On 2016/10/24 14:43:54, Yoav Weiss wrote: > Are absolute ref URLs an issue? What would happen with them? I think absolute ref URLs are okay because we really just don't want the URL to canonicalize into a data URL. This will happen with relative ref URLs but not absolute ref URLs. I can probably amend this comment to make that a little clearer.
https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (left): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:234: if (!url.isEmpty()) { On 2016/10/24 14:49:31, Charlie Harrison wrote: > On 2016/10/24 14:43:54, Yoav Weiss wrote: > > Why create a request for empty URLs? > > I moved the isEmpty check to the PreloadRequest class. OK, thanks! https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (left): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:33: static std::unique_ptr<PreloadRequest> create( Usually `create()` doesn't fail in normal circumstances. Could you rename to createIfNeeded() or something similar? https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:48: if (resourceURL.isEmpty() || resourceURL.startsWith("#") || On 2016/10/24 14:49:31, Charlie Harrison wrote: > On 2016/10/24 14:43:54, Yoav Weiss wrote: > > Are absolute ref URLs an issue? What would happen with them? > > I think absolute ref URLs are okay because we really just don't want the URL to > canonicalize into a data URL. This will happen with relative ref URLs but not > absolute ref URLs. I can probably amend this comment to make that a little > clearer. Thanks. Clarifying that in the comment would be indeed helpful.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by csharrison@chromium.org to run a CQ dry run
Addressed all comments. PTAL thanks! https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (left): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:33: static std::unique_ptr<PreloadRequest> create( On 2016/10/24 15:00:32, Yoav Weiss wrote: > Usually `create()` doesn't fail in normal circumstances. Could you rename to > createIfNeeded() or something similar? Done. https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right): https://codereview.chromium.org/2440803002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:48: if (resourceURL.isEmpty() || resourceURL.startsWith("#") || On 2016/10/24 15:00:32, Yoav Weiss wrote: > On 2016/10/24 14:49:31, Charlie Harrison wrote: > > On 2016/10/24 14:43:54, Yoav Weiss wrote: > > > Are absolute ref URLs an issue? What would happen with them? > > > > I think absolute ref URLs are okay because we really just don't want the URL > to > > canonicalize into a data URL. This will happen with relative ref URLs but not > > absolute ref URLs. I can probably amend this comment to make that a little > > clearer. > > Thanks. Clarifying that in the comment would be indeed helpful. Done.
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: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
LGTM % nits and test case Can you add a test to HTMLPreloadScannerTest to see that data URIs and relative refs are in fact getting ignored? https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:236: std::unique_ptr<PreloadRequest> request = PreloadRequest::createIfNeeded( Nit: should this be "auto request"? https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:244: std::unique_ptr<PreloadRequest> request = PreloadRequest::createIfNeeded( same nit: should we auto this variable? https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp (left): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp:57: if (testCase.isCORS) Assuming we're certain here that request was created (which I think we are), can you DCHECK it? https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp:52: std::unique_ptr<PreloadRequest> preloadRequest = ditto https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/PreloadRequest.h (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/PreloadRequest.h:50: // overhead, which can be significant for large URLs. perfect :)
Thanks for the reviews! https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/CSSPreloadScanner.cpp:236: std::unique_ptr<PreloadRequest> request = PreloadRequest::createIfNeeded( On 2016/10/25 04:34:14, Yoav Weiss wrote: > Nit: should this be "auto request"? Sure. Done. https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLPreloadScanner.cpp:244: std::unique_ptr<PreloadRequest> request = PreloadRequest::createIfNeeded( On 2016/10/25 04:34:14, Yoav Weiss wrote: > same nit: should we auto this variable? Done. https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp (left): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp:57: if (testCase.isCORS) On 2016/10/25 04:34:14, Yoav Weiss wrote: > Assuming we're certain here that request was created (which I think we are), can > you DCHECK it? Done. https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp (right): https://codereview.chromium.org/2440803002/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/html/parser/HTMLResourcePreloaderTest.cpp:52: std::unique_ptr<PreloadRequest> preloadRequest = On 2016/10/25 04:34:14, Yoav Weiss wrote: > ditto Done.
The CQ bit was checked by csharrison@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yoav@yoav.ws Link to the patchset: https://codereview.chromium.org/2440803002/#ps80001 (title: "nits and test")
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 #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Filter out data urls in the preload scanner Previously, we happily constructed PreloadRequests with data URLs, only to cancel them once they reached the HTMLResourcePreloader. This entails a whole song and dance including copying the resource url. Since data URLs are huge this is not ideal. BUG=657978 ========== to ========== Filter out data urls in the preload scanner Previously, we happily constructed PreloadRequests with data URLs, only to cancel them once they reached the HTMLResourcePreloader. This entails a whole song and dance including copying the resource url. Since data URLs are huge this is not ideal. BUG=657978 Committed: https://crrev.com/ac38e9cdd60cf37902e040d78f8749b0e35c52a5 Cr-Commit-Position: refs/heads/master@{#427349} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/ac38e9cdd60cf37902e040d78f8749b0e35c52a5 Cr-Commit-Position: refs/heads/master@{#427349} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
