Bug 100791 - ResourceLoader can start itself in cancel()
Summary: ResourceLoader can start itself in cancel()
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-10-30 15:04 PDT by Yong Li
Modified: 2014-02-05 11:02 PST (History)
3 users (show)

See Also:


Attachments
the patch (4.15 KB, patch)
2012-10-31 07:14 PDT, Yong Li
webkit-ews: commit-queue-
Details | Formatted Diff | Diff
second (4.14 KB, patch)
2012-10-31 07:25 PDT, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2012-10-30 15:04:05 PDT
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"
Comment 1 Yong Li 2012-10-31 07:14:06 PDT
Created attachment 171648 [details]
the patch
Comment 2 Early Warning System Bot 2012-10-31 07:21:22 PDT
Comment on attachment 171648 [details]
the patch

Attachment 171648 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14677168
Comment 3 Brady Eidson 2012-10-31 07:24:19 PDT
> 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?
Comment 4 Brady Eidson 2012-10-31 07:24:58 PDT
(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.
Comment 5 Yong Li 2012-10-31 07:25:55 PDT
Created attachment 171654 [details]
second
Comment 6 Yong Li 2012-10-31 07:29:36 PDT
(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...
Comment 7 Nate Chapin 2012-10-31 09:23:35 PDT
FYI, https://bugs.webkit.org/show_bug.cgi?id=87743 would probably also resolve this by having us do much less in SubresourceLoader::releaseResources().
Comment 8 Yong Li 2012-11-02 11:08:01 PDT
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?
Comment 9 Nate Chapin 2012-11-02 11:17:51 PDT
(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().
Comment 10 Yong Li 2012-11-02 11:39:46 PDT
(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.
Comment 11 Nate Chapin 2012-11-02 11:44:29 PDT
(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 12 Yong Li 2012-11-15 10:16:58 PST
Comment on attachment 171654 [details]
second

cancel the request, as the right solution is probably to make CachedResource not push ResourceLoadSecheduler.
Comment 13 Yong Li 2012-12-10 14:24:31 PST
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 14 Anders Carlsson 2014-02-05 11:02:41 PST
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.