Fix XMLHttpRequest leak document when send() is called multiple times.

Fix XMLHttpRequest leak document when send() is called multiple times.

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.

BUG=270000
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156078

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=156456

[kouhei (in TOK), 2013-08-14 01:20:33]

abarth, haraken: Would you take a look?

[haraken, 2013-08-14 01:30:58]

https://codereview.chromium.org/23005006/diff/1/Source/core/xml/XMLHttpReques...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:764: if (!hasPendingActivity())

I'm not sure if this is a right fix. If send() is called twice, you're not
setting the pending activity for the second send(). Thus, the XHR object for the
second send() can be prematurely deleted.

Maybe a right fix would be to keep track of the list of pending activities and
make sure that stop() clears all the pending activities?

[kouhei (in TOK), 2013-08-14 02:30:09]

Updated patch. Would you take a look?

https://codereview.chromium.org/23005006/diff/1/Source/core/xml/XMLHttpReques...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/1/Source/core/xml/XMLHttpReques...
Source/core/xml/XMLHttpRequest.cpp:764: if (!hasPendingActivity())
On 2013/08/14 01:30:59, haraken wrote:
> 
> I'm not sure if this is a right fix. If send() is called twice, you're not
> setting the pending activity for the second send(). Thus, the XHR object for
the
> second send() can be prematurely deleted.
> 
> Maybe a right fix would be to keep track of the list of pending activities and
> make sure that stop() clears all the pending activities?

There should not be a pending activity at this point anyway. I changed this to
assertion in updated patch. m_loader is ensured to be null on initSend(), which
must be called prior to this function.

The case where this could have a pending activity here was from
unsetPendingActivity called asynchronously. I fixed the issue in the updated
patch.

[haraken, 2013-08-14 02:35:06]

LGTM. abarth might want to take another look.

https://codereview.chromium.org/23005006/diff/7001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/7001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:759: ASSERT(!hasPendingActivity());

Would you add a comment on this line?

https://codereview.chromium.org/23005006/diff/7001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:805: void XMLHttpRequest::internalAbort(bool
dropProtectionAsync)

Instead of bool, could you use a more descriptive enum?

[kouhei (in TOK), 2013-08-14 03:19:30]

abarth: Would you take a look?

https://codereview.chromium.org/23005006/diff/7001/Source/core/xml/XMLHttpReq...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/7001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:759: ASSERT(!hasPendingActivity());
On 2013/08/14 02:35:07, haraken wrote:
> 
> Would you add a comment on this line?

Done.

https://codereview.chromium.org/23005006/diff/7001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:805: void XMLHttpRequest::internalAbort(bool
dropProtectionAsync)
On 2013/08/14 02:35:07, haraken wrote:
> 
> Instead of bool, could you use a more descriptive enum?

Done.

[abarth-chromium, 2013-08-14 04:11:48]

https://codereview.chromium.org/23005006/diff/18001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/18001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1233: internalAbort(DropProtectionAsync);
What I would do is make DropProtectionSync the default value and have
DropProtectionAsync in the one call site that needs it.  That makes it clear
that most code wants to drop protection sync.

LGTM

[kouhei (in TOK), 2013-08-14 04:58:20]

Thank you for the review!

https://codereview.chromium.org/23005006/diff/18001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/18001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1233: internalAbort(DropProtectionAsync);
On 2013/08/14 04:11:49, abarth wrote:
> What I would do is make DropProtectionSync the default value and have
> DropProtectionAsync in the one call site that needs it.  That makes it clear
> that most code wants to drop protection sync.
> 
> LGTM

Done.

[commit-bot: I haz the power, 2013-08-14 04:58:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/23005006/25001

[commit-bot: I haz the power, 2013-08-14 05:02:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/23005006/2001

[commit-bot: I haz the power, 2013-08-14 05:24:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/23005006/39001

[commit-bot: I haz the power, 2013-08-14 07:49:15]

Change committed as 156078

[kouhei (in TOK), 2013-08-14 08:41:14]

Change was reverted as r156082

[kouhei (in TOK), 2013-08-15 04:08:39]

abarth: Would you take a look?

The CL failed to land as ASSERT(!hasPendingActivity()); failed in async XHR
tests.

The problem was that changeState() in didFinishLoad() was called before
unsetPendingActivity(), and changeState() invokes onreadystatechange callbacks,
and those callbacks may call send() internally, causing the assertion to fail. I
have modified the CL so that unsetPendingActivity() is always called before
changeState() so it is unset in send().

[haraken, 2013-08-15 04:53:54]

On 2013/08/15 04:08:39, kouhei wrote:
> abarth: Would you take a look?
> 
> The CL failed to land as ASSERT(!hasPendingActivity()); failed in async XHR
> tests.
> 
> The problem was that changeState() in didFinishLoad() was called before
> unsetPendingActivity(), and changeState() invokes onreadystatechange
callbacks,
> and those callbacks may call send() internally, causing the assertion to fail.
I
> have modified the CL so that unsetPendingActivity() is always called before
> changeState() so it is unset in send().

LGTM. abarth might want to take another look.

[kouhei (in TOK), 2013-08-16 06:00:07]

> LGTM. abarth might want to take another look.
abarth: Would you take a look? Thanks.

[abarth-chromium, 2013-08-19 17:53:17]

lgtm

https://chromiumcodereview.appspot.com/23005006/diff/50001/Source/core/xml/XM...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://chromiumcodereview.appspot.com/23005006/diff/50001/Source/core/xml/XM...
Source/core/xml/XMLHttpRequest.cpp:1091: protect = this;
I probably would have just done this unconditionally on line 1086.

[abarth-chromium, 2013-08-19 17:53:23]

Sorry for the delay in reviewing.

[kouhei (in TOK), 2013-08-19 23:44:59]

Thanks for the review!

https://codereview.chromium.org/23005006/diff/50001/Source/core/xml/XMLHttpRe...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/23005006/diff/50001/Source/core/xml/XMLHttpRe...
Source/core/xml/XMLHttpRequest.cpp:1091: protect = this;
On 2013/08/19 17:53:17, abarth wrote:
> I probably would have just done this unconditionally on line 1086.

Done.

[commit-bot: I haz the power, 2013-08-20 00:06:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/23005006/69001

[commit-bot: I haz the power, 2013-08-20 01:43:19]

Retried try job too often on linux_blink_rel for step(s) webkit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_blin...

[kouhei (in TOK), 2013-08-21 06:17:10]

It seems that there was a sanity check added to XHR::send(). Updated the
LayoutTest to comply with it.

[commit-bot: I haz the power, 2013-08-21 06:17:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/kouhei@chromium.org/23005006/77001

[commit-bot: I haz the power, 2013-08-21 08:20:23]

Change committed as 156456