Bug 173478 - REGRESSION (r218015) IconLoaders for already-cached resources expect to be asynchronous, no longer are.
Summary: REGRESSION (r218015) IconLoaders for already-cached resources expect to be as...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Brady Eidson
URL:
Keywords: Regression
Depends on:
Blocks:
 
Reported: 2017-06-16 10:03 PDT by Brady Eidson
Modified: 2017-06-16 13:34 PDT (History)
6 users (show)

See Also:


Attachments
Patch (8.07 KB, patch)
2017-06-16 11:41 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2017-06-16 12:40 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2017-06-16 12:55 PDT, Brady Eidson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2017-06-16 10:03:27 PDT
REGRESSION (r218015) IconLoaders for an already cached resource no longer load asynchronously.
Comment 1 Brady Eidson 2017-06-16 10:04:50 PDT
Inside bool CachedResource::addClientToSet(CachedResourceClient& client), the check "m_type == RawResource || m_type == MainResource" needed to be updated to include Favicon type.
Comment 2 Brady Eidson 2017-06-16 10:05:24 PDT
But, actually, I think maybe we want this new behavior - But it is currently causing an ASSERT in debug builds.
Comment 3 Brady Eidson 2017-06-16 10:11:05 PDT
ASSERTION FAILED: loadIdentifier
/Volumes/Data/git/OpenSource/Source/WebCore/loader/DocumentLoader.cpp(1691) : void WebCore::DocumentLoader::finishedLoadingIcon(WebCore::IconLoader &, WebCore::SharedBuffer *)
Jun 16 10:10:48  MiniBrowser[29028] <Error>: CGImageCreateWithImageProvider: invalid image size: 0 x 0.
1   0x1069cb51d WTFCrash
2   0x10e90d75d WebCore::DocumentLoader::finishedLoadingIcon(WebCore::IconLoader&, WebCore::SharedBuffer*)
3   0x10ef8e0a8 WebCore::IconLoader::notifyFinished(WebCore::CachedResource&)
4   0x10e39b6d1 WebCore::CachedResource::didAddClient(WebCore::CachedResourceClient&)
5   0x10e394922 WebCore::CachedRawResource::didAddClient(WebCore::CachedResourceClient&)
6   0x10e39b180 WebCore::CachedResource::addClient(WebCore::CachedResourceClient&)
7   0x10ef8de2d WebCore::IconLoader::startLoading()
Comment 4 Daniel Bates 2017-06-16 11:12:25 PDT
Is there a straightforward way to enable the icon database in MiniBrowser? I take it you just visited a web site that has a favicon to hit this once the icon database is enabled?
Comment 5 Brady Eidson 2017-06-16 11:41:46 PDT
Created attachment 313102 [details]
Patch
Comment 6 Brady Eidson 2017-06-16 11:43:31 PDT
(In reply to Daniel Bates from comment #4)
> Is there a straightforward way to enable the icon database in MiniBrowser? I
> take it you just visited a web site that has a favicon to hit this once the
> icon database is enabled?

This has nothing to do with the icon database.

Minibrowser has a check box to turn on the icon loading delegate for all icons.
Comment 7 Daniel Bates 2017-06-16 12:28:48 PDT
Comment on attachment 313102 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=313102&action=review

> Source/WebCore/ChangeLog:10
> +        Being synchronous is actually better, because it's actually resolved another issue or two.

This sentence does not read well. In particular, the phrase "it's actually resolved".

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:160
> +static const char mainBytes2[] =
> +"Oh, hello there!";

Minor: It is unnecessary to use the keyword static. In C++ and Objective-C++ a const has internal linkage. I also do not see the need to spread the assignment across two lines (we are not using VT100 terminals these days) :). I suggest writing the assignment on one line.

> Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:190
> +    // Load another main resource that results in the same icon being loaded (which should come from the memory cache)

Minor: Missing period at the end of this sentence.
Comment 8 Brady Eidson 2017-06-16 12:39:03 PDT
(In reply to Daniel Bates from comment #7)
> 
> > Tools/TestWebKitAPI/Tests/WebKit2Cocoa/IconLoadingDelegate.mm:160
> > +static const char mainBytes2[] =
> > +"Oh, hello there!";
> 
> Minor: It is unnecessary to use the keyword static. In C++ and Objective-C++
> a const has internal linkage. I also do not see the need to spread the
> assignment across two lines (we are not using VT100 terminals these days)
> :). I suggest writing the assignment on one line.

It's established style for custom protocol html resources in API tests to make them multiline, including in this same file. I'm going to leave it set up for that.
Comment 9 Brady Eidson 2017-06-16 12:40:06 PDT
Created attachment 313112 [details]
Patch
Comment 10 Daniel Bates 2017-06-16 12:41:20 PDT
(In reply to Brady Eidson from comment #6)
> (In reply to Daniel Bates from comment #4)
> > Is there a straightforward way to enable the icon database in MiniBrowser? I
> > take it you just visited a web site that has a favicon to hit this once the
> > icon database is enabled?
> 
> This has nothing to do with the icon database.
> 
> Minibrowser has a check box to turn on the icon loading delegate for all
> icons.

Where is this checkbox? I only see Settings > Load All Site Icons Per Page. When I enable this option and load apple.com (it has a favicon) in either a Legacy WebKit or WebKit window I am not able hit this assertion failure in a debug build of WebKit at r218201. From briefly debugging, Document calls IconController::startLoader(), <https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.cpp?rev=218395#L2688>, which early returns because IconDatabase::isEnabled() returns false at <https://trac.webkit.org/browser/trunk/Source/WebCore/loader/icon/IconController.cpp?rev=200591#L134>.
Comment 11 WebKit Commit Bot 2017-06-16 12:41:33 PDT
Comment on attachment 313112 [details]
Patch

Rejecting attachment 313112 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 313112, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in Source/WebCore/ChangeLog contains OOPS!.

Full output: http://webkit-queues.webkit.org/results/3943214
Comment 12 Brady Eidson 2017-06-16 12:55:21 PDT
Created attachment 313115 [details]
Patch
Comment 13 Brady Eidson 2017-06-16 12:57:12 PDT
(In reply to Daniel Bates from comment #10)
> (In reply to Brady Eidson from comment #6)
> > (In reply to Daniel Bates from comment #4)
> > > Is there a straightforward way to enable the icon database in MiniBrowser? I
> > > take it you just visited a web site that has a favicon to hit this once the
> > > icon database is enabled?
> > 
> > This has nothing to do with the icon database.
> > 
> > Minibrowser has a check box to turn on the icon loading delegate for all
> > icons.
> 
> Where is this checkbox? I only see Settings > Load All Site Icons Per Page.
> When I enable this option and load apple.com (it has a favicon) in either a
> Legacy WebKit or WebKit window I am not able hit this assertion failure in a
> debug build of WebKit at r218201. From briefly debugging, Document calls
> IconController::startLoader(),
> <https://trac.webkit.org/browser/trunk/Source/WebCore/dom/Document.
> cpp?rev=218395#L2688>, which early returns because IconDatabase::isEnabled()
> returns false at
> <https://trac.webkit.org/browser/trunk/Source/WebCore/loader/icon/
> IconController.cpp?rev=200591#L134>.

It's probably unnecessary for you to spend time debugging a solved issue.

That said, that is the correct check box, but you're not actually testing the bug - You have to get the icon into the memory cache through a successful page load, and then re-access it from the memory cache on the second load.
Comment 14 WebKit Commit Bot 2017-06-16 13:34:51 PDT
Comment on attachment 313115 [details]
Patch

Clearing flags on attachment: 313115

Committed r218409: <http://trac.webkit.org/changeset/218409>
Comment 15 WebKit Commit Bot 2017-06-16 13:34:53 PDT
All reviewed patches have been landed.  Closing bug.