[XHR] Use hasPendingActivity() to manage XHR's life time.

[XHR] Use hasPendingActivity() to manage XHR's life time.

BUG=none

[tyoshino (SeeGerritForStatus), 2013-12-04 16:52:53]

Please stop reviewing. I just noticed I forgot to remove setPendingActivity()
call.

[tyoshino (SeeGerritForStatus), 2013-12-04 17:57:16]

Fixed. PTAL

[abarth-chromium, 2013-12-05 04:14:03]

If this works, this is great.

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1346: internalAbort();
Don't we need to set some sort of flag here to prevent us from starting a new
request?

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1351: return m_loader;
Is there something that stops m_loader from being created if we're already in
the stopped state?

[abarth-chromium, 2013-12-05 04:17:24]

This looks very promising, but I think there are a couple details to worry
about.

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (left):

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:851: RefPtr<XMLHttpRequest> protect(this);
Don't we still need this?  Won't internalAbort remove m_loader and make use
eligible for GC?  If a GC occurs during this function, we'll be destroyed while
on the stack.

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:904: m_error = true;
For example, on this line of code, m_loader is 0, which means we could be GCed.

[tyoshino (SeeGerritForStatus), 2013-12-05 04:26:20]

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (right):

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1346: internalAbort();
On 2013/12/05 04:14:03, abarth wrote:
> Don't we need to set some sort of flag here to prevent us from starting a new
> request?

Good catch! We don't want it to start a new loading here.

When I was fixing uaf issue due to reentrance, I asked japhet@ for comments.
There were two choices
a) suppress new loading by inner open() (via window.onload) and continue outer
open()
b) continue inner and abort outer abort

We chose (b). Maybe it's time to rethink. What do you think? We can do (a)
exceptionally for stop() case, but if it makes sense for other cases, we can do
(a) for all.

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:1351: return m_loader;
On 2013/12/05 04:14:03, abarth wrote:
> Is there something that stops m_loader from being created if we're already in
> the stopped state?

I once tried to add a boolean member, say "m_abortOngoing", to prevent new
loading being started and set it in internalAbort() so that any XHR method will
be noop while we're in internalAbort(). Maybe checking it just in open() is
enough.

[tyoshino (SeeGerritForStatus), 2013-12-05 04:32:10]

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
File Source/core/xml/XMLHttpRequest.cpp (left):

https://codereview.chromium.org/104363002/diff/60001/Source/core/xml/XMLHttpR...
Source/core/xml/XMLHttpRequest.cpp:851: RefPtr<XMLHttpRequest> protect(this);
On 2013/12/05 04:17:24, abarth wrote:
> Don't we still need this?  Won't internalAbort remove m_loader and make use
> eligible for GC?  If a GC occurs during this function, we'll be destroyed
while
> on the stack.

oh right! I still didn't understand correctly when GC may happen. kouhei
educated me. thanks. I'll put it back.

[tyoshino (SeeGerritForStatus), 2013-12-05 04:57:15]

I forgot to check if there's any user of WebCore::XMLHttpRequest than V8
wrapper. Different from Notification and WebSocket, it is.

InspectorResourceAgent::replayXHR() is using WebCore::XMLHttpRequest directly
and looks depending on the current set/unsetPendingActivity() based lifetime
management mechanism which inc/dec ref count.

[tyoshino (SeeGerritForStatus), 2013-12-05 05:08:20]

On 2013/12/05 04:57:15, tyoshino wrote:
> I forgot to check if there's any user of WebCore::XMLHttpRequest than V8
> wrapper. Different from Notification and WebSocket, it is.
> 
> InspectorResourceAgent::replayXHR() is using WebCore::XMLHttpRequest directly
> and looks depending on the current set/unsetPendingActivity() based lifetime
> management mechanism which inc/dec ref count.

We could converge them into V8 wrapped pattern by creating a V8 wrapper also in
this code path.

[tyoshino (SeeGerritForStatus), 2014-08-21 06:22:08]

On 2013/12/05 05:08:20, tyoshino wrote:
> On 2013/12/05 04:57:15, tyoshino wrote:
> > I forgot to check if there's any user of WebCore::XMLHttpRequest than V8
> > wrapper. Different from Notification and WebSocket, it is.
> > 
> > InspectorResourceAgent::replayXHR() is using WebCore::XMLHttpRequest
directly
> > and looks depending on the current set/unsetPendingActivity() based lifetime
> > management mechanism which inc/dec ref count.
> 
> We could converge them into V8 wrapped pattern by creating a V8 wrapper also
in
> this code path.

Closing obsolete CL. This has been done by kouhei@.