Project: chromium Issues People Development process History Sign in
New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Issue 632495 (Single) image loading in Chrome 52+ perceived slower due to white boxes with grey border (+ JPEGs not progressive)
Starred by 12 users Reported by kaihollb...@googlemail.com, Jul 28 2016 Back to list
Status: Fixed
Owner:
Closed: Oct 3
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux, Windows, Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment
Chrome Version       : 52+ - '52.0.2743.82'+, including current canary '54.0.2810.2 canary (64-bit)'
URLs (if applicable) : E.g. http://img.zeit.de/entdecken/reisen/2016-07/wales-coast-path1/original (Any image, preferably progressive JPEG)
Other browsers tested:
  Not applicable

What steps will reproduce the problem?
(1) Use Chrome 52 or newer
(2) Load an image, e.g. http://img.zeit.de/entdecken/reisen/2016-07/wales-coast-path1/original
(3) See small and large white box with border instead of the image loading. Esp. for progressive JPEG, this is perceived way slower.

What is the expected result?
- Loading behaviour of Chrome < 52, e.g. 51

What happens instead?
- White boxes, image paints when loaed (perceived way slower)

Please provide any additional information below. Attach a screenshot if
possible.
- For easier reproduction use a large image and disable cache and throttle the network to, e.g. 'DSL' preset or slower, using DevTools
- I recorded a video of the previous behaviour and the buggy one which is attached, also viewable here: http://webmshare.com/07A7m
- Screenshot of the loading-state is also attached
- Doesnt appear to occur and regular page load, but still very annoying, esp. when debugging
 
Chrome_canary_image.PNG
22.1 KB View Download
Chrome_52.0.2743.82_vs_Chrome_54.0.2810.2_canary_(64-bit)_Loading_Image.webm
4.1 MB Download
Failed to mention: Tested Windows 7, Windows 10, OS X El Capitan 10.11.6
Cc: hirosh...@chromium.org yhirano@chromium.org
Components: Blink
Labels: -Type-Bug -Pri-3 M-52 OS-Linux OS-Mac OS-Windows Pri-1 Type-Bug-Regression
Owner: japhet@chromium.org
Status: Assigned
Able to reproduce the issue on win8.1 chrome version 54.0.2810.2 - Loadind teh page takes some time with white box displayed at the left most corner of the page.

This is working fine in M51 and hence a regression
Manual Bisect:

Good Build:52.0.2718.0 
Bad Build:52.0.2719.0

Bisect Tool Info:

https://chromium.googlesource.com/chromium/src/+log/f9a87728113bccfac096b340f59a03f877327359..33a081977fa4f03d96265f6ada2d3347d9f0db46

Possible suspect : https://codereview.chromium.org/1879793003

Issue seen in mac and Linux as well

Comment 3 by kojii@chromium.org, Jul 30 2016
Components: -Blink Blink>Loader
Cc: -hirosh...@chromium.org japhet@chromium.org
Owner: hirosh...@chromium.org
Status: Started
Confirmed https://codereview.chromium.org/1879793003 was the first bad commit.
This was because:
After |setImage(ImageResource::create(imageSourceToKURL(m_element->imageSourceURL())));| in ImageLoader::updateFromElement(), m_hasPendingLoadEvent is false (set in ImageLoader::setImageWithoutConsideringPendingLoadEvent()).
So ImageLoader is considered as finished, but ImageResource doesn't have image, so considered as broken.

Project Member Comment 6 by bugdroid1@chromium.org, Aug 24 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/576c7a048bfc30faf510eeda07f303fa9dd12898

commit 576c7a048bfc30faf510eeda07f303fa9dd12898
Author: hiroshige <hiroshige@chromium.org>
Date: Wed Aug 24 07:45:22 2016

Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in ImageDocument

Case 1: When a image is loaded as a subresource, loading is stared by
ImageResource::fetch() in ImageLoader::doUpdateFromElement().
Case 2: When a image is loaded as a main document, the start of the loading is
emulated by setImage() in ImageLoader::updateFromElement() and
ImageDocumentParser will do the subsequent loading steps.

However in Case 2, |m_hasPendingLoadEvent| is set to false
(in setImageWithoutConsideringPendingLoadEvent()), causing |imageStillLoading|
to be false and ensureFallbackContent() to be called in
HTMLImageElement::selectSourceURL(), and thus a box indicating a broken image
is displayed during loading instead of the image displayed progressively.
This is regression since https://codereview.chromium.org/1879793003.

This CL makes the behavior of Case 1 and 2 consistent, by moving what is done
in Case 1 to setImagePending(), and makes Case 1 and 2 both to call
setImagePending().

This CL also adds a layout test to confirm that an image is displayed
progressively when loaded as a main document.

BUG= 632495 

Review-Url: https://codereview.chromium.org/2208073004
Cr-Commit-Position: refs/heads/master@{#414008}

[add] https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[add] https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[add] https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898/third_party/WebKit/Source/core/loader/ImageLoader.h

Project Member Comment 7 by bugdroid1@chromium.org, Aug 25 2016
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8b3f5df1a65187e662fcf2fd5ead1ccb89226f31

commit 8b3f5df1a65187e662fcf2fd5ead1ccb89226f31
Author: johnme <johnme@chromium.org>
Date: Thu Aug 25 15:30:16 2016

Revert of Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in ImageDocument (patchset #4 id:60001 of https://codereview.chromium.org/2208073004/ )

Reason for revert:
Reverting since http/tests/images/png-partial-load-as-document.html consistently leaks on WebKit Linux Leak and LeakExpectations says "Sheriff is expected to revert culprit CLs instead of suppressing the leaks"

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Fimages%2Fpng-partial-load-as-document.html&testType=webkit_tests

Original issue's description:
> Fix ImageLoader::m_hasPendingLoadEvent/m_imageComplete in ImageDocument
>
> Case 1: When a image is loaded as a subresource, loading is stared by
> ImageResource::fetch() in ImageLoader::doUpdateFromElement().
> Case 2: When a image is loaded as a main document, the start of the loading is
> emulated by setImage() in ImageLoader::updateFromElement() and
> ImageDocumentParser will do the subsequent loading steps.
>
> However in Case 2, |m_hasPendingLoadEvent| is set to false
> (in setImageWithoutConsideringPendingLoadEvent()), causing |imageStillLoading|
> to be false and ensureFallbackContent() to be called in
> HTMLImageElement::selectSourceURL(), and thus a box indicating a broken image
> is displayed during loading instead of the image displayed progressively.
> This is regression since https://codereview.chromium.org/1879793003.
>
> This CL makes the behavior of Case 1 and 2 consistent, by moving what is done
> in Case 1 to setImagePending(), and makes Case 1 and 2 both to call
> setImagePending().
>
> This CL also adds a layout test to confirm that an image is displayed
> progressively when loaded as a main document.
>
> BUG= 632495 
>
> Committed: https://crrev.com/576c7a048bfc30faf510eeda07f303fa9dd12898
> Cr-Commit-Position: refs/heads/master@{#414008}

TBR=japhet@chromium.org,yhirano@chromium.org,hiroshige@chromium.org
NOTRY=true
NOTREECHECKS=true
BUG= 632495 

Review-Url: https://codereview.chromium.org/2278953002
Cr-Commit-Position: refs/heads/master@{#414441}

[delete] https://crrev.com/19ad77688a08480a7641d621787525ad336cbe69/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[delete] https://crrev.com/19ad77688a08480a7641d621787525ad336cbe69/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[delete] https://crrev.com/19ad77688a08480a7641d621787525ad336cbe69/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/8b3f5df1a65187e662fcf2fd5ead1ccb89226f31/third_party/WebKit/Source/core/loader/ImageLoader.cpp
[modify] https://crrev.com/8b3f5df1a65187e662fcf2fd5ead1ccb89226f31/third_party/WebKit/Source/core/loader/ImageLoader.h

The fix was reverted due to leak (Issue 640886).
Probably the CL makes ImageLoader::m_hasPendingLoadEvent true and thus might prevent something from being garbage collected.
Particularly, ImageLoader should be notified when the loading of the image document is cancelled, but it seems not.
Cc: ssamanoori@chromium.org
Issue 635215 has been merged into this issue.
Issue 645247 has been merged into this issue.
Project Member Comment 12 by bugdroid1@chromium.org, Sep 22
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c5e43f7edf4e55155fcda51be0285f37b20c2e76

commit c5e43f7edf4e55155fcda51be0285f37b20c2e76
Author: hiroshige <hiroshige@chromium.org>
Date: Thu Sep 22 19:52:41 2016

HTMLImageElement: do not use fallback content for ImageDocument

After https://codereview.chromium.org/1879793003,
ImageLoader::hasPendingActivity() became false for ImageDocument and thus
HTMLImageElement::selectSourceURL() didn't consider the image as still loading
during load (|imageStillLoading| became false).
This caused ImageDocument not to be displayed progressively.

This CL makes HTMLImageElement to use primary content for ImageDocument.
This causes ImageDocument not to be replaced with fallback content, but I
expect that is more acceptable than images not displayed progressively.

BUG= 632495 

Review-Url: https://codereview.chromium.org/2343613002
Cr-Commit-Position: refs/heads/master@{#420432}

[add] https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[add] https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[add] https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76/third_party/WebKit/Source/core/loader/ImageLoader.h

Project Member Comment 13 by bugdroid1@chromium.org, Sep 22
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/70e44d10fc52150a6af5a7de760cb9181587fbb4

commit 70e44d10fc52150a6af5a7de760cb9181587fbb4
Author: gcasto <gcasto@chromium.org>
Date: Thu Sep 22 23:27:34 2016

Revert of HTMLImageElement: do not use fallback content for ImageDocument (patchset #2 id:20001 of https://codereview.chromium.org/2343613002/ )

Reason for revert:
Speculatively reverting to fix broken layout tests on Windows (https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win10, https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7)

Tests are

fast/backgrounds/border-radius-split-background-image.html
fast/backgrounds/border-radius-split-background.html
fast/borders/border-styles-split.html
paint/invalidation/background-resize-height.html

Original issue's description:
> HTMLImageElement: do not use fallback content for ImageDocument
>
> After https://codereview.chromium.org/1879793003,
> ImageLoader::hasPendingActivity() became false for ImageDocument and thus
> HTMLImageElement::selectSourceURL() didn't consider the image as still loading
> during load (|imageStillLoading| became false).
> This caused ImageDocument not to be displayed progressively.
>
> This CL makes HTMLImageElement to use primary content for ImageDocument.
> This causes ImageDocument not to be replaced with fallback content, but I
> expect that is more acceptable than images not displayed progressively.
>
> BUG= 632495 
>
> Committed: https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76
> Cr-Commit-Position: refs/heads/master@{#420432}

TBR=yhirano@chromium.org,japhet@chromium.org,ellyjones@chromium.org,hiroshige@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 632495 

Review-Url: https://codereview.chromium.org/2365523003
Cr-Commit-Position: refs/heads/master@{#420499}

[delete] https://crrev.com/04b1826356a37de2a322e7e6e0e80f07d05f210f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[delete] https://crrev.com/04b1826356a37de2a322e7e6e0e80f07d05f210f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[delete] https://crrev.com/04b1826356a37de2a322e7e6e0e80f07d05f210f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/70e44d10fc52150a6af5a7de760cb9181587fbb4/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/70e44d10fc52150a6af5a7de760cb9181587fbb4/third_party/WebKit/Source/core/loader/ImageLoader.h

Project Member Comment 14 by bugdroid1@chromium.org, Sep 23
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/cc960a01f9c5d39ead364b1cb929c999323d3dc6

commit cc960a01f9c5d39ead364b1cb929c999323d3dc6
Author: hiroshige <hiroshige@chromium.org>
Date: Fri Sep 23 15:48:08 2016

Reland of HTMLImageElement: do not use fallback content for ImageDocument (patchset #1 id:1 of https://codereview.chromium.org/2365523003/ )

Reason for revert:
As per comment #8, this CL wasn't the cause of bot failures.
https://codereview.chromium.org/2365523003/#msg8

Original issue's description:
> Revert of HTMLImageElement: do not use fallback content for ImageDocument (patchset #2 id:20001 of https://codereview.chromium.org/2343613002/ )
>
> Reason for revert:
> Speculatively reverting to fix broken layout tests on Windows (https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win10, https://uberchromegw.corp.google.com/i/chromium.webkit/builders/WebKit%20Win7)
>
> Tests are
>
> fast/backgrounds/border-radius-split-background-image.html
> fast/backgrounds/border-radius-split-background.html
> fast/borders/border-styles-split.html
> paint/invalidation/background-resize-height.html
>
> Original issue's description:
> > HTMLImageElement: do not use fallback content for ImageDocument
> >
> > After https://codereview.chromium.org/1879793003,
> > ImageLoader::hasPendingActivity() became false for ImageDocument and thus
> > HTMLImageElement::selectSourceURL() didn't consider the image as still loading
> > during load (|imageStillLoading| became false).
> > This caused ImageDocument not to be displayed progressively.
> >
> > This CL makes HTMLImageElement to use primary content for ImageDocument.
> > This causes ImageDocument not to be replaced with fallback content, but I
> > expect that is more acceptable than images not displayed progressively.
> >
> > BUG= 632495 
> >
> > Committed: https://crrev.com/c5e43f7edf4e55155fcda51be0285f37b20c2e76
> > Cr-Commit-Position: refs/heads/master@{#420432}
>
> TBR=yhirano@chromium.org,japhet@chromium.org,ellyjones@chromium.org,hiroshige@chromium.org
> # Skipping CQ checks because original CL landed less than 1 days ago.
> NOPRESUBMIT=true
> NOTREECHECKS=true
> NOTRY=true
> BUG= 632495 
>
> Committed: https://crrev.com/70e44d10fc52150a6af5a7de760cb9181587fbb4
> Cr-Commit-Position: refs/heads/master@{#420499}

TBR=yhirano@chromium.org,japhet@chromium.org,ellyjones@chromium.org,gcasto@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG= 632495 

Review-Url: https://codereview.chromium.org/2364073002
Cr-Commit-Position: refs/heads/master@{#420616}

[add] https://crrev.com/cc960a01f9c5d39ead364b1cb929c999323d3dc6/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[add] https://crrev.com/cc960a01f9c5d39ead364b1cb929c999323d3dc6/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[add] https://crrev.com/cc960a01f9c5d39ead364b1cb929c999323d3dc6/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/cc960a01f9c5d39ead364b1cb929c999323d3dc6/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/cc960a01f9c5d39ead364b1cb929c999323d3dc6/third_party/WebKit/Source/core/loader/ImageLoader.h

Labels: Merge-Request-54
The fix (#12, relanded #14) was landed on 55.0.2870.0 and I confirmed the large image (Comment #0) was shown progressively on 55.0.2871.0 on Windows 7.

Requesting merge to M-54 beta (the fix stayed on canary for > one day).
Labels: -Merge-Request-54 Merge-Approved-54 Hotlist-Merge-Approved
Your change meets the bar and is auto-approved for M54 (branch: 2840)
Project Member Comment 17 by bugdroid1@chromium.org, Sep 26
Labels: -merge-approved-54 merge-merged-2840
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f

commit 01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Mon Sep 26 05:12:36 2016

HTMLImageElement: do not use fallback content for ImageDocument

After https://codereview.chromium.org/1879793003,
ImageLoader::hasPendingActivity() became false for ImageDocument and thus
HTMLImageElement::selectSourceURL() didn't consider the image as still loading
during load (|imageStillLoading| became false).
This caused ImageDocument not to be displayed progressively.

This CL makes HTMLImageElement to use primary content for ImageDocument.
This causes ImageDocument not to be replaced with fallback content, but I
expect that is more acceptable than images not displayed progressively.

BUG= 632495 

Review-Url: https://codereview.chromium.org/2343613002
Cr-Commit-Position: refs/heads/master@{#420432}
(cherry picked from commit c5e43f7edf4e55155fcda51be0285f37b20c2e76)

Review URL: https://codereview.chromium.org/2371643002 .

Cr-Commit-Position: refs/branch-heads/2840@{#522}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[add] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[add] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[add] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/Source/core/loader/ImageLoader.h

Labels: TE-Verified-54.0.2840.41 TE-Verified-M54
Verified the issue on Mac 10.11.6, Win 10 and Ubuntu 14.04 using Beta # 54.0.2840.41 and its working fine now.
Please find the attached screen shot for the same.

hiroshige@ : Please mark the issue to Fixed if there is no further work to be done on it.
632495_Sept_28.mp4
1.3 MB View Download
Labels: Merge-Request-53
Requesting merge to M-53 if there was a further M-53 stable update.
If the next stable update is M-54 then we don't need merge.
Labels: -Merge-Request-53 Merge-Review-53 Hotlist-Merge-Review
[Automated comment] Request affecting a post-stable build (M53), manual review required.
Labels: -Merge-Review-53
Status: Fixed
Dropping the request as Comment #20 says M-53 is post-stable.

Closing as fixed.
Labels: -Hotlist-Merge-review
Project Member Comment 23 by bugdroid1@chromium.org, Oct 27
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f

commit 01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f
Author: Hiroshige Hayashizaki <hiroshige@chromium.org>
Date: Mon Sep 26 05:12:36 2016

HTMLImageElement: do not use fallback content for ImageDocument

After https://codereview.chromium.org/1879793003,
ImageLoader::hasPendingActivity() became false for ImageDocument and thus
HTMLImageElement::selectSourceURL() didn't consider the image as still loading
during load (|imageStillLoading| became false).
This caused ImageDocument not to be displayed progressively.

This CL makes HTMLImageElement to use primary content for ImageDocument.
This causes ImageDocument not to be replaced with fallback content, but I
expect that is more acceptable than images not displayed progressively.

BUG= 632495 

Review-Url: https://codereview.chromium.org/2343613002
Cr-Commit-Position: refs/heads/master@{#420432}
(cherry picked from commit c5e43f7edf4e55155fcda51be0285f37b20c2e76)

Review URL: https://codereview.chromium.org/2371643002 .

Cr-Commit-Position: refs/branch-heads/2840@{#522}
Cr-Branched-From: 1ae106dbab4bddd85132d5b75c670794311f4c57-refs/heads/master@{#414607}

[add] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.png
[add] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document-expected.txt
[add] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/LayoutTests/http/tests/images/png-partial-load-as-document.html
[modify] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/Source/core/html/HTMLImageElement.cpp
[modify] https://crrev.com/01e1c1571cd16ec4f3ff0342f8f1e0f943cd942f/third_party/WebKit/Source/core/loader/ImageLoader.h

Sign in to add a comment