WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
NEW
100791
ResourceLoader can start itself in cancel()
https://bugs.webkit.org/show_bug.cgi?id=100791
Summary
ResourceLoader can start itself in cancel()
Yong Li
Reported
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"
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
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Yong Li
Comment 1
2012-10-31 07:14:06 PDT
Created
attachment 171648
[details]
the patch
Early Warning System Bot
Comment 2
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
Brady Eidson
Comment 3
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?
Brady Eidson
Comment 4
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.
Yong Li
Comment 5
2012-10-31 07:25:55 PDT
Created
attachment 171654
[details]
second
Yong Li
Comment 6
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...
Nate Chapin
Comment 7
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().
Yong Li
Comment 8
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?
Nate Chapin
Comment 9
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().
Yong Li
Comment 10
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.
Nate Chapin
Comment 11
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.
Yong Li
Comment 12
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.
Yong Li
Comment 13
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.
Anders Carlsson
Comment 14
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.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug