Bug 100791 – ResourceLoader can start itself in cancel()

• Component: Page Loading
• Status: NEW

[Yong Li]

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"

[Yong Li]

Created attachment 171648 [details]
the patch

[Early Warning System Bot]

Comment on attachment 171648 [details]
the patch

Attachment 171648 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/14677168

[Brady Eidson]

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

[Brady Eidson]

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

[Yong Li]

Created attachment 171654 [details]
second

[Yong Li]

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

[Nate Chapin]

FYI, https://bugs.webkit.org/show_bug.cgi?id=87743 would probably also resolve this by having us do much less in SubresourceLoader::releaseResources().

[Yong Li]

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?

[Nate Chapin]

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

[Yong Li]

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

[Nate Chapin]

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

[Yong Li]

Comment on attachment 171654 [details]
second

cancel the request, as the right solution is probably to make CachedResource not push ResourceLoadSecheduler.

[Yong Li]

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.

[Anders Carlsson]

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.