network service breaks rendering file://.*svgz URLs. |
|||||||||||
Issue descriptionURLRequestFileJob has magic handling of *.svgz files: Namely, it attaches a gzip filter to them (It doesn't do this for *.gz files). This allows svgz file URLs to be opened and viewer in Chrome (Both as top level URLs and when included in an image tag). When the network service is enabled, we don't use URLRequestFileJob, so we're completely unable to render svgz files (We don't decompress them in images, either). So I think we need to make a decision, before going to canary, to either drop svgz support in file URLs (In which case, we should drop support in the legacy path as well), or to fix this. If we go the former result, I'd advocate for removing svgz support from URLRequestFileJob before shipping the network service, so we see if that's going to be a problem, rather than tying shipping the network service to removing svgz file URL support.
,
Jul 12
this is something we will fix before pushing beta. This is rarely enough not warranted for canary.
,
Jul 13
Devs are more likely to be using canary than average, and this will likely cause the most problem for devs testing their sites using file URLs (Admittedly, most devs *probably* don't do that, since it breaks so much, but I'm not too confident of that). My main concern is breaking Canary to a degree that we scare people off.
,
Jul 17
i agree. Hopefully a release note should help this.
,
Aug 20
,
Sep 4
,
Sep 6
,
Sep 7
This issue is marked as a release blocker with no milestone associated. Please add an appropriate milestone. All release blocking issues should have milestones associated to it, so that the issue can tracked and the fixes can be pushed promptly. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Sep 7
,
Sep 7
,
Sep 18
,
Sep 18
Interesting observation: Windows properly displays the .svgz file, but Linux does not - it downloads it. I'm not sure this ever worked on Linux.
,
Sep 18
Ohh....if we see svgz, we attach a gzip extraction filter, but we rely on the normal mimetype logic (Which is based on file extension and platform settings + mime sniffing) to detect it's an svg file. We don't mime-sniff svg files, so we just depend on platform settings (And a hard-coded lookup table or two).
,
Sep 21
So this is easy to fix if we can read the entire *.svgz file into RAM and expand. It's a fair bit harder to do it the "right" way as I'd have to write something akin to net::GzipSourceStream. If this is a developer-only feature used mostly on desktop would it be acceptable to fix this the easy way?
,
Oct 5
I've got an approximately 80% working solution to this at https://crrev.com/c/1239536 that runs in the render process. Unfortunately it's got some threading issues and I believe fixing them will make this implementation quite complicated. An alternative is to expand the svgz stream in the render process. I suspect this will be less complex, but still a medium size chunk of code. I'm wondering if the right solution here is to drop this feature. At present (w/o network service) it works on Windows/Mac, but not Linux (CrOS untested). No other browsers (Edge/Firefox/Safari) support this. This was originally added for https://bugs.chromium.org/p/chromium/issues/detail?id=9936.
,
Oct 8
I'm fine with dropping the feature, though if we go that route, I think we should land a change now removing support, rather than relying on the network service switch over.
,
Nov 3
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae commit 6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae Author: Chris Mumford <cmumford@google.com> Date: Sat Nov 03 02:52:54 2018 Fix loading of file://.../*.svgz files w/network service. Chrome supports loading gzipped SVG files from the file scheme which was unsupported with the network service implementation. Note: The non-network service implementation of *.svgz loading is currently broken on Linux, but works on Windows and macOS. This change works on both platforms. Bug: 857117 Change-Id: Id90deba4761ea67d4ac7bef681051ea9326e113b Reviewed-on: https://chromium-review.googlesource.com/c/1268960 Commit-Queue: Chris Mumford <cmumford@chromium.org> Reviewed-by: Matt Menke <mmenke@chromium.org> Reviewed-by: Alex Moshchuk <alexmos@chromium.org> Cr-Commit-Position: refs/heads/master@{#605136} [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/content/browser/file_url_loader_factory.cc [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/content/renderer/loader/url_loader_client_impl.cc [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/content/renderer/loader/url_response_body_consumer.cc [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/content/renderer/loader/url_response_body_consumer.h [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/content/renderer/loader/url_response_body_consumer_unittest.cc [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/net/BUILD.gn [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/net/filter/gzip_source_stream.cc [modify] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/net/filter/gzip_source_stream.h [add] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/net/filter/zlib_stream_wrapper.cc [add] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/net/filter/zlib_stream_wrapper.h [add] https://crrev.com/6dc6ba26aa2e7ef4b3f33f2c29d0ff0cae95e7ae/net/filter/zlib_stream_wrapper_unittest.cc
,
Nov 5
The above CL caused a memory performance regression: https://bugs.chromium.org/p/chromium/issues/detail?id=901891. Currently testing a fix (https://chromium-review.googlesource.com/c/chromium/src/+/1317922) at https://pinpoint-dot-chromeperf.appspot.com/job/1009d38de40000
,
Nov 12
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba9a4288f7440532b91f99593dc59822c7478cd3 commit ba9a4288f7440532b91f99593dc59822c7478cd3 Author: Chris Mumford <cmumford@google.com> Date: Mon Nov 12 21:56:29 2018 Revert file://.../*.svgz files w/network service change. Revert two CL's that added support for loading *.svgz resources via the file scheme in the network service due to increased memory use. Original change's description: > Chrome supports loading gzipped SVG files from the file scheme > which was unsupported with the network service implementation. > > Note: The non-network service implementation of *.svgz loading > is currently broken on Linux, but works on Windows and macOS. > This change works on both platforms. > > Bug: 857117 > Change-Id: Id90deba4761ea67d4ac7bef681051ea9326e113b > Reviewed-on: https://chromium-review.googlesource.com/c/1268960 > Commit-Queue: Chris Mumford <cmumford@chromium.org> > Reviewed-by: Matt Menke <mmenke@chromium.org> > Reviewed-by: Alex Moshchuk <alexmos@chromium.org> > Cr-Commit-Position: refs/heads/master@{#605136} Also reverted a second CL containing a speculative fix: > Fix memory performance regression in file:///*.svgz filter. > > A speculative fix for a 7%-19.2% regression of memory use. This > change releases the ZLib buffers used for Gzip inflation - releasing > the memory as soon as it is no longer needed instead of waiting for > the URLResponseBodyConsumer instance to be deleted. > > This change also fixes an incorrect reset of |zlib_wrapper_|, which > was calling release() instead of reset(), when zlib would fail to > initialize. This was likely a latent bug and not the cause of > the regression. > > Bug: 901891 > Change-Id: I52687f5614b7a76c52f4b9d5317deb33a299e4cb > Reviewed-on: https://chromium-review.googlesource.com/c/1317922 > Reviewed-by: Yutaka Hirano <yhirano@chromium.org> > Commit-Queue: Chris Mumford <cmumford@chromium.org> > Cr-Commit-Position: refs/heads/master@{#607124} TBRing mmenke@ - main reviewer of initial CL. Note: These two CL's reverted cleanly with no conflicts. TBR=mmenke@chromium.org Bug: 901891 Change-Id: I700b994797c4c2ac10ffef74f5c449cb5dfc0da8 Reviewed-on: https://chromium-review.googlesource.com/c/1331610 Reviewed-by: Chris Mumford <cmumford@chromium.org> Commit-Queue: Chris Mumford <cmumford@chromium.org> Cr-Commit-Position: refs/heads/master@{#607341} [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/content/browser/file_url_loader_factory.cc [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/content/renderer/loader/url_loader_client_impl.cc [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/content/renderer/loader/url_response_body_consumer.cc [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/content/renderer/loader/url_response_body_consumer.h [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/content/renderer/loader/url_response_body_consumer_unittest.cc [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/net/BUILD.gn [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/net/filter/gzip_source_stream.cc [modify] https://crrev.com/ba9a4288f7440532b91f99593dc59822c7478cd3/net/filter/gzip_source_stream.h [delete] https://crrev.com/3c870ab9d8b126c1e36a27e505e20fd1f03234d8/net/filter/zlib_stream_wrapper.cc [delete] https://crrev.com/3c870ab9d8b126c1e36a27e505e20fd1f03234d8/net/filter/zlib_stream_wrapper.h [delete] https://crrev.com/3c870ab9d8b126c1e36a27e505e20fd1f03234d8/net/filter/zlib_stream_wrapper_unittest.cc
,
Nov 16
Hi, can you tell me what you are going to do next? Currently we're planning to drastically simplify response body handling in content/renderer/loader and that will make this kind of response body data modification difficult. If you want to do it in renderer I'd recommend to implement it in blink.
,
Nov 16
for M70, we will keep the behavior the same with network service on and network service off. for M71, we are adding UMA to see what the usage of svgz is. for M72+, depending on what the usage is, we might not be porting over this running network service.
,
Nov 28
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6fad75ff82feb7fab5780825bb52bf67ab65aec4 commit 6fad75ff82feb7fab5780825bb52bf67ab65aec4 Author: Chris Mumford <cmumford@google.com> Date: Wed Nov 28 20:11:56 2018 Add UMA counter for file:///*.svgz resource loads. Adding a UMA counter for file:///*.svgz file loads in order to ensure we aren't deprecating support for something widely used in Chrome. Bug: 857117 Change-Id: I74e40df6c9aebd33197c2a5cfaf1af83bb84ea51 Reviewed-on: https://chromium-review.googlesource.com/c/1338302 Reviewed-by: Brian White <bcwhite@chromium.org> Reviewed-by: Eric Roman <eroman@chromium.org> Commit-Queue: Chris Mumford <cmumford@chromium.org> Cr-Commit-Position: refs/heads/master@{#611842} [modify] https://crrev.com/6fad75ff82feb7fab5780825bb52bf67ab65aec4/net/url_request/url_request_file_job.cc [modify] https://crrev.com/6fad75ff82feb7fab5780825bb52bf67ab65aec4/tools/metrics/histograms/histograms.xml |
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by jam@chromium.org
, Jul 12