Prevent entering XHR methods while loader is being cancelled.

Prevent entering XHR methods while loader is being cancelled.

Calling cancel() on DocumentThreadableLoader may results in calling
window.onload synchronously. If open(), send(), etc. are called on the same
XMLHttpRequest object, it'll be hard to resolve conflict of states without
losing spec conformance. This CL avoids that by just disallowing reentrant
and throwing an exception.

BUG=292422

[Nate Chapin, 2013-09-19 16:31:30]

Ideally, we could solve this without an exception and without the new boolean.

Particularly, I don't have permissions to approve idl changes, so you would
still need to get OWNERS approval to add the exception.

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

https://codereview.chromium.org/23465030/diff/6001/Source/core/xml/XMLHttpReq...
Source/core/xml/XMLHttpRequest.cpp:881: m_loader->cancel();
Is it possible to solve this similarly to
http://src.chromium.org/viewvc/blink?view=revision&revision=120845, by storing
an RefPtr locally and clearing m_loader before calling cancel()?

[tyoshino (SeeGerritForStatus), 2013-09-19 17:51:50]

On 2013/09/19 16:31:30, Nate Chapin wrote:
> Ideally, we could solve this without an exception and without the new boolean.
> 
> Particularly, I don't have permissions to approve idl changes, so you would
> still need to get OWNERS approval to add the exception.

OK. I'll ask haraken or someone for review once we reach right solution.

> 
>
https://codereview.chromium.org/23465030/diff/6001/Source/core/xml/XMLHttpReq...
> File Source/core/xml/XMLHttpRequest.cpp (right):
> 
>
https://codereview.chromium.org/23465030/diff/6001/Source/core/xml/XMLHttpReq...
> Source/core/xml/XMLHttpRequest.cpp:881: m_loader->cancel();
> Is it possible to solve this similarly to
> http://src.chromium.org/viewvc/blink?view=revision&revision=120845, by storing
> an RefPtr locally and clearing m_loader before calling cancel()?

I tried. It was

RefPtr<ThreadableLoader> loader = m_loader.release();
loader->cancel();

maybe same as what you're suggesting. I could avoid crash for reentrant of
open() by this method, but if onload contains open() and send() call, we'll have
non NULL m_loader after loader->cancel() call. So, we need to cancel and clear
it again. Regarding onload, maybe it's ok if we do clean up twice. But I'm not
sure if this is robust, i.e. works for other events (I haven't surveyed if
there're such ones).

[Nate Chapin, 2013-09-19 17:58:21]

On 2013/09/19 17:51:50, tyoshino wrote:
> On 2013/09/19 16:31:30, Nate Chapin wrote:
> > Ideally, we could solve this without an exception and without the new
boolean.
> > 
> > Particularly, I don't have permissions to approve idl changes, so you would
> > still need to get OWNERS approval to add the exception.
> 
> OK. I'll ask haraken or someone for review once we reach right solution.
> 
> > 
> >
>
https://codereview.chromium.org/23465030/diff/6001/Source/core/xml/XMLHttpReq...
> > File Source/core/xml/XMLHttpRequest.cpp (right):
> > 
> >
>
https://codereview.chromium.org/23465030/diff/6001/Source/core/xml/XMLHttpReq...
> > Source/core/xml/XMLHttpRequest.cpp:881: m_loader->cancel();
> > Is it possible to solve this similarly to
> > http://src.chromium.org/viewvc/blink?view=revision&revision=120845, by
storing
> > an RefPtr locally and clearing m_loader before calling cancel()?
> 
> I tried. It was
> 
> RefPtr<ThreadableLoader> loader = m_loader.release();
> loader->cancel();
> 
> maybe same as what you're suggesting. I could avoid crash for reentrant of
> open() by this method, but if onload contains open() and send() call, we'll
have
> non NULL m_loader after loader->cancel() call. So, we need to cancel and clear
> it again. Regarding onload, maybe it's ok if we do clean up twice. But I'm not
> sure if this is robust, i.e. works for other events (I haven't surveyed if
> there're such ones).

Ok, what about:

RefPtr<ThreadableLoader> loader = m_loader.release();
loader->cancel();
// A new load may start re-entrantly if cancel() causes an event to fire.
if (m_loader)
    return;

[tyoshino (SeeGerritForStatus), 2013-09-19 18:15:16]

On 2013/09/19 17:58:21, Nate Chapin wrote:
> Ok, what about:
> 
> RefPtr<ThreadableLoader> loader = m_loader.release();
> loader->cancel();
> // A new load may start re-entrantly if cancel() causes an event to fire.
> if (m_loader)
>     return;

I see. That is..
- make internalAbort() return boolean (or refactor to do the same)
- if a new load is started (by inner open() call i.e. in window.onload), abort
all steps in open() (outer one) below internalAbort()
right? I'll check if this works tomorrow.

BTW, do you think it is right behavior that onload in synchronously invoked
inside domContentLoaded event?

[Nate Chapin, 2013-09-19 18:19:04]

On 2013/09/19 18:15:16, tyoshino wrote:
> On 2013/09/19 17:58:21, Nate Chapin wrote:
> > Ok, what about:
> > 
> > RefPtr<ThreadableLoader> loader = m_loader.release();
> > loader->cancel();
> > // A new load may start re-entrantly if cancel() causes an event to fire.
> > if (m_loader)
> >     return;
> 
> I see. That is..
> - make internalAbort() return boolean (or refactor to do the same)
> - if a new load is started (by inner open() call i.e. in window.onload), abort
> all steps in open() (outer one) below internalAbort()
> right? I'll check if this works tomorrow.
> 
> BTW, do you think it is right behavior that onload in synchronously invoked
> inside domContentLoaded event?

Events firing inside of events is a bad thing in general. I don't know if it
will break compatibility, but I would support forbidding onload inside of
domContentLoaded.

[Nate Chapin, 2013-09-24 18:28:05]

I believe this was fixed another way in a different issue. Can we close this
one?

[tyoshino (SeeGerritForStatus), 2013-09-24 18:29:20]

On 2013/09/24 18:28:05, Nate Chapin wrote:
> I believe this was fixed another way in a different issue. Can we close this
> one?

Yes. Closed. Thanks for review.