Oilpan: fix PromiseTracker's handling of its persistent promise wrappers.

Oilpan: fix PromiseTracker's handling of its persistent promise wrappers.

The handling of the PromiseDataWrapper in r181662 wasn't complete, as it
failed to keep an Oilpan-reachable persistent to the GCed wrapper object,
risking premature finalization. For Oilpan also, this wrapper object should 
only keep its references weakly alive.

To make the tracing of these relationships work out, PromiseTracker is
no longer a part object, but heap allocated.

R=haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=181845

[sof, 2014-09-10 13:48:09]

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

[sof, 2014-09-10 13:48:09]

Please take a look.

A crasher due to attempting to keep a WeakMember<> directly to the
PromiseTracker's internal map had me stumped for a while. Ended up making
PromiseTracker no longer be a part object.

[aandrey, 2014-09-10 14:42:17]

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

[aandrey, 2014-09-10 14:42:18]

FYI

https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/Promis...
File Source/core/inspector/PromiseTracker.cpp (right):

https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:112: if (!vector->size())
if (vector->isEmpty())

https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:125:
PromiseDataWrapper(WeakPtrWillBeRawPtr<PromiseTracker::PromiseData> data, 
PromiseTracker* tracker)
nit: extra space before PromiseTracker*

[haraken, 2014-09-10 15:54:27]

Hmm, the wrapper handling looks quite complicated.

Can we simplify the wrapper handling by using
DOMWrapperWorld::registerDOMObjectHolder()? By using
DOMWrapperWorld::registerDOMObjectHolder(object, wrapper), you can keep the
object alive as long as the wrapper is alive.

[sof, 2014-09-11 05:20:53]

https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/Promis...
File Source/core/inspector/PromiseTracker.cpp (right):

https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:112: if (!vector->size())
On 2014/09/10 14:42:18, aandrey wrote:
> if (vector->isEmpty())

Done.

https://codereview.chromium.org/561753002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:125:
PromiseDataWrapper(WeakPtrWillBeRawPtr<PromiseTracker::PromiseData> data, 
PromiseTracker* tracker)
On 2014/09/10 14:42:18, aandrey wrote:
> nit: extra space before PromiseTracker*

Well spotted :)

[sof, 2014-09-11 06:56:28]

On 2014/09/10 15:54:27, haraken wrote:
> Hmm, the wrapper handling looks quite complicated.
> 
> Can we simplify the wrapper handling by using
> DOMWrapperWorld::registerDOMObjectHolder()? By using
> DOMWrapperWorld::registerDOMObjectHolder(object, wrapper), you can keep the
> object alive as long as the wrapper is alive.

This sounds like a PromiseTracker follow up task; switching over doesn't appear
to be readily doable.

[haraken, 2014-09-11 13:46:57]

On 2014/09/11 06:56:28, sof wrote:
> On 2014/09/10 15:54:27, haraken wrote:
> > Hmm, the wrapper handling looks quite complicated.
> > 
> > Can we simplify the wrapper handling by using
> > DOMWrapperWorld::registerDOMObjectHolder()? By using
> > DOMWrapperWorld::registerDOMObjectHolder(object, wrapper), you can keep the
> > object alive as long as the wrapper is alive.
> 
> This sounds like a PromiseTracker follow up task; switching over doesn't
appear
> to be readily doable.

LGTM.

However I still think that we should consider using
DOMWrapperWorld::registerDOMObjectHolder(). It might require some improvement on
DOMWrapperWorld::registerDOMObjectHolder, but it will enable us to remove the
complexity about the PromiseDataWrapper handling and eliminate #if
ENABLE(OILPAN).

[sof, 2014-09-11 14:42:22]

On 2014/09/11 13:46:57, haraken wrote:
> On 2014/09/11 06:56:28, sof wrote:
> > On 2014/09/10 15:54:27, haraken wrote:
> > > Hmm, the wrapper handling looks quite complicated.
> > > 
> > > Can we simplify the wrapper handling by using
> > > DOMWrapperWorld::registerDOMObjectHolder()? By using
> > > DOMWrapperWorld::registerDOMObjectHolder(object, wrapper), you can keep
the
> > > object alive as long as the wrapper is alive.
> > 
> > This sounds like a PromiseTracker follow up task; switching over doesn't
> appear
> > to be readily doable.
> 
> LGTM.
> 
> However I still think that we should consider using
> DOMWrapperWorld::registerDOMObjectHolder(). It might require some improvement
on
> DOMWrapperWorld::registerDOMObjectHolder, but it will enable us to remove the
> complexity about the PromiseDataWrapper handling and eliminate #if
> ENABLE(OILPAN).

Thanks, not suggesting it should be end-of-story. But addressing this flake


http://test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRu...

first makes sense.

Added a FIXME; if anyone familiar with PromiseTracker and/or DOMWrapperWorld
wants to take care of it, please do. (I'm not on either counts.)

[sof, 2014-09-11 14:42:41]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-11 14:42:54]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561753002/60001

[commit-bot: I haz the power, 2014-09-11 16:12:43]

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

[commit-bot: I haz the power, 2014-09-11 16:12:44]

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

[sof, 2014-09-11 17:01:30]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-11 17:02:26]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561753002/60001

[sof, 2014-09-11 18:16:18]

The CQ bit was unchecked by sigbjornf@opera.com

[sof, 2014-09-11 18:18:13]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-11 18:18:45]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patchset/561753002/60001

[commit-bot: I haz the power, 2014-09-11 19:56:07]

Committed patchset #4 (id:60001) as 181845