Bug 35377

There is no known workaround for this problem.

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

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

Reopening, based on ap's comments.

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 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 on attachment 144102 [details] Cancel loads in CachedResource::allClientsRemoved. Clearing flags on attachment: 144102 Committed r118618: <http://trac.webkit.org/changeset/118618>

All reviewed patches have been landed. Closing bug.

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.

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.

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

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

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

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

Re-opened since this is blocked by 88720

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

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

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

<rdar://problem/15310413>

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

Created attachment 327942 [details] Patch

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

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

Created attachment 328006 [details] Patch

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

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

Created attachment 328065 [details] Patch

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

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

Created attachment 328104 [details] Patch

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

Created attachment 330746 [details] Patch

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

Created attachment 330768 [details] Patch

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

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.

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.