New issue
Advanced search Search tips
Note: Color blocks (like █ or █) mean that a user may not be available. Tooltip shows the reason.

Issue 901552 link

Starred by 1 user
Status: Fixed
Owner:
alancutter@chromium.org
Closed: Nov 26
Cc:
█ ortuno@chromium.org
dominickn@chromium.org
█ bgalbs@google.com
pcovell@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

DCHECK reachable in WebAppIconDownloader::DidDownloadFavicon

Project Member Reported by reillyg@chromium.org, Nov 2 
Chrome Version: HEAD
OS: Windows

What steps will reproduce the problem?
(1) Build Chromium with DCHECKs enabled
(2) Launch Chromium
(3) Sign into a profile with lots of sync data

What is the expected result?
No crashes.

What happens instead?
Chromium hits this DCHECK in WebAppIconDownloader::DidDownloadFavicon():

  DCHECK_LE(100, http_status_code);

When the DCHECK is hit |http_status_code| is equal to 0.

Comment 1 by ortuno@chromium.org, Nov 4

Owner: alancutter@chromium.org
Status: Assigned (was: Untriaged)
It seems that ImageDownloaderBase::DownloadImage() returns 0 for the HTTP Status code if the URL is a Data URL. I'm not sure if that's the right thing to do but we should at least fix our DCHECK.

Alan, could you take a look since you added the DCHECK?

Comment 2 by ortuno@chromium.org, Nov 4 

Also, thanks for the report!

Comment 3 by alancutter@chromium.org, Nov 5

Labels: -OS-Windows
The DCHECKs are correct, they caught a bug.
Project Member

Comment 4 by bugdroid1@chromium.org, Nov 26 

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

commit 1058b14496da2a10ea36713bbef31ca7d1779242
Author: Alan Cutter <alancutter@chromium.org>
Date: Mon Nov 26 07:26:42 2018

Fix bookmark icon download status logging to account for data: URLs

This CL makes Extensions.BookmarkApp.Icon.HttpStatusCodeClassOnSync and
Extensions.BookmarkApp.Icon.HttpStatusCodeClassOnCreate ignore data: URLs
which have no HTTP status code.

Bug:  901552 
Change-Id: I5a7ec6f9300c0b91668e9994b769073a638aff5b
Reviewed-on: https://chromium-review.googlesource.com/c/1317213
Commit-Queue: Alan Cutter <alancutter@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Cr-Commit-Position: refs/heads/master@{#610777}
[modify] https://crrev.com/1058b14496da2a10ea36713bbef31ca7d1779242/chrome/browser/web_applications/components/web_app_icon_downloader.cc
[modify] https://crrev.com/1058b14496da2a10ea36713bbef31ca7d1779242/chrome/browser/web_applications/components/web_app_icon_downloader_unittest.cc
[modify] https://crrev.com/1058b14496da2a10ea36713bbef31ca7d1779242/content/common/image_downloader/image_downloader.mojom
[modify] https://crrev.com/1058b14496da2a10ea36713bbef31ca7d1779242/content/public/browser/web_contents.h

Comment 5 by alancutter@chromium.org, Nov 26

Status: Fixed (was: Assigned)
►

Sign in to add a comment