Bug 35377 – Loading a CachedResource should be canceled if all its clients are removed

• Status: NEW
• Component: Images

If webkit starts to load an image it is not cancelable.

The load process should stop if 
1.) src attribute is changed
2.) img element is removed from html page

To see the (mis)behaviour go to page:
http://lamp2.fhstp.ac.at/~lbz/beispiele/ss2010/webkit/shift.html

Clicking the "next" button will load 100 pictures. If pressing the "next" button for example 5 times, it means
that webkit has to load 500 images. Firefox loads only (about) 100 images and is MUCH faster on that.

There is no known workaround for this problem.

[Darin Adler]

CachedResource doesn't stop the load when the last client goes away. CachedResource::removeClient is the function that would do this.

[Alexey Proskuryakov]

*** This bug has been marked as a duplicate of bug 6656 ***

[Simon Fraser (smfr)]

Reopening, based on ap's comments.

[Nate Chapin]

Created attachment 144102 [details]
Cancel loads in CachedResource::allClientsRemoved.

We currently cancel CachedRawResources when the last client is removed. This patch moves that logic down into CachedResource so all resource types can use it.

This patch may also fix bug 6656, but I haven't thoroughly tested all cases.

[Darin Adler]

Comment on attachment 144102 [details]
Cancel loads in CachedResource::allClientsRemoved.

In cases like this, where we want to be sure that derived classes call through to the base class, it can be useful to have a debug assertion that fires if someone forgets to do that.

If we know there is only one call site for a particular virtual function, we can set up a global pointer to double check that we call all the way through to the base class’s implementation exactly once. We should consider creating a debug macro to help deploy this kind of checking for cases like this one.

[WebKit Review Bot]

Comment on attachment 144102 [details]
Cancel loads in CachedResource::allClientsRemoved.

Clearing flags on attachment: 144102

Committed r118618: <http://trac.webkit.org/changeset/118618>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

The following test cases started failing on EFL port after this patch:
  fast/dom/beforeload/image-object-before-load-innerHTML.html = TEXT
  fast/dom/beforeload/image-object-before-load.html = TEXT

I opened Bug 87631 to track the issue and skipped the tests to make the tree green. You can find information about initial investigation of the failure on that bug. Any input to help solve the problem is welcome.

[János Badics]

http/tests/cache/cancel-in-progress-load.html started failing on Qt from r118618:

-<unknown> - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code -999, failing URL "http://127.0.0.1:8000/navigation/resources/slow-resource.pl">
+<unknown> - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code 5, failing URL "http://127.0.0.1:8000/navigation/resources/slow-resource.pl">

I will skip this test until it is fixed.

[János Badics]

http/tests/cache/cancel-in-progress-load.html has been skipped on Qt in
http://trac.webkit.org/changeset/118743

[Csaba Osztrogonác]

(In reply to comment #10)
> http/tests/cache/cancel-in-progress-load.html started failing on Qt from r118618:
> 
> -<unknown> - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code -999, failing URL "http://127.0.0.1:8000/navigation/resources/slow-resource.pl">
> +<unknown> - didFailLoadingWithError: <NSError domain NSURLErrorDomain, code 5, failing URL "http://127.0.0.1:8000/navigation/resources/slow-resource.pl">
> 
> I will skip this test until it is fixed.

It is a new test introduced in r118618 and fails on Qt. Please file a new bug report for it.

[Csaba Osztrogonác]

It fails on GTK too, check http://trac.webkit.org/changeset/118630 for details.

[Stephanie Lewis]

Mac as well, filed https://bugs.webkit.org/show_bug.cgi?id=87728.  Added to Mac skip list in http://trac.webkit.org/projects/webkit/changeset/118753

[WebKit Review Bot]

Re-opened since this is blocked by 88720

[Abhishek Arya]

Patch was rolled out in http://trac.webkit.org/changeset/119918 due to excessive number of security crashes.

[Thomas Fischer]

This bug is a show blocker for web apps which have to manage lot of images like a map app.
The only workaround yet is creating a hidden frame for each and every image to be able to cacnel the image loading by calling frame.stop()

The other side of the coin: This workaround causes a heavy memory foor print  :-(

Please, let the Image object behave like in all other browsers:
If the source has been changed the current http request is canceled and the already loaded part of the image is cached as "partial" 
If later on the previous image resource is requested again then utilize http "Content-Range" to load the rest of the resource

[Shunfang]

I fully agree Thomas Fischer's way, it's useful and important on mobile platform too.

(In reply to comment #17)
> This bug is a show blocker for web apps which have to manage lot of images like a map app.
> The only workaround yet is creating a hidden frame for each and every image to be able to cacnel the image loading by calling frame.stop()
> 
> The other side of the coin: This workaround causes a heavy memory foor print  :-(
> 
> Please, let the Image object behave like in all other browsers:
> If the source has been changed the current http request is canceled and the already loaded part of the image is cached as "partial" 
> If later on the previous image resource is requested again then utilize http "Content-Range" to load the rest of the resource

[Radar WebKit Bug Importer]

<rdar://problem/15310413>

[Simon Fraser (smfr)]

http://lamp2.fhstp.ac.at/~lbz/beispiele/ss2010/webkit/shift.html doesn't exist now. Does anyone have a reachable test page?

[Said Abou-Hallawa]

Created attachment 327942 [details]
Patch

[Said Abou-Hallawa]

I think the original patch was fine but the test was wrong. It has the following javascript code:

function finish() {
    (window.layoutTestController)
        layoutTestController.notifyDone();
}

[EWS Watchlist]

Comment on attachment 327942 [details]
Patch

Attachment 327942 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5412631

New failing tests:
fast/replaced/outline-replaced-elements-offset.html
media/modern-media-controls/media-controller/media-controller-click-on-video-background-to-dismiss-tracks-panel-should-not-toggle-playback.html

[EWS Watchlist]

Created attachment 327946 [details]
Archive of layout-test-results from ews112 for mac-elcapitan

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

[Said Abou-Hallawa]

Created attachment 328006 [details]
Patch

[EWS Watchlist]

Comment on attachment 328006 [details]
Patch

Attachment 328006 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/5422049

New failing tests:
media/modern-media-controls/media-controller/media-controller-click-on-video-background-to-dismiss-tracks-panel-should-not-toggle-playback.html

[EWS Watchlist]

Created attachment 328021 [details]
Archive of layout-test-results from ews115 for mac-elcapitan

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

[Said Abou-Hallawa]

Created attachment 328065 [details]
Patch

[EWS Watchlist]

Comment on attachment 328065 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/resource-timing/single-entry-per-resource.html

[EWS Watchlist]

Created attachment 328068 [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.12.6

[Said Abou-Hallawa]

Created attachment 328104 [details]
Patch

[Said Abou-Hallawa]

-- The wincairo ews is down so it can't build any patch.

-- The test imported/w3c/web-platform-tests/resource-timing/single-entry-per-resource.html was failing on ios-sim but this test has been flaky and this failure is not related to this patch.

[Said Abou-Hallawa]

Created attachment 330746 [details]
Patch

[Tim Horton]

Comment on attachment 330746 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> [phone]  Said Abou-Hallawa  <[e-mail]>

You should probably get a loadingy person to review. Also, what will the web inspector say??

> Source/WebCore/ChangeLog:4
> +        https://bugs.webkit.org/show_bug.cgi?id=35377

I /know/ there’s a radar that should be here

> Source/WebCore/ChangeLog:9
> +        as a client of a CachedResourse and also it's the last client of this

Resource

> Source/WebCore/ChangeLog:10
> +        CachedResourse, usually it is not safe to cancel loading the resource if

Resource

> Source/WebCore/ChangeLog:17
> +        canceling loading the CachedResourse will be allowed.

Resource

> Source/WebCore/loader/cache/CachedFont.cpp:146
> +void CachedFont::allClientsRemoved(bool isOkToCancelLoading)

As usual, I would rather use an enum (class).

> LayoutTests/http/tests/cache/cancel-in-progress-load.html:12
> +    <img onload="finish();" onerror="finish();" src="http://127.0.0.1:8000/navigation/resources/slow-resource.pl?delay=3000"></img>

No need for these weird semicolons

[Said Abou-Hallawa]

Created attachment 330768 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 330746 [details]
Patch

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

>> Source/WebCore/ChangeLog:4
>> +        https://bugs.webkit.org/show_bug.cgi?id=35377
> 
> I /know/ there’s a radar that should be here

A radar link was added.

>> Source/WebCore/ChangeLog:9
>> +        as a client of a CachedResourse and also it's the last client of this
> 
> Resource

Fixed.

>> Source/WebCore/ChangeLog:10
>> +        CachedResourse, usually it is not safe to cancel loading the resource if
> 
> Resource

Fixed.

>> Source/WebCore/ChangeLog:17
>> +        canceling loading the CachedResourse will be allowed.
> 
> Resource

Fixed.

>> Source/WebCore/loader/cache/CachedFont.cpp:146
>> +void CachedFont::allClientsRemoved(bool isOkToCancelLoading)
> 
> As usual, I would rather use an enum (class).

I passed the lastClient instead. CachedResource::allClientsRemoved() will check lastClient.isOkToCancelLoading().

>> LayoutTests/http/tests/cache/cancel-in-progress-load.html:12
>> +    <img onload="finish();" onerror="finish();" src="http://127.0.0.1:8000/navigation/resources/slow-resource.pl?delay=3000"></img>
> 
> No need for these weird semicolons

Sime-colons were removed.

[Simon Fraser (smfr)]

Comment on attachment 330768 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResource.h:147
> +    virtual void allClientsRemoved(const CachedResourceClient& lastClient);

It's a bit weird to pass the last client here.

> Source/WebCore/rendering/RenderObject.h:751
> +    bool isOkToCancelLoading() const override { return !beingDestroyed(); }

Why can't we cancel if the renderer is being destroyed? Isn't this the most common case?

[youenn fablet]

Comment on attachment 330768 [details]
Patch

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

> Source/WebCore/ChangeLog:13
> +        "complete" event to the renderer's element. Deleting the RenderObject might

I am probably missing something here.
If CachedResource has no longer any client, how is it able to trigger this "complete event" on the render element?
Can we try to change that code path instead?

Cancelling the load when there is no longer any client seems good to me.
Improving the way we are cancelling or treating CachedResource callbacks in the case we need to skip the event dispatching seems more natural to me.

> Source/WebCore/loader/cache/CachedImage.h:112
> +    void allClientsRemoved(const CachedResourceClient& lastClient) override;

Should probably be final.
Agreed with smfr that passing lastClient as parameter seems strange.
If we want to go in that direction of checking whether last client wants to cancel the load or not, can CachedResource::removeClient do something like:

allClientsRemoved();
if (m_loader && lastClient.isOkToCancelLoading())
    m_loader->cancelIfNotFinishing();

[Darin Adler]

Seems that basing the decision on whether to cancel the load on the last client is intrinsically racy; removing clients in a different order can result in a load canceled or not. I can’t imagine that will reliably get us the behavior we want. We need to think in terms of invariants, not in terms of the effects of any one client removal.

[Alex Christensen]

Comment on attachment 330768 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.