Bug 170697 – CachedImage should cancel loading images for unsupported/unknown types

• Status: RESOLVED
• Resolution: FIXED
• Component: Images

[Said Abou-Hallawa]

To save CPU cycles and network bandwidth, it is worth cancelling loading images with unsupported/unknown types.

[Radar WebKit Bug Importer]

<rdar://problem/31543168>

[Said Abou-Hallawa]

Created attachment 306771 [details]
Patch

[Build Bot]

Comment on attachment 306771 [details]
Patch

Attachment 306771 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3514938

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 306775 [details]
Archive of layout-test-results from ews107 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews107  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 306771 [details]
Patch

Attachment 306771 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/3514943

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 306778 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Created attachment 306825 [details]
Patch

[Said Abou-Hallawa]

Created attachment 306839 [details]
Patch

[youenn fablet]

Comment on attachment 306839 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.cpp:408
> +            m_loader->cancel();

Should we set m_loader to nullptr?
It is sufficient to set m_loader to nullptr?

Ideally, CachedImage should notify its clients that something went wrong and all clients should clean themselves, and CachedImage should be destroyed, thus cancelling the load. This approach ensures the load gets cancelled so this is probably good enough.

> Source/WebCore/loader/cache/CachedImage.cpp:486
> +void CachedImage::cancelLoad()

Is it needed to do the clean-up once m_loader->cancel() is called.
If it is the case, can we do the clean-up up there?
If not, what does it bring?

> Source/WebCore/loader/cache/CachedImage.h:112
> +    void cancelLoad() override;

I am not sure we want that.
This adds some difficulty in understanding and CachedImage::cancelLoad seems very similar to CachedImage::cancelLoad.
Using an existing mechanism might be ok like checkNotify() for instance which is already virtual.
Would that be possible?

[youenn fablet]

(In reply to youenn fablet from comment #9)
> Comment on attachment 306839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306839&action=review
> 
> > Source/WebCore/loader/cache/CachedImage.cpp:408
> > +            m_loader->cancel();
> 
> Should we set m_loader to nullptr?
> It is sufficient to set m_loader to nullptr?
> 
> Ideally, CachedImage should notify its clients that something went wrong and
> all clients should clean themselves, and CachedImage should be destroyed,
> thus cancelling the load. This approach ensures the load gets cancelled so
> this is probably good enough.

'This approach' means the approach you are proposing in this patch.

[Said Abou-Hallawa]

Comment on attachment 306839 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedImage.cpp:486
>> +void CachedImage::cancelLoad()
> 
> Is it needed to do the clean-up once m_loader->cancel() is called.
> If it is the case, can we do the clean-up up there?
> If not, what does it bring?

CachedImage::cancelLoad() gets called form  ResourceLoader::cancel() via SubresourceLoader::didCancel(). It marks the image as a broken image.  Without adding CachedImage::cancelLoad(), nothing will be drawn for the unspotted image not even the ? mark for the broken image. The code below is very similar to CachedImage::error().

>> Source/WebCore/loader/cache/CachedImage.h:112
>> +    void cancelLoad() override;
> 
> I am not sure we want that.
> This adds some difficulty in understanding and CachedImage::cancelLoad seems very similar to CachedImage::cancelLoad.
> Using an existing mechanism might be ok like checkNotify() for instance which is already virtual.
> Would that be possible?

Did you mean: "CachedImage::cancelLoad seems very similar to CachedImage::error" above?

[youenn fablet]

(In reply to Said Abou-Hallawa from comment #11)
> Comment on attachment 306839 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=306839&action=review
> 
> >> Source/WebCore/loader/cache/CachedImage.cpp:486
> >> +void CachedImage::cancelLoad()
> > 
> > Is it needed to do the clean-up once m_loader->cancel() is called.
> > If it is the case, can we do the clean-up up there?
> > If not, what does it bring?
> 
> CachedImage::cancelLoad() gets called form  ResourceLoader::cancel() via
> SubresourceLoader::didCancel(). It marks the image as a broken image. 
> Without adding CachedImage::cancelLoad(), nothing will be drawn for the
> unspotted image not even the ? mark for the broken image. The code below is
> very similar to CachedImage::error().

If this code is triggered only in the case CachedImage is calling m_loader->canvel(), it might be better to do that at the call site, not here.
> 
> >> Source/WebCore/loader/cache/CachedImage.h:112
> >> +    void cancelLoad() override;
> > 
> > I am not sure we want that.
> > This adds some difficulty in understanding and CachedImage::cancelLoad seems very similar to CachedImage::cancelLoad.
> > Using an existing mechanism might be ok like checkNotify() for instance which is already virtual.
> > Would that be possible?
> 
> Did you mean: "CachedImage::cancelLoad seems very similar to
> CachedImage::error" above?

I was meaning CachedResource::cancelLoad. Would be good if we could not change cacelLoad at all.

[Said Abou-Hallawa]

Comment on attachment 306839 [details]
Patch

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

>>>> Source/WebCore/loader/cache/CachedImage.cpp:486
>>>> +void CachedImage::cancelLoad()
>>> 
>>> Is it needed to do the clean-up once m_loader->cancel() is called.
>>> If it is the case, can we do the clean-up up there?
>>> If not, what does it bring?
>> 
>> CachedImage::cancelLoad() gets called form  ResourceLoader::cancel() via SubresourceLoader::didCancel(). It marks the image as a broken image.  Without adding CachedImage::cancelLoad(), nothing will be drawn for the unspotted image not even the ? mark for the broken image. The code below is very similar to CachedImage::error().
> 
> If this code is triggered only in the case CachedImage is calling m_loader->canvel(), it might be better to do that at the call site, not here.

Why do you think it might be better to move the code of this function to the call site?

I looked at the CachedResource class and i noticed it has these two function which are very similar.

void CachedResource::error(CachedResource::Status status)
{
    setStatus(status);
    ASSERT(errorOccurred());
    m_data = nullptr;

    setLoading(false);
    checkNotify();
}
    
void CachedResource::cancelLoad()
{
    if (!isLoading() && !stillNeedsLoad())
        return;

    setStatus(LoadError);
    setLoading(false);
    checkNotify();
}

CachedImage::error() calls CachedResource::error() in the following order. I am not sure if the order of the calls matters or not. But I think it matters because CachedResource::checkNotify() notifies the clients that the image has finished loading while CachedImage::notifyObservers() notifies the observers that they need to repaint themselves.

void CachedImage::error(CachedResource::Status status)
{
    checkShouldPaintBrokenImage();
    clear();
    CachedResource::error(status);
    notifyObservers();
}

I also wanted to do cancelling the image load to be very similar to the case of error except to stops loading data into the image. So I added the function CachedImage::cancelLoad() which is very similar to CachedImage::error():

void CachedImage::cancelLoad()
{
    checkShouldPaintBrokenImage();
    clear();
    CachedResource::cancelLoad();
    notifyObservers();
}

Now how do I call this function? Since ResourceLoader::cancel() currently calls CachedResource::cancelLoad() though SubresourceLoader::didCancel(), I just made CachedResource::cancelLoad() virtual and I called ResourceLoader::cancel() from CachedImage::addIncrementalDataBuffer() in the case of error/unsupported image format. This will make the sequence and the oder of calls for cancelling the load be the same as for the error case.

So unless you are sure the way I organized this code can cause problems, I think this order of calls is safer to use since it has been used for the error case without known issues. And also it makes the code more consistent.

[youenn fablet]

Do you know whether checkNotify/notifyObservers order is important or not?
In case of regular loading end, checkNotify is done after notifyObservers.

I think your patch is working fine.
It is more a matter of clarity of the code.
If the call to checkShouldPaintBrokenImage/clear/... is done when checking for EncodedDataStatus::Error, the intent is very clear (paint a broken image and cancel the network load).

If we follow the error() approach, it is less easy to follow the code flow.
For instance, is the error() approach done so that we actually ensure that checkNotify is called before notifyObservers for instance? Or is it purely a style artifact?

Maybe we need to follow the error() approach for the reason you mentioned.
If not, why not trying to simplify things. A little refactoring would also help group code common to CachedImage::error() and your proposed CachedImage::cancelLoad() for instance.

As a minor thing, the cancelLoad approach is slightly less efficient: we are doing twice the checks of "!isLoading() && !stillNeedsLoad()", plus the cost of the virtual cancelLoad.

[Said Abou-Hallawa]

(In reply to youenn fablet from comment #14)
> Do you know whether checkNotify/notifyObservers order is important or not?
> In case of regular loading end, checkNotify is done after notifyObservers.
> 

I answered this question above: "I am not sure if the order of the calls matters or not. But I think it matters because CachedResource::checkNotify() notifies the clients that the image has finished loading while CachedImage::notifyObservers() notifies the observers that they need to repaint themselves."

> I think your patch is working fine.
> It is more a matter of clarity of the code.
> If the call to checkShouldPaintBrokenImage/clear/... is done when checking
> for EncodedDataStatus::Error, the intent is very clear (paint a broken image
> and cancel the network load).
> 

I can log a new bug for the code clarity issue "Investigate the order of calls made by CachedImage::error and CachedImage::cancelLoad()".

> If we follow the error() approach, it is less easy to follow the code flow.
> For instance, is the error() approach done so that we actually ensure that
> checkNotify is called before notifyObservers for instance? Or is it purely a
> style artifact?
> 

I don't know for sure but I think the current order of calls is correct.

> Maybe we need to follow the error() approach for the reason you mentioned.
> If not, why not trying to simplify things. A little refactoring would also
> help group code common to CachedImage::error() and your proposed
> CachedImage::cancelLoad() for instance.
> 

This will be covered by the new bug.

> As a minor thing, the cancelLoad approach is slightly less efficient: we are
> doing twice the checks of "!isLoading() && !stillNeedsLoad()", plus the cost
> of the virtual cancelLoad.

I would have agreed with this analysis if this code were in a critical code path. But this code will run when loading an image  is canceled because of decoding error or unsupported format. The new code will save a lot time which is currently spent in loading the rest of the encoded image data even though we know for sure it is just waste and will be thrown away. Also I think it is common practice to make the function itself protected against bad arguments or wrong states regardless who calls it. So you can see the same check is done in multiple levels of the code because the lowest level function might be called from a caller which does not make the same check. For example in the code below if (x < 0) will be executed twice when calling func1(). And the reason is func1() needs it because it does not want to do_some_stuff1() if (x < 0). And func3() has it because func2() wants to do_some_stuff2() regardless of the value of x.

void func1(int x)
{
    if (x < 0)
        return;
    func3(x);
    do_some_stuff1();
}

void func2(int x)
{
    func3(x);
    do_some_stuff2();
}

void func3(int x)
{
    if (x < 0)
        return;
    do_other_stuff3();
}

If you think the patch is fine, please r+ since I need to move forward with the patch.

[youenn fablet]

What about the following?

void CachedImage::addIncrementalDataBuffer(SharedBuffer& data)
{
...
   if (encodedDataStatus == EncodedDataStatus::Error) {
        error(DecodeError);
        if (m_loader)
            m_loader->cancel();
        if (inCache())
            MemoryCache::singleton().remove(*this);
        return;
   }
...
}

It seems easier to understand and, from a quick look, equivalent in terms of behavior.

[Said Abou-Hallawa]

(In reply to youenn fablet from comment #16)
> What about the following?
> 
> void CachedImage::addIncrementalDataBuffer(SharedBuffer& data)
> {
> ...
>    if (encodedDataStatus == EncodedDataStatus::Error) {
>         error(DecodeError);
>         if (m_loader)
>             m_loader->cancel();
>         if (inCache())
>             MemoryCache::singleton().remove(*this);
>         return;
>    }
> ...
> }
> 
> It seems easier to understand and, from a quick look, equivalent in terms of
> behavior.

But this suggestion will call both CachedResource::error() and CachedResource::cancelLoad()

// CachedImage::addIncrementalDataBuffer() calls CachedImage::error() which calls CachedResource::error().
void CachedResource::error(CachedResource::Status status)
{
    setStatus(status);
    ASSERT(errorOccurred());
    m_data = nullptr;

    setLoading(false);
    checkNotify();
}

// CachedImage::addIncrementalDataBuffer() calls ResourceLoader::cancel() which SubresourceLoader::didCancel() which calls CachedResource::cancelLoad().
void CachedResource::cancelLoad()
{
    if (!isLoading() && !stillNeedsLoad())
        return;

    setStatus(LoadError);
    setLoading(false);
    checkNotify();
}

How can this be easier to understand? How can calling setStatus(DecodeError) then calling setStatus(LoadError) be easier to understand? Why do we call setLoading(false) and checkNotify() twice?

[youenn fablet]

(In reply to Said Abou-Hallawa from comment #17)
> (In reply to youenn fablet from comment #16)
> > What about the following?
> > 
> > void CachedImage::addIncrementalDataBuffer(SharedBuffer& data)
> > {
> > ...
> >    if (encodedDataStatus == EncodedDataStatus::Error) {
> >         error(DecodeError);
> >         if (m_loader)
> >             m_loader->cancel();
> >         if (inCache())
> >             MemoryCache::singleton().remove(*this);
> >         return;
> >    }
> > ...
> > }
> > 
> > It seems easier to understand and, from a quick look, equivalent in terms of
> > behavior.
> 
> But this suggestion will call both CachedResource::error() and
> CachedResource::cancelLoad()
> 
> // CachedImage::addIncrementalDataBuffer() calls CachedImage::error() which
> calls CachedResource::error().
> void CachedResource::error(CachedResource::Status status)
> {
>     setStatus(status);
>     ASSERT(errorOccurred());
>     m_data = nullptr;
> 
>     setLoading(false);
>     checkNotify();
> }
> 
> // CachedImage::addIncrementalDataBuffer() calls ResourceLoader::cancel()
> which SubresourceLoader::didCancel() which calls
> CachedResource::cancelLoad().
> void CachedResource::cancelLoad()
> {
>     if (!isLoading() && !stillNeedsLoad())
>         return;
> 
>     setStatus(LoadError);
>     setLoading(false);
>     checkNotify();
> }
> 
> How can this be easier to understand? How can calling setStatus(DecodeError)
> then calling setStatus(LoadError) be easier to understand? Why do we call
> setLoading(false) and checkNotify() twice?

CachedResource::isLoading() will return false.
I would expect CachedResource::stillNeedsLoad() to do the same.
If so cancelLoad will exit early.

[Said Abou-Hallawa]

Created attachment 307254 [details]
Patch

[youenn fablet]

Comment on attachment 307254 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.cpp:407
>          // Image decoding failed. Either we need more image data or the image data is malformed.

This comment seems wrong to me from reading the code below.
If we need more image data, we should not call error(). We should just exit early in order to wait for more data.
Can you check we are doing the right thing for this case and update the comment accordingly?

> Source/WebCore/loader/cache/CachedImage.cpp:409
> +        if (m_loader)

Can we have the case of a null m_loader in CachedImage::addIncrementalDataBuffer.
Probably not, so we might be able to call m_loader->cancel() directly. Maybe with an ASSERT(m_loader) before?

[Build Bot]

Comment on attachment 307254 [details]
Patch

Attachment 307254 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3549497

New failing tests:
webrtc/multi-video.html

[Build Bot]

Created attachment 307256 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Said Abou-Hallawa]

Comment on attachment 307254 [details]
Patch

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

>> Source/WebCore/loader/cache/CachedImage.cpp:409
>> +        if (m_loader)
> 
> Can we have the case of a null m_loader in CachedImage::addIncrementalDataBuffer.
> Probably not, so we might be able to call m_loader->cancel() directly. Maybe with an ASSERT(m_loader) before?

Yes we can have a null m_loader. It happens with ImageDocument. The loader is the document loader. CachedImage does not hold a loader in this case.

[Said Abou-Hallawa]

Created attachment 307527 [details]
Patch

[Said Abou-Hallawa]

Created attachment 307608 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 307608 [details]
Patch

Clearing flags on attachment: 307608

Committed r215574: <http://trac.webkit.org/changeset/215574>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Comment on attachment 307608 [details]
Patch

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

> Source/WebCore/loader/cache/CachedImage.cpp:410
> +            m_loader->cancel();

I know your changes are not causing that, but CachedImage::addIncrementalDataBuffer code seems still a bit odd.
For instance, why are we removing the image from the MemoryCache if it is waiting for more data? We shouldn't do that I guess.

Should we just return early if m_image->isNull() && encodedDataStatus != EncodedDataStatus::Error?
Maybe writing this method with a switch(encodedDataStatus) would make things clearer.
Wdyt?