Oilpan: Move core/xml/ to oilpan's heap

Oilpan: Move core/xml/ to oilpan's heap

This CL moves DOMParser, XMLHttpRequest, XMLSerializer, XPathEvaluator, XPathExpression, XPathNSResolver, XPathResult, XSLTProcessor to oilpan's heap.

XMLHttpRequest can be used by workers, so I added XMLHttpRequest to USED_FROM_MULTIPLE_THREADS. I'm not sure if workers can touch XMLSerializer, XPathResult etc, but I added them to USED_FROM_MULTIPLE_THREADS just in case. (We'll remove USED_FROM_MULTIPLE_THREADS soon.)

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

[haraken, 2014-02-19 06:24:21]

PTAL

[Mads Ager (chromium), 2014-02-19 10:24:29]

LGTM

https://codereview.chromium.org/166903010/diff/80001/Source/core/xml/XPathPar...
File Source/core/xml/XPathParser.h (right):

https://codereview.chromium.org/166903010/diff/80001/Source/core/xml/XPathPar...
Source/core/xml/XPathParser.h:65: class Parser {
I think parser is actually stack allocated? In that case we should be able to
put in STACK_ALLOCATED here and replace the Persistent will a RawPtr.

[haraken, 2014-02-19 10:40:51]

https://codereview.chromium.org/166903010/diff/80001/Source/core/xml/XPathPar...
File Source/core/xml/XPathParser.h (right):

https://codereview.chromium.org/166903010/diff/80001/Source/core/xml/XPathPar...
Source/core/xml/XPathParser.h:65: class Parser {
On 2014/02/19 10:24:30, Mads Ager (chromium) wrote:
> I think parser is actually stack allocated? In that case we should be able to
> put in STACK_ALLOCATED here and replace the Persistent will a RawPtr.

STACK_ALLOCATED is not yet available, so I added a DISALLOW_ALLOCATION and a
FIXME.

[haraken, 2014-02-19 10:41:00]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-19 10:41:09]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/166903010/150001

[commit-bot: I haz the power, 2014-02-19 11:07:00]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 11:07:00]

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

[haraken, 2014-02-19 11:37:18]

The CQ bit was checked by haraken@chromium.org

[commit-bot: I haz the power, 2014-02-19 11:37:26]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/166903010/230001

[commit-bot: I haz the power, 2014-02-19 12:00:56]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 12:00:57]

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

[haraken, 2014-02-19 12:11:28]

tkent@ or jochen@: Would you rubberstamp web/?

[pfeldman, 2014-02-19 12:12:53]

web/ rslgtm

[haraken, 2014-02-19 12:21:57]

On 2014/02/19 12:12:53, pfeldman wrote:
> web/ rslgtm

Thanks!

[haraken, 2014-02-19 12:22:03]

The CQ bit was checked by haraken@chromium.org

[sof, 2014-02-19 12:22:10]

https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.h:57: class XMLHttpRequest FINAL : public
RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, public
ScriptWrappable, public XMLHttpRequestEventTarget, private
ThreadableLoaderClient, public ActiveDOMObject {
Just making a note of it here.. a followup here is to have the upload object be
a member instead (and it now being able to have a member reference back.)

[commit-bot: I haz the power, 2014-02-19 12:22:16]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/166903010/230001

[haraken, 2014-02-19 12:26:50]

https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
File Source/core/xml/XMLHttpRequest.h (right):

https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
Source/core/xml/XMLHttpRequest.h:57: class XMLHttpRequest FINAL : public
RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, public
ScriptWrappable, public XMLHttpRequestEventTarget, private
ThreadableLoaderClient, public ActiveDOMObject {
On 2014/02/19 12:22:11, sof wrote:
> Just making a note of it here.. a followup here is to have the upload object
be
> a member instead (and it now being able to have a member reference back.)

Nice catch. I'll move XMLHttpRequestEventTarget and XMLHttpRequestUpload in the
next CL.

[Paweł Hajdan Jr., 2014-02-19 19:33:15]

The CQ bit was unchecked by phajdan.jr@chromium.org

[Paweł Hajdan Jr., 2014-02-19 19:35:53]

The CQ bit was checked by phajdan.jr@chromium.org

[commit-bot: I haz the power, 2014-02-19 21:55:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/166903010/230001

[commit-bot: I haz the power, 2014-02-19 23:01:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/haraken@chromium.org/166903010/230001

[commit-bot: I haz the power, 2014-02-20 07:04:03]

Change committed as 167459

[haraken, 2014-02-21 06:39:16]

On 2014/02/19 12:22:10, sof wrote:
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> File Source/core/xml/XMLHttpRequest.h (right):
> 
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> Source/core/xml/XMLHttpRequest.h:57: class XMLHttpRequest FINAL : public
> RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, public
> ScriptWrappable, public XMLHttpRequestEventTarget, private
> ThreadableLoaderClient, public ActiveDOMObject {
> Just making a note of it here.. a followup here is to have the upload object
be
> a member instead (and it now being able to have a member reference back.)

I noticed that we need to move EventTarget to the heap before moving
XMLHttpRequestUpload to the heap, because XMLHttpRequestUpload inherits from
EventTarget.

[Mads Ager (chromium), 2014-02-21 08:03:40]

On 2014/02/21 06:39:16, haraken wrote:
> On 2014/02/19 12:22:10, sof wrote:
> >
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> > File Source/core/xml/XMLHttpRequest.h (right):
> > 
> >
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> > Source/core/xml/XMLHttpRequest.h:57: class XMLHttpRequest FINAL : public
> > RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, public
> > ScriptWrappable, public XMLHttpRequestEventTarget, private
> > ThreadableLoaderClient, public ActiveDOMObject {
> > Just making a note of it here.. a followup here is to have the upload object
> be
> > a member instead (and it now being able to have a member reference back.)
> 
> I noticed that we need to move EventTarget to the heap before moving
> XMLHttpRequestUpload to the heap, because XMLHttpRequestUpload inherits from
> EventTarget.

You could move it to the heap using RefCountedGarbageCollected if you want to,
right? We have moved multiple things that are event targets already I believe?

[haraken, 2014-02-21 09:03:03]

On 2014/02/21 08:03:40, Mads Ager (chromium) wrote:
> On 2014/02/21 06:39:16, haraken wrote:
> > On 2014/02/19 12:22:10, sof wrote:
> > >
> >
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> > > File Source/core/xml/XMLHttpRequest.h (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> > > Source/core/xml/XMLHttpRequest.h:57: class XMLHttpRequest FINAL : public
> > > RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, public
> > > ScriptWrappable, public XMLHttpRequestEventTarget, private
> > > ThreadableLoaderClient, public ActiveDOMObject {
> > > Just making a note of it here.. a followup here is to have the upload
object
> > be
> > > a member instead (and it now being able to have a member reference back.)
> > 
> > I noticed that we need to move EventTarget to the heap before moving
> > XMLHttpRequestUpload to the heap, because XMLHttpRequestUpload inherits from
> > EventTarget.
> 
> You could move it to the heap using RefCountedGarbageCollected if you want to,
> right? We have moved multiple things that are event targets already I believe?

We could, but I have one question.

What we've moved so far has the following class shape:

  // before
  class A : public RefCounted<A>, public EventTarget { };

  // after
  class A : public RefCountedGarbageCollected<A>, public EventTarget { };

On the other hand, in case of XMLHttpRequestUpload, we have the following shape:

  // before
  class XMLHttpRequested : public EventTarget { };

  // after
  class XMLHttpRequested : RefCountedGarbageCollected<XMLHttpRequested>, public
EventTarget { };

In other words, we need to add RefCountedGarbageCollected as a new parent class.
Won't it cause any issue? (I'm not sure.)

[Mads Ager (chromium), 2014-02-21 09:13:32]

On 2014/02/21 09:03:03, haraken wrote:
> On 2014/02/21 08:03:40, Mads Ager (chromium) wrote:
> > On 2014/02/21 06:39:16, haraken wrote:
> > > On 2014/02/19 12:22:10, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> > > > File Source/core/xml/XMLHttpRequest.h (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/166903010/diff/230001/Source/core/xml/XMLHttp...
> > > > Source/core/xml/XMLHttpRequest.h:57: class XMLHttpRequest FINAL : public
> > > > RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, public
> > > > ScriptWrappable, public XMLHttpRequestEventTarget, private
> > > > ThreadableLoaderClient, public ActiveDOMObject {
> > > > Just making a note of it here.. a followup here is to have the upload
> object
> > > be
> > > > a member instead (and it now being able to have a member reference
back.)
> > > 
> > > I noticed that we need to move EventTarget to the heap before moving
> > > XMLHttpRequestUpload to the heap, because XMLHttpRequestUpload inherits
from
> > > EventTarget.
> > 
> > You could move it to the heap using RefCountedGarbageCollected if you want
to,
> > right? We have moved multiple things that are event targets already I
believe?
> 
> We could, but I have one question.
> 
> What we've moved so far has the following class shape:
> 
>   // before
>   class A : public RefCounted<A>, public EventTarget { };
> 
>   // after
>   class A : public RefCountedGarbageCollected<A>, public EventTarget { };
> 
> On the other hand, in case of XMLHttpRequestUpload, we have the following
shape:
> 
>   // before
>   class XMLHttpRequested : public EventTarget { };
> 
>   // after
>   class XMLHttpRequested : RefCountedGarbageCollected<XMLHttpRequested>,
public
> EventTarget { };
> 
> In other words, we need to add RefCountedGarbageCollected as a new parent
class.
> Won't it cause any issue? (I'm not sure.)

OK, yeah I didn't look at the actual class in question. XMLHttpRequestUpload is
owned by the XMLHttpRequest and reference counting of the XMLHttpRequestUpload
objects does not ref the upload object but the owning XMLHttpRequest. What that
means for Oilpan is that these two objects live and die together. Therefore,
they should both be in the heap and have a member pointer to eachother.
Therefore, XMLHttpRequestUpload should probably be
NoBaseWillBeReferenceCountedGarbageCollected until we have moved all event
targets to the oilpan heap.

class XMLHttpRequest : public
RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, ... {
    ...
private:
    OwnPtrWillBeMember<XMLHttpRequestUpload> m_upload;
};

class XMLHttpRequestUpload : public
NoBaseWillBeRefCountedGarbageCollected<XMLHttpRequestUpload>, ... {
    ...
private:
    RawPtrWillBeMember<XMLHttpRequest> m_xmlHttpRequest;
};

[haraken, 2014-02-21 09:26:32]

> OK, yeah I didn't look at the actual class in question. XMLHttpRequestUpload
is
> owned by the XMLHttpRequest and reference counting of the XMLHttpRequestUpload
> objects does not ref the upload object but the owning XMLHttpRequest. What
that
> means for Oilpan is that these two objects live and die together. Therefore,
> they should both be in the heap and have a member pointer to eachother.
> Therefore, XMLHttpRequestUpload should probably be
> NoBaseWillBeReferenceCountedGarbageCollected until we have moved all event
> targets to the oilpan heap.
> 
> class XMLHttpRequest : public
> RefCountedWillBeRefCountedGarbageCollected<XMLHttpRequest>, ... {
>     ...
> private:
>     OwnPtrWillBeMember<XMLHttpRequestUpload> m_upload;
> };
> 
> class XMLHttpRequestUpload : public
> NoBaseWillBeRefCountedGarbageCollected<XMLHttpRequestUpload>, ... {
>     ...
> private:
>     RawPtrWillBeMember<XMLHttpRequest> m_xmlHttpRequest;
> };

Sounds reasonable. I'll write a CL.

[haraken, 2014-02-24 05:32:01]

sof: BTW, what's the benefit of moving XMLHttpRequestUpload to the heap at this
point? AFAICT, it will just complicate the code base. Probably it would be
better to move XMLHttpRequestUpload after moving EventTarget to the heap,
wouldn't it?

[sof, 2014-02-24 06:28:58]

On 2014/02/24 05:32:01, haraken wrote:
> sof: BTW, what's the benefit of moving XMLHttpRequestUpload to the heap at
this
> point? AFAICT, it will just complicate the code base. Probably it would be
> better to move XMLHttpRequestUpload after moving EventTarget to the heap,
> wouldn't it?

That's a good plan, little point moving it if it ends up being essentially ref
counted for now.

If not already, leave a FIXME next to XMLHttpRequestUpload (next to its class
declaration or its use on XHR)?

[haraken, 2014-02-24 06:36:58]

On 2014/02/24 06:28:58, sof wrote:
> On 2014/02/24 05:32:01, haraken wrote:
> > sof: BTW, what's the benefit of moving XMLHttpRequestUpload to the heap at
> this
> > point? AFAICT, it will just complicate the code base. Probably it would be
> > better to move XMLHttpRequestUpload after moving EventTarget to the heap,
> > wouldn't it?
> 
> That's a good plan, little point moving it if it ends up being essentially ref
> counted for now.
> 
> If not already, leave a FIXME next to XMLHttpRequestUpload (next to its class
> declaration or its use on XHR)?

Sure, will do in the next CL.