I've seen ResourceLoader starting itself in when cancelling. The sequence is like: DocumentLoader::stopLoading() cancels a ResourceLoader for a subresource which hasn't been started yet. ResourceLoader::cancel() calls releaseResources() which is a virtual function SubresourceLoader::releaseResources() triggers CachedResourceLoader::loadDone() CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests() which starts the same job ResourceLoader::start() is called.. ... SubresourceLoader::releaseResources() calls ResourceLoader::releaseResources() at the end ResourceLoader::releaseResources() removes itself from ResourceLoadScheduler's list, however, it is too late! ResourceLoader::releaseResources() clears ResourceHandle's client but it doesn't cancel the job. So the real networking job could still be performed, depending on the implementation. Initial thought, move "resourceLoadScheduler()->remove(this)" out of releaseResources(), so make sure it is called before SubresourceLoader triggering "servePendingRequests"
Created attachment 171648 [details] the patch
Comment on attachment 171648 [details] the patch Attachment 171648 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/14677168
> I've seen ResourceLoader starting itself in when cancelling. > > The sequence is like: > > DocumentLoader::stopLoading() cancels a ResourceLoader for a subresource which hasn't been started yet. > ResourceLoader::cancel() calls releaseResources() which is a virtual function > SubresourceLoader::releaseResources() triggers CachedResourceLoader::loadDone() > CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests() which starts the same job > ResourceLoader::start() is called.. > ... > SubresourceLoader::releaseResources() calls ResourceLoader::releaseResources() at the end > ResourceLoader::releaseResources() removes itself from ResourceLoadScheduler's list, however, it is too late! > ResourceLoader::releaseResources() clears ResourceHandle's client but it doesn't cancel the job. So the real networking job could still be performed, depending on the implementation. > To be absolutely clear - This backtrace is the *same* ResourceLoader in ::cancel() as it is in ::start()? And not a different ResourceLoader start()ing as a result of a different one cancel()ing?
(In reply to comment #3) > > I've seen ResourceLoader starting itself in when cancelling. > > > > The sequence is like: > > > > DocumentLoader::stopLoading() cancels a ResourceLoader for a subresource which hasn't been started yet. > > ResourceLoader::cancel() calls releaseResources() which is a virtual function > > SubresourceLoader::releaseResources() triggers CachedResourceLoader::loadDone() > > CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests() which starts the same job > > ResourceLoader::start() is called.. > > ... > > SubresourceLoader::releaseResources() calls ResourceLoader::releaseResources() at the end > > ResourceLoader::releaseResources() removes itself from ResourceLoadScheduler's list, however, it is too late! > > ResourceLoader::releaseResources() clears ResourceHandle's client but it doesn't cancel the job. So the real networking job could still be performed, depending on the implementation. > > > > To be absolutely clear - This backtrace is the *same* ResourceLoader in ::cancel() as it is in ::start()? And not a different ResourceLoader start()ing as a result of a different one cancel()ing? I know that's the premise of the bug, but I just want to make it crystal clear that actually debugging has shown these are one-and-the-same.
Created attachment 171654 [details] second
(In reply to comment #3) > > I've seen ResourceLoader starting itself in when cancelling. > > > > The sequence is like: > > > > DocumentLoader::stopLoading() cancels a ResourceLoader for a subresource which hasn't been started yet. > > ResourceLoader::cancel() calls releaseResources() which is a virtual function > > SubresourceLoader::releaseResources() triggers CachedResourceLoader::loadDone() > > CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests() which starts the same job > > ResourceLoader::start() is called.. > > ... > > SubresourceLoader::releaseResources() calls ResourceLoader::releaseResources() at the end > > ResourceLoader::releaseResources() removes itself from ResourceLoadScheduler's list, however, it is too late! > > ResourceLoader::releaseResources() clears ResourceHandle's client but it doesn't cancel the job. So the real networking job could still be performed, depending on the implementation. > > > > To be absolutely clear - This backtrace is the *same* ResourceLoader in ::cancel() as it is in ::start()? And not a different ResourceLoader start()ing as a result of a different one cancel()ing? Yes. It is exactly same ResourceLoader...
FYI, https://bugs.webkit.org/show_bug.cgi?id=87743 would probably also resolve this by having us do much less in SubresourceLoader::releaseResources().
Actually I don't like the way how CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests(). In the case user just closes a page or stops loading by clicking on stop button, this just kicks off network jobs that will soon be cancelled (or give bigger troubles if not). Should we just make CachedResourceLoader::loadDone() schedule a timer in ResourceLoadScheduler rather than serving the requests right away?
(In reply to comment #8) > Actually I don't like the way how CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests(). In the case user just closes a page or stops loading by clicking on stop button, this just kicks off network jobs that will soon be cancelled (or give bigger troubles if not). > > Should we just make CachedResourceLoader::loadDone() schedule a timer in ResourceLoadScheduler rather than serving the requests right away? It may actually be unnecessary. ResourceLoader::releaseResources() calls ResourceLoadScheduler::remove(), which if I'm reading it correctly should start a timer for servePendingRequests().
(In reply to comment #9) > (In reply to comment #8) > > Actually I don't like the way how CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests(). In the case user just closes a page or stops loading by clicking on stop button, this just kicks off network jobs that will soon be cancelled (or give bigger troubles if not). > > > > Should we just make CachedResourceLoader::loadDone() schedule a timer in ResourceLoadScheduler rather than serving the requests right away? > > It may actually be unnecessary. ResourceLoader::releaseResources() calls ResourceLoadScheduler::remove(), which if I'm reading it correctly should start a timer for servePendingRequests(). The problem is here: void CachedResourceLoader::loadDone() { RefPtr<Document> protect(m_document); if (frame()) frame()->loader()->loadDone(); performPostLoadActions(); if (!m_garbageCollectDocumentResourcesTimer.isActive()) m_garbageCollectDocumentResourcesTimer.startOneShot(0); } void CachedResourceLoader::performPostLoadActions() { checkForPendingPreloads(); resourceLoadScheduler()->servePendingRequests(); } I would like to see performPostLoadActions() being removed, as it shouldn't be responsible to push ResourceLoadScheduler.
(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > Actually I don't like the way how CachedResourceLoader::loadDone() triggers ResourceLoadScheduler::servePendingRequests(). In the case user just closes a page or stops loading by clicking on stop button, this just kicks off network jobs that will soon be cancelled (or give bigger troubles if not). > > > > > > Should we just make CachedResourceLoader::loadDone() schedule a timer in ResourceLoadScheduler rather than serving the requests right away? > > > > It may actually be unnecessary. ResourceLoader::releaseResources() calls ResourceLoadScheduler::remove(), which if I'm reading it correctly should start a timer for servePendingRequests(). > > The problem is here: > > void CachedResourceLoader::loadDone() > { > RefPtr<Document> protect(m_document); > if (frame()) > frame()->loader()->loadDone(); > performPostLoadActions(); > > if (!m_garbageCollectDocumentResourcesTimer.isActive()) > m_garbageCollectDocumentResourcesTimer.startOneShot(0); > } > > void CachedResourceLoader::performPostLoadActions() > { > checkForPendingPreloads(); > resourceLoadScheduler()->servePendingRequests(); > } > > I would like to see performPostLoadActions() being removed, as it shouldn't be responsible to push ResourceLoadScheduler. Right. We might not be able to get rid of the checkForPendingPreloads() call, but I *think* servePendingRequests() is redundant in every useful case, and detrimental in the case you're describing.
Comment on attachment 171654 [details] second cancel the request, as the right solution is probably to make CachedResource not push ResourceLoadSecheduler.
Comment on attachment 171654 [details] second When page is loading, it is good to start next job immediately when it finishes one. Also the patch for Bug 87743 doesn't solve this issue. Raising review request again.
Comment on attachment 171654 [details] second Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.