New issue
Advanced search Search tips

Issue 892461 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Oct 9
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug



Sign in to add a comment

Broken images appear when clicking "show original" on some pages that use LoFi previews

Project Member Reported by sclit...@chromium.org, Oct 5

Issue description

Chrome Version: since 70.0.3524.0
OS: Android

What steps will reproduce the problem?
(1) Enable "Client-side Lo-Fi previews" and "Previews Allowed" in chrome://flags, plus set "Override effective connection type" to 2G.
(2) Open a site with lots of images, e.g. https://www.pexels.com/search/nature
(3) Click "show original" on the infobar.

What is the expected result?

All of the full original images load in.

What happens instead?

Some images load in fully, but for some images, only a top slice of the image gets shown. See attached screenshot.

This bug has the same root cause as  Issue 888742 , but since LoFi is already launched I've separated this into a separate bug to fix and hopefully merge into M70.
 
Screenshot_20181004-173040.png
169 KB View Download
Summary: Broken images appear when clicking "show original" on a page that uses LoFi previews (was: Broken images appeat when clicking "show original" on a page that uses LoFi previews)
Summary: Broken images appear when clicking "show original" on some pages that use LoFi previews (was: Broken images appear when clicking "show original" on a page that uses LoFi previews)

Comment 3 Deleted

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 5

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ecfb06487f00a123cabfdbf48cb10e8efc61400d

commit ecfb06487f00a123cabfdbf48cb10e8efc61400d
Author: Scott Little <sclittle@chromium.org>
Date: Fri Oct 05 18:54:19 2018

Bypass the cache when reloading LoFi images.

This is a partial revert of
https://chromium-review.googlesource.com/c/chromium/src/+/1166600, as a
temporary workaround to the bug listed below, which is caused by a bug
in the network stack that affects partial range cache revalidations.

This CL only bypasses the cache when reloading placeholders for LoFi,
and not when reloading placeholders for lazily loaded images, because
bypassing the cache when lazily loading images can potentially waste a
lot of network data, plus lazy image loading isn't launched yet, so
keeping the behavior as-is will make it easier to debug and fix the
underlying problem.

Bug:  892461 
Change-Id: I96ed023431fc4e7e346497820da1de14940aca64
Reviewed-on: https://chromium-review.googlesource.com/c/1263565
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597219}
[modify] https://crrev.com/ecfb06487f00a123cabfdbf48cb10e8efc61400d/third_party/blink/renderer/core/loader/resource/image_resource.cc
[modify] https://crrev.com/ecfb06487f00a123cabfdbf48cb10e8efc61400d/third_party/blink/renderer/platform/loader/fetch/resource.cc
[modify] https://crrev.com/ecfb06487f00a123cabfdbf48cb10e8efc61400d/third_party/blink/renderer/platform/loader/fetch/resource.h

Labels: Merge-Request-70
Is there any way we can merge this into M70? The fix above is quite simple and safe - it's basically just a partial revert of an older CL, and it would be nice if users didn't have to see a broken experience in Stable. 
Project Member

Comment 6 by sheriffbot@chromium.org, Oct 8

Labels: -Merge-Request-70 Merge-Review-70 Hotlist-Merge-Review
This bug requires manual review: We are only 7 days from stable.
Please contact the milestone owner if you have questions.
Owners: benmason@(Android), kariahda@(iOS), geohsu@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Cc: benmason@chromium.org
@benmason: Friendly ping about merging into Android M70.
Labels: -Hotlist-Merge-Review -Merge-Review-70 Merge-Approved-70
Approved for merge to 70, branch 3538.
Labels: -Merge-Approved-70 Merge-Merged-70-3538
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/fb0468cb544bb9ce4f130db21da3c2e3c62dab57

Commit: fb0468cb544bb9ce4f130db21da3c2e3c62dab57
Author: sclittle@chromium.org
Commiter: sclittle@chromium.org
Date: 2018-10-09 15:32:48 +0000 UTC

Bypass the cache when reloading LoFi images.

This is a partial revert of
https://chromium-review.googlesource.com/c/chromium/src/+/1166600, as a
temporary workaround to the bug listed below, which is caused by a bug
in the network stack that affects partial range cache revalidations.

This CL only bypasses the cache when reloading placeholders for LoFi,
and not when reloading placeholders for lazily loaded images, because
bypassing the cache when lazily loading images can potentially waste a
lot of network data, plus lazy image loading isn't launched yet, so
keeping the behavior as-is will make it easier to debug and fix the
underlying problem.

Bug:  892461 
Change-Id: I96ed023431fc4e7e346497820da1de14940aca64
Reviewed-on: https://chromium-review.googlesource.com/c/1263565
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Scott Little <sclittle@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#597219}(cherry picked from commit ecfb06487f00a123cabfdbf48cb10e8efc61400d)
Reviewed-on: https://chromium-review.googlesource.com/c/1269761
Reviewed-by: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#921}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
Project Member

Comment 10 by bugdroid1@chromium.org, Oct 9

Labels: merge-merged-3538
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb0468cb544bb9ce4f130db21da3c2e3c62dab57

commit fb0468cb544bb9ce4f130db21da3c2e3c62dab57
Author: Scott Little <sclittle@chromium.org>
Date: Tue Oct 09 15:32:48 2018

Bypass the cache when reloading LoFi images.

This is a partial revert of
https://chromium-review.googlesource.com/c/chromium/src/+/1166600, as a
temporary workaround to the bug listed below, which is caused by a bug
in the network stack that affects partial range cache revalidations.

This CL only bypasses the cache when reloading placeholders for LoFi,
and not when reloading placeholders for lazily loaded images, because
bypassing the cache when lazily loading images can potentially waste a
lot of network data, plus lazy image loading isn't launched yet, so
keeping the behavior as-is will make it easier to debug and fix the
underlying problem.

Bug:  892461 
Change-Id: I96ed023431fc4e7e346497820da1de14940aca64
Reviewed-on: https://chromium-review.googlesource.com/c/1263565
Reviewed-by: Kinuko Yasuda <kinuko@chromium.org>
Commit-Queue: Scott Little <sclittle@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#597219}(cherry picked from commit ecfb06487f00a123cabfdbf48cb10e8efc61400d)
Reviewed-on: https://chromium-review.googlesource.com/c/1269761
Reviewed-by: Scott Little <sclittle@chromium.org>
Cr-Commit-Position: refs/branch-heads/3538@{#921}
Cr-Branched-From: 79f7c91a2b2a2932cd447fa6f865cb6662fa8fa6-refs/heads/master@{#587811}
[modify] https://crrev.com/fb0468cb544bb9ce4f130db21da3c2e3c62dab57/third_party/blink/renderer/core/loader/resource/image_resource.cc
[modify] https://crrev.com/fb0468cb544bb9ce4f130db21da3c2e3c62dab57/third_party/blink/renderer/platform/loader/fetch/resource.cc
[modify] https://crrev.com/fb0468cb544bb9ce4f130db21da3c2e3c62dab57/third_party/blink/renderer/platform/loader/fetch/resource.h

Status: Fixed (was: Started)

Sign in to add a comment