Bug 22521 – REGRESSION (r31287): loading a CachedImage now sends a didFinishLoading delegate message

• Component: Page Loading
• Status: RESOLVED FIXED

[Cameron Zwarich (cpst)]

Before r31287, loading a CachedImage via CachedResource::load(DocLoader*) did not send a delegate message when it reached FrameLoader::loadedResourceFromMemoryCache(), because there is an early return path:

    if (!resource->sendResourceLoadCallbacks() || haveToldClientAboutLoad(resource->url()))
        return;

Since resource->sendResourceLoadCallbacks() was false, this would return before sending the delegate message. After r31287, CachedImage::load(DocLoader*) sets m_sendResourceLoadCallbacks to true, avoiding this early return and causing the delegate message to be sent.

This causes problems with WebKit clients that do not expect these delegate messages, e.g. <rdar://problem/6007746>.

[Cameron Zwarich (cpst)]

Antti, you were asking why m_sendResourceLoadCallbacks was ever false in the old code. It used to be the default of the optional value sendResourceLoadCallbacks in the CachedResource constructor. This default value was implicitly used by the CachedImage, CachedScript, CachedFont, CachedXSLStyleSheet, and CachedXBLDocument constructors, because didn't pass a value for it. There was no other other code that set m_sendResourceLoadCallbacks, so this default value was used later.

After r32187, m_sendResourceLoadCallbacks is always set to true on object construction, but it is also set to true later by the various calls to the non-virtual CachedResource::load() member function.

Is there ever any time that we want a value of m_sendResourceLoadCallbacks that is different from one that we could know at the time we create the CachedResource? It seems like it would make more sense to have it be immutable and set during object initialization.

[Cameron Zwarich (cpst)]

I think some confusion was caused by the poor interaction between the default values of four optional parameters.

Before r32187, the CachedResource constructor had a default value of false for m_sendResourceLoadCallbacks. This default was only ever overloaded for CachedCSSStyleSheet. The createResource() function in Cache.cpp also had a sendResourceLoadCallbacks parameter with a default value of true, but this was only used in the CachedCSSStyleSheet case. It made no sense for createResource() to have a default parameter in the first place, since it had one caller in the same file, and this caller, Cache::requestResource(), always supplied the value:

        resource = createResource(type, docLoader, url, charset, skipCanLoadCheck, sendResourceLoadCallbacks);

The sendResourceLoadCallbacks parameter of Cache::requestResource() was also optional, with a default of true. The caller of Cache::requestResource() was DocLoader::requestResource(), which also had an optional sendResourceLoadCallbacks parameter with a default value of true. The only place that overrode this value was DocLoader::requestCSSStyleSheet(), which would pass !isUserStyleSheet as sendResourceLoadCallbacks.

The end result of the old code was that CachedResource always had m_sendResourceLoadCallbacks set to false, except in the case when it was a stylesheet that was not the user stylesheet.

Most of these changes were originally made in r23741:

http://trac.webkit.org/changeset/23741

By now, the original intentions of that patch are a bit moot (maybe the intent was to send resource load callbacks in more cases?), but we have a compatibility problem due to the move away from that behaviour.

I propose moving to codifying the old behaviour before r32187 in a simpler way, as an immutable value set at CachedResource construction. This is possible because it is always false, except in the case of a non-user stylesheet. We also always know at the creation of a CachedCSSStylesheet whether the object is for a user stylesheet or not.

[Cameron Zwarich (cpst)]

This bug gets even more complicated. While it is true that the default value of CachedResource::m_sendResourceLoadCallbacks was false before r31287, the call to Loader::load() in the CachedResource subclass constructors was generally relying on the default true value of the sendResourceLoadCallback parameter of Loader::load(). This means that the initial Request object was always being created with a true value of Request::m_sendResourceLoadCallbacks.

This causes some things to happen in the WebKit layer during the initial handling of the Request. However, when control reached FrameLoader::loadedResourceFromMemoryCache(), it checked the m_sendResourceLoadCallbacks property of the CachedResource, which was false.

I mention this because my naive attempt to implement my fix was leading to some assertion failures on loader tests due to using false for the first value as well. I think I now know how to get the Safari 3.1 behaviour with a patch to ToT, but this loader code is really starting to scare me. These two things called m_sendResourceLoadCallback should probably have different names, since they obviously do two different things. I don't think the one on Request actually ever causes a delegate message to be sent, at least in normal situations.

Antti and Anders, is my explanation of the issues clear enough? Should I attempt a behaviour fix before a cleanup? You guys know this code better than me, but I've been trying pretty hard to fix this bug, so I think I have picked it up fairly well.

[Cameron Zwarich (cpst)]

Created attachment 25559 [details]
Patch in progress

This patch fixes the bug by restoring Safari 3.1 behaviour, except in the case of stylesheets. The situation for stylesheets is as follows:

Non-user stylesheets have a true CachedCSSStyleSheet::m_sendResourceLoadCallbacks value and pass true to Loader::load()

User stylesheets have a false CachedCSSStyleSheet::m_sendResourceLoadCallbacks value and pass false to Loader::load()

Everything else had a false CachedCSSStyleSheet::m_sendResourceLoadCallbacks value and passed true to Loader::load().

I am not entirely sure what is correct here -- perhaps user stylesheets should also pass true to Loader::load(), or non-user stylesheets should not send callbacks as well. That would lead to unified behaviour for all cached resources, but it might raise other compatibility issues. Some clients may rely on receiving a CachedCSSStyleSheet delegate message, even though they don't rely on it for images or anything else.

[Cameron Zwarich (cpst)]

I forgot to mention: this patch passes all tests. I can also copy Safari 3.1's behaviour for user stylesheets pretty easily, but I want to get your feedback first.

[Antti Koivisto]

With this patch isn't m_sendResourceLoadCallbacks always false? If so, why have the field in the first place? Can we safely eliminate the latter half of the FrameLoader::loadedResourceFromMemoryCache() too?

[Cameron Zwarich (cpst)]

(In reply to comment #6)
> With this patch isn't m_sendResourceLoadCallbacks always false? If so, why have
> the field in the first place?

I left it there even though it is always false, because if we want to exactly copy Safari 3.1's behaviour, we need to make it true for non-user stylesheets (and nothing else).

> Can we safely eliminate the latter half of the
> FrameLoader::loadedResourceFromMemoryCache() too?

I think a similar answer applies here.

[Cameron Zwarich (cpst)]

The correct solution might be to schedule these delegate callbacks to be sent asynchronously via  zero-delay timer, but that is not as easy as it sounds, because the CachedResource may be freed before the timer executes and there is no normal refcounting of CachedResources. Antti, do you know a clean way of doing this?

[Cameron Zwarich (cpst)]

Comment on attachment 25559 [details]
Patch in progress

This patch is obseleted by r41071, which adds a workaround for iChat:

http://trac.webkit.org/changeset/41071

[Cameron Zwarich (cpst)]

This bug can now be closed. Antti and I concluded that the current WebKit behaviour is correct, despite it causing an iChat crash. Perhaps it is worth making a bug about the iChat behaviour causing a crash. The loader callbacks should probably not be delivered synchronously from layout, but they also shouldn't be delivered from a timer.