Bug 35377 - Loading a CachedResource should be canceled if all its clients are removed
Summary: Loading a CachedResource should be canceled if all its clients are removed
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Images (show other bugs)
Version: 420+
Hardware: All All
: P2 Normal
Assignee: Said Abou-Hallawa
URL: http://lamp2.fhstp.ac.at/~lbz/beispie...
Keywords: InRadar
Depends on: 87631 87716 88720
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-25 02:28 PST by lbzwischenbrugger
Modified: 2019-03-12 14:27 PDT (History)
26 users (show)

See Also:


Attachments
Cancel loads in CachedResource::allClientsRemoved. (9.58 KB, patch)
2012-05-25 11:02 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff
Patch (9.07 KB, patch)
2017-11-29 18:12 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews112 for mac-elcapitan (2.99 MB, application/zip)
2017-11-29 19:44 PST, Build Bot
no flags Details
Patch (9.98 KB, patch)
2017-11-30 12:25 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews115 for mac-elcapitan (3.33 MB, application/zip)
2017-11-30 14:01 PST, Build Bot
no flags Details
Patch (13.53 KB, patch)
2017-11-30 19:07 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (2.10 MB, application/zip)
2017-11-30 20:33 PST, Build Bot
no flags Details
Patch (13.53 KB, patch)
2017-12-01 09:31 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.79 KB, patch)
2018-01-08 15:43 PST, Said Abou-Hallawa
no flags Details | Formatted Diff | Diff
Patch (12.93 KB, patch)
2018-01-08 17:48 PST, Said Abou-Hallawa
sabouhallawa: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description lbzwischenbrugger 2010-02-25 02:28:29 PST
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.
Comment 1 lbzwischenbrugger 2010-02-25 02:30:07 PST
There is no known workaround for this problem.
Comment 2 Darin Adler 2010-02-25 08:26:13 PST
CachedResource doesn't stop the load when the last client goes away. CachedResource::removeClient is the function that would do this.
Comment 3 Alexey Proskuryakov 2010-02-25 15:55:03 PST

*** This bug has been marked as a duplicate of bug 6656 ***
Comment 4 Simon Fraser (smfr) 2011-08-26 16:52:47 PDT
Reopening, based on ap's comments.
Comment 5 Nate Chapin 2012-05-25 11:02:35 PDT
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.
Comment 6 Darin Adler 2012-05-26 17:01:18 PDT
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.
Comment 7 WebKit Review Bot 2012-05-26 17:46:27 PDT
Comment on attachment 144102 [details]
Cancel loads in CachedResource::allClientsRemoved.

Clearing flags on attachment: 144102

Committed r118618: <http://trac.webkit.org/changeset/118618>
Comment 8 WebKit Review Bot 2012-05-26 17:46:32 PDT
All reviewed patches have been landed.  Closing bug.
Comment 9 Chris Dumez 2012-05-28 01:17:48 PDT
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.
Comment 10 János Badics 2012-05-29 01:30:23 PDT
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.
Comment 11 János Badics 2012-05-29 02:23:32 PDT
http/tests/cache/cancel-in-progress-load.html has been skipped on Qt in
http://trac.webkit.org/changeset/118743
Comment 12 Csaba Osztrogonác 2012-05-29 02:31:27 PDT
(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.
Comment 13 Csaba Osztrogonác 2012-05-29 02:34:09 PDT
It fails on GTK too, check http://trac.webkit.org/changeset/118630 for details.
Comment 14 Stephanie Lewis 2012-05-29 04:36:45 PDT
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
Comment 15 WebKit Review Bot 2012-06-09 15:56:39 PDT
Re-opened since this is blocked by 88720
Comment 16 Abhishek Arya 2012-06-11 09:23:51 PDT
Patch was rolled out in http://trac.webkit.org/changeset/119918 due to excessive number of security crashes.
Comment 17 Thomas Fischer 2013-02-01 07:43:48 PST
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
Comment 18 Shunfang 2013-10-24 03:28:07 PDT
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
Comment 19 Radar WebKit Bug Importer 2013-10-24 10:26:27 PDT
<rdar://problem/15310413>
Comment 20 Simon Fraser (smfr) 2017-07-06 13:47:59 PDT
http://lamp2.fhstp.ac.at/~lbz/beispiele/ss2010/webkit/shift.html doesn't exist now. Does anyone have a reachable test page?
Comment 21 Said Abou-Hallawa 2017-11-29 18:12:47 PST
Created attachment 327942 [details]
Patch
Comment 22 Said Abou-Hallawa 2017-11-29 18:17:09 PST
I think the original patch was fine but the test was wrong. It has the following javascript code:

function finish() {
    (window.layoutTestController)
        layoutTestController.notifyDone();
}
Comment 23 Build Bot 2017-11-29 19:44:19 PST
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
Comment 24 Build Bot 2017-11-29 19:44:21 PST
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
Comment 25 Said Abou-Hallawa 2017-11-30 12:25:03 PST
Created attachment 328006 [details]
Patch
Comment 26 Build Bot 2017-11-30 14:01:45 PST
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
Comment 27 Build Bot 2017-11-30 14:01:47 PST
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
Comment 28 Said Abou-Hallawa 2017-11-30 19:07:01 PST
Created attachment 328065 [details]
Patch
Comment 29 Build Bot 2017-11-30 20:33:52 PST
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
Comment 30 Build Bot 2017-11-30 20:33:53 PST
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
Comment 31 Said Abou-Hallawa 2017-12-01 09:31:22 PST
Created attachment 328104 [details]
Patch
Comment 32 Said Abou-Hallawa 2017-12-01 10:28:11 PST
-- 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.
Comment 33 Said Abou-Hallawa 2018-01-08 15:43:41 PST
Created attachment 330746 [details]
Patch
Comment 34 Tim Horton 2018-01-08 17:03:40 PST
Comment on attachment 330746 [details]
Patch

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

> Source/WebCore/ChangeLog:1
> +2018-01-08  Said Abou-Hallawa  <sabouhallawa@apple.com>

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
Comment 35 Said Abou-Hallawa 2018-01-08 17:48:13 PST
Created attachment 330768 [details]
Patch
Comment 36 Said Abou-Hallawa 2018-01-08 17:50:50 PST
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.
Comment 37 Simon Fraser (smfr) 2018-01-08 18:18:26 PST
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?
Comment 38 youenn fablet 2018-01-09 03:15:48 PST
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();
Comment 39 Darin Adler 2018-01-09 08:02:01 PST
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.