Oilpan: move ScriptLoader to the heap.

Oilpan: move ScriptLoader to the heap.

So as to be able to trace its PendingScript part object.

(Initially landed as r183909, relanded after r183984 fix.)

R=haraken
BUG=

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

[sof, 2014-10-17 11:55:18]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-10-17 11:55:19]

Please take a look.

Addressing ToT trouble. ~ScriptLoader() accessing its PendingScript looks safe,
I don't see other potential problems with having ScriptLoader be Oilpan heap
allocated.

[haraken, 2014-10-17 12:35:28]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-10-17 12:35:30]

LGTM. I'm sorry I missed this in the previous review.

https://codereview.chromium.org/660233002/diff/20001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.cpp (right):

https://codereview.chromium.org/660233002/diff/20001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.cpp:78: m_pendingScript.stopWatchingForLoad(this);

This won't touch other on-heap objects, so looks safe.

https://codereview.chromium.org/660233002/diff/20001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/660233002/diff/20001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.h:97: Element* m_element;

Can we make this a Member?

[sof, 2014-10-17 12:55:46]

Good, removed an Oilpan FIXME as a bonus :)

https://codereview.chromium.org/660233002/diff/20001/Source/core/dom/ScriptLo...
File Source/core/dom/ScriptLoader.h (right):

https://codereview.chromium.org/660233002/diff/20001/Source/core/dom/ScriptLo...
Source/core/dom/ScriptLoader.h:97: Element* m_element;
On 2014/10/17 12:35:29, haraken wrote:
> 
> Can we make this a Member?

Thanks, I overlooked that -- yes, the owning script element reference can be a
traced Member now. The two objects being GCed at the same time.

[sof, 2014-10-17 13:03:46]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-17 13:05:12]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660233002/2

[commit-bot: I haz the power, 2014-10-17 16:56:02]

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

[commit-bot: I haz the power, 2014-10-17 16:56:03]

Try jobs failed on following builders:
  win_blink_rel on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/win_blink_rel/builds/32321)

[sof, 2014-10-17 16:59:28]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-17 17:00:41]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660233002/2

[commit-bot: I haz the power, 2014-10-17 17:58:51]

Committed patchset #3 (id:2) as 183909

[dstockwell, 2014-10-19 23:16:56]

A revert of this CL (patchset #3 id:2) has been created in
https://codereview.chromium.org/656413003/ by dstockwell@chromium.org.

The reason for reverting is: Depends on
https://codereview.chromium.org/656113002 which introduced memory leaks..

[marja, 2014-10-20 09:09:57]

On 2014/10/19 23:16:56, dstockwell wrote:
> A revert of this CL (patchset #3 id:2) has been created in
> https://codereview.chromium.org/656413003/ by mailto:dstockwell@chromium.org.
> 
> The reason for reverting is: Depends on
> https://codereview.chromium.org/656113002 which introduced memory leaks..

Hey, would you like to reland this before I reland my CL?

[sof, 2014-10-20 09:16:19]

On 2014/10/20 09:09:57, marja wrote:
> On 2014/10/19 23:16:56, dstockwell wrote:
> > A revert of this CL (patchset #3 id:2) has been created in
> > https://codereview.chromium.org/656413003/ by
mailto:dstockwell@chromium.org.
> > 
> > The reason for reverting is: Depends on
> > https://codereview.chromium.org/656113002 which introduced memory leaks..
> 
> Hey, would you like to reland this before I reland my CL?

It'll have to be relanded afterwards, I think. I'll just watch out for 656113002
and do that?

[haraken, 2014-10-20 14:51:07]

On 2014/10/20 09:16:19, sof wrote:
> On 2014/10/20 09:09:57, marja wrote:
> > On 2014/10/19 23:16:56, dstockwell wrote:
> > > A revert of this CL (patchset #3 id:2) has been created in
> > > https://codereview.chromium.org/656413003/ by
> mailto:dstockwell@chromium.org.
> > > 
> > > The reason for reverting is: Depends on
> > > https://codereview.chromium.org/656113002 which introduced memory leaks..
> > 
> > Hey, would you like to reland this before I reland my CL?
> 
> It'll have to be relanded afterwards, I think. I'll just watch out for
656113002
> and do that?

Now that marja's CL was landed, it's time to land sof's CL.

[sof, 2014-10-20 14:55:46]

On 2014/10/20 09:16:19, sof wrote:
> On 2014/10/20 09:09:57, marja wrote:
> > On 2014/10/19 23:16:56, dstockwell wrote:
> > > A revert of this CL (patchset #3 id:2) has been created in
> > > https://codereview.chromium.org/656413003/ by
> mailto:dstockwell@chromium.org.
> > > 
> > > The reason for reverting is: Depends on
> > > https://codereview.chromium.org/656113002 which introduced memory leaks..
> > 
> > Hey, would you like to reland this before I reland my CL?
> 
> It'll have to be relanded afterwards, I think. I'll just watch out for
656113002
> and do that?

With r183984 relanded, this can go ahead. But, all the detaching now done in
~ScriptRunner needs to be recast somehow.

[sof, 2014-10-20 18:21:35]

On 2014/10/20 14:55:46, sof wrote:
> On 2014/10/20 09:16:19, sof wrote:
> > On 2014/10/20 09:09:57, marja wrote:
> > > On 2014/10/19 23:16:56, dstockwell wrote:
> > > > A revert of this CL (patchset #3 id:2) has been created in
> > > > https://codereview.chromium.org/656413003/ by
> > mailto:dstockwell@chromium.org.
> > > > 
> > > > The reason for reverting is: Depends on
> > > > https://codereview.chromium.org/656113002 which introduced memory
leaks..
> > > 
> > > Hey, would you like to reland this before I reland my CL?
> > 
> > It'll have to be relanded afterwards, I think. I'll just watch out for
> 656113002
> > and do that?
> 
> With r183984 relanded, this can go ahead. But, all the detaching now done in
> ~ScriptRunner needs to be recast somehow.

It's not needed for Oilpan, as the objects die together (i.e.,
Member<ScriptLoader> is kept from ScriptRunner.)

Verified locally not to leak.

[sof, 2014-10-20 18:23:30]

Will reland this right away to heal the Oilpan tree.

haraken: if you can have a look at ScriptRunner whenever you next have the time,
that'd be great.

[sof, 2014-10-20 18:23:44]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-20 18:24:39]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/660233002/70001

[commit-bot: I haz the power, 2014-10-20 20:39:46]

Committed patchset #5 (id:70001) as 184013