Oilpan: handle delayed removal of PromiseTracker promises.

Oilpan: handle delayed removal of PromiseTracker promises.

When a PromiseTracker is disabled as part of shutting down, it clears
its ID -> vector(PromiseData) map. However, once the unfulfilled
promises in that cleared map later on have their weak callbacks
invoked, we weren't prepared for the map being empty. Add the required
empty map check.

A further twist for Oilpan is that the ScopedPersistents held on the
PromiseData objects need to be cleared out eagerly to prevent v8
assuming that these objects will leak -- they won't, but will only be
cleared at the next Oilpan GC. Be more eager, and promptly clear out
the references.

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

[aandrey, 2014-09-15 12:38:43]

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

[aandrey, 2014-09-15 12:38:44]

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

https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:121: return;
let's bail out if (map.find() == map.end()) instead

[sof, 2014-09-15 12:39:11]

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

[sof, 2014-09-15 12:39:12]

Please take a look.

Reliably reproduces on the Oilpan leak bot, and with --enable-leak-detection.
(The FIXME on https://codereview.chromium.org/561753002/#msg9 still applies, of
course.)

[sof, 2014-09-15 12:40:30]

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

https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:121: return;
On 2014/09/15 12:38:44, aandrey wrote:
> let's bail out if (map.find() == map.end()) instead

That would mask a bug -- all promise hashes should have a mapping here..or the
table should be empty.

[aandrey, 2014-09-15 12:46:04]

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

[aandrey, 2014-09-15 12:46:05]

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

https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
Source/core/inspector/PromiseTracker.cpp:121: return;
On 2014/09/15 12:40:30, sof wrote:
> On 2014/09/15 12:38:44, aandrey wrote:
> > let's bail out if (map.find() == map.end()) instead
> 
> That would mask a bug -- all promise hashes should have a mapping here..or the
> table should be empty.

The PromiseTracker could be disabled and re-enabled again. AFAIU, this won't
necessarily prevent a weak callback from an old promise to execute up to this
point in Oilpan build. If my understanding is correct, the
"map.find(promiseHash)->value" will crash.

[sof, 2014-09-15 12:50:00]

On 2014/09/15 12:46:05, aandrey wrote:
>
https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
> File Source/core/inspector/PromiseTracker.cpp (right):
> 
>
https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
> Source/core/inspector/PromiseTracker.cpp:121: return;
> On 2014/09/15 12:40:30, sof wrote:
> > On 2014/09/15 12:38:44, aandrey wrote:
> > > let's bail out if (map.find() == map.end()) instead
> > 
> > That would mask a bug -- all promise hashes should have a mapping here..or
the
> > table should be empty.
> 
> The PromiseTracker could be disabled and re-enabled again. AFAIU, this won't
> necessarily prevent a weak callback from an old promise to execute up to this
> point in Oilpan build. If my understanding is correct, the
> "map.find(promiseHash)->value" will crash.

Oh, if it can, then we need to; will adjust.

(why reuse the PromiseTracker instance?)

[sof, 2014-09-15 13:13:38]

On 2014/09/15 12:50:00, sof wrote:
> On 2014/09/15 12:46:05, aandrey wrote:
> >
>
https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
> > File Source/core/inspector/PromiseTracker.cpp (right):
> > 
> >
>
https://codereview.chromium.org/571043002/diff/1/Source/core/inspector/Promis...
> > Source/core/inspector/PromiseTracker.cpp:121: return;
> > On 2014/09/15 12:40:30, sof wrote:
> > > On 2014/09/15 12:38:44, aandrey wrote:
> > > > let's bail out if (map.find() == map.end()) instead
> > > 
> > > That would mask a bug -- all promise hashes should have a mapping here..or
> the
> > > table should be empty.
> > 
> > The PromiseTracker could be disabled and re-enabled again. AFAIU, this won't
> > necessarily prevent a weak callback from an old promise to execute up to
this
> > point in Oilpan build. If my understanding is correct, the
> > "map.find(promiseHash)->value" will crash.
> 
> Oh, if it can, then we need to; will adjust.
> 
> (why reuse the PromiseTracker instance?)

Done, as that would be a possibility. Thanks much for pointing it out.

[aandrey, 2014-09-15 13:23:51]

lgtm

[aandrey, 2014-09-15 13:24:18]

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

[aandrey, 2014-09-15 13:24:18]

also need owner review

[haraken, 2014-09-15 13:32:24]

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

[haraken, 2014-09-15 13:32:25]

LGTM

(However, I think a better fix would be to use
DOMWrapperWorld::registerDOMObjectHolder. DOMObjectHolders are expected to
encapsulate this kind of complexity.)

[sof, 2014-09-15 13:46:45]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-09-15 13:47:05]

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

[commit-bot: I haz the power, 2014-09-15 14:25:51]

Committed patchset #2 (id:20001) as 181979