New issue
Advanced search Search tips

Issue 857117 link

Starred by 6 users

Issue metadata

Status: Started
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Android , Windows , Chrome , Mac
Pri: 1
Type: Bug



Sign in to add a comment

network service breaks rendering file://.*svgz URLs.

Project Member Reported by mmenke@chromium.org, Jun 27 2018

Issue description

URLRequestFileJob 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.
 
Labels: OS-Android OS-Chrome OS-Linux OS-Mac OS-Windows
Labels: -Proj-Servicification-Canary Proj-Servicification
this is something we will fix before pushing beta. This is rarely enough not warranted for canary.
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.
Labels: Hotlist-KnownIssue
i agree. Hopefully a release note should help this.
Owner: cmumford@chromium.org
Status: Assigned (was: Untriaged)
Labels: -Hotlist-KnownIssue
Labels: ReleaseBlock-Stable
Project Member

Comment 8 by sheriffbot@chromium.org, Sep 7

Cc: dxie@google.com
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
Labels: -ReleaseBlock-Stable
Labels: Proj-Servicification-Stable Hotlist-KnownIssue
Status: Started (was: Assigned)
Interesting observation: Windows properly displays the .svgz file, but Linux does not - it downloads it. I'm not sure this ever worked on Linux.
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).
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?
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.
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.
Project Member

Comment 17 by bugdroid1@chromium.org, 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

Project Member

Comment 19 by bugdroid1@chromium.org, 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

Cc: yhirano@chromium.org
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.
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.
Project Member

Comment 22 by bugdroid1@chromium.org, 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