Bug 173478 – REGRESSION (r218015) IconLoaders for already-cached resources expect to be asynchronous, no longer are.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Brady Eidson]

REGRESSION (r218015) IconLoaders for an already cached resource no longer load asynchronously.

[Brady Eidson]

Inside bool CachedResource::addClientToSet(CachedResourceClient& client), the check "m_type == RawResource || m_type == MainResource" needed to be updated to include Favicon type.

[Brady Eidson]

But, actually, I think maybe we want this new behavior - But it is currently causing an ASSERT in debug builds.

[Brady Eidson]

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()

[Daniel Bates]

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?

[Brady Eidson]

Created attachment 313102 [details]
Patch

[Brady Eidson]

(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.

[Daniel Bates]

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.

[Brady Eidson]

(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.

[Brady Eidson]

Created attachment 313112 [details]
Patch

[Daniel Bates]

(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>.

[WebKit Commit Bot]

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

[Brady Eidson]

Created attachment 313115 [details]
Patch

[Brady Eidson]

(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.

[WebKit Commit Bot]

Comment on attachment 313115 [details]
Patch

Clearing flags on attachment: 313115

Committed r218409: <http://trac.webkit.org/changeset/218409>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.