Bug 120508 - Fix XMLHttpRequest leak document when send() is called multiple times
Summary: Fix XMLHttpRequest leak document when send() is called multiple times
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: XML (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: BlinkMergeCandidate, InRadar
: 120886 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-08-29 20:44 PDT by Ryosuke Niwa
Modified: 2016-02-24 11:48 PST (History)
5 users (show)

See Also:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Ryosuke Niwa 2013-08-29 20:44:10 PDT
Consider merging https://chromium.googlesource.com/chromium/blink/+/3a1a3f05e3a139166f5fa81d1acf3d512cf1c9a1
if the bug exists in WebKit as well

XMLHttpRequest creates a ThreadableLoader which may call XHR async, so it setPendingActivity() to avoid being destroyed. However, before this patch, unsetPendingActivity() was called asynchronously after ThreadableLoader was destroyed, so it lead to multiple problems:
a) When next m_loader was set in send() with pending unsetPendingActivity(), the pendingActivity may be dropped even when there exists new m_loader need protection.
b) pendingActivity may be set multiple times from pending unsetPendingActivity(), but dropProtectionSoon() only decrements m_pendingActivityCount by one, leading to a leak.

This patch fix the above problems by unsetPendingActivity() synchronously with m_loader destruction where possible. XMLHttpRequest::stop() still uses asynchronous unsetPendingActivity() to workaround issues mentioned in r152266.

The file "leak-check.js" was moved from fast/dom to fast/js to enable access from http tests.
Comment 1 Alexey Proskuryakov 2013-09-06 12:54:32 PDT
*** Bug 120886 has been marked as a duplicate of this bug. ***
Comment 2 Alexey Proskuryakov 2013-09-09 12:52:32 PDT
<rdar://problem/14946971>
Comment 3 Alexey Proskuryakov 2016-02-24 09:26:43 PST
WebKit doesn't have this leak, it was introduced in Blink after forking.

I'm not super eager to merge just the tests, as leak tests seems to generally not be worth the cost, due to being unreliable.

> XMLHttpRequest::stop() still uses asynchronous unsetPendingActivity() to workaround issues mentioned in r152266.

This refers to <https://chromiumcodereview.appspot.com/16284009>. Darin has fixed those issues in a better way in <http://trac.webkit.org/changeset/167579>.
Comment 4 Darin Adler 2016-02-24 09:37:38 PST
(In reply to comment #3)
> I'm not super eager to merge just the tests, as leak tests seems to
> generally not be worth the cost, due to being unreliable.

I agree with everything else, but I don’t agree with this! My theory is that we think of leak tests as unreliable right now because we have so many unfixed leaks.
Comment 5 Alexey Proskuryakov 2016-02-24 11:48:10 PST
I think that it's also about timing dependence due to garbage collection, and often relying on additional hooks.