Fix re-entrancy and lifetime issue in RenderFrameObserverNatives::OnDocumentElementCreated

Fix re-entrancy and lifetime issue in RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268, 568130
Committed: https://crrev.com/5a15b72a270b514cd442872221a788a303bdaa88
Cr-Commit-Position: refs/heads/master@{#374758}

[robwu, 2016-02-10 18:14:59]

Description was changed from

==========
Fix re-entrancy in RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268
==========

to

==========
Fix re-entrancy and lifetime issue in
RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268,568130
==========

[robwu, 2016-02-10 18:14:59]

rob@robwu.nl changed reviewers:
+ rdevlin.cronin@chromium.org

[robwu, 2016-02-10 18:22:01]

Hi Devlin! See https://crbug.com/585268#c6

[Devlin, 2016-02-10 19:04:25]

lgtm

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
File extensions/renderer/render_frame_observer_natives.cc (right):

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:22:
LoadWatcher(content::RenderFrame* frame, base::Callback<void(bool)> callback)
nit: const& on the callback.

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:83:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&callback),
nit: I think this might be cleaner if we cache the callback, i.e.:
v8::Global<v8::Function> v8_callback(context()->isolate(),
                                     args[1].As<v8::Function>());
base::Callback<void(bool)> callback(
    base::Bind(&RenderFrameObserverNatives::InvokeCallback,
               weak_ptr_factory_.GetWeakPtr(),
               base::Passed(&v8_callback));
if (did_create_doc_element)
  base::MessageLoop::current()->PostTask(FROM_HERE, callback.Run(true));
else
  new LoadWatcher(frame, callback);

It makes it more clear that we're using the same function in both places, extra
assurance we don't base::Passed the v8 callback multiple times, and is just a
little easier to read IMO.

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:97: v8::Isolate* isolate =
context()->isolate();
Is there any chance that the context could be invalid at this time?  I don't
*think* so, but clearly this has been biting us lately.  WDYT of putting a
CHECK(is_valid()) here?

[robwu, 2016-02-10 20:18:52]

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
File extensions/renderer/render_frame_observer_natives.cc (right):

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:22:
LoadWatcher(content::RenderFrame* frame, base::Callback<void(bool)> callback)
On 2016/02/10 19:04:25, Devlin wrote:
> nit: const& on the callback.

Done.

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:83:
weak_ptr_factory_.GetWeakPtr(), base::Passed(&callback),
On 2016/02/10 19:04:25, Devlin wrote:
> nit: I think this might be cleaner if we cache the callback, i.e.:
> v8::Global<v8::Function> v8_callback(context()->isolate(),
>                                      args[1].As<v8::Function>());
> base::Callback<void(bool)> callback(
>     base::Bind(&RenderFrameObserverNatives::InvokeCallback,
>                weak_ptr_factory_.GetWeakPtr(),
>                base::Passed(&v8_callback));
> if (did_create_doc_element)
>   base::MessageLoop::current()->PostTask(FROM_HERE, callback.Run(true));
> else
>   new LoadWatcher(frame, callback);
> 
> It makes it more clear that we're using the same function in both places,
extra
> assurance we don't base::Passed the v8 callback multiple times, and is just a
> little easier to read IMO.

Done.

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:97: v8::Isolate* isolate =
context()->isolate();
On 2016/02/10 19:04:25, Devlin wrote:
> Is there any chance that the context could be invalid at this time?  I don't
> *think* so, but clearly this has been biting us lately.  WDYT of putting a
> CHECK(is_valid()) here?

After your comment, I checked again when context() and |this| are exactly
invalidated.
context() is invalidated at the end of ScriptContext::Invalidate().
Before that, ModuleSystem is "invalidated", which means that the Invalidate()
method is called for all native handlers. I don't see an explicit destruction
anywhere, so my patch so far was not sufficient.

I've now added an Invalidate() override that explicitly invalidates the weak
pointers. With that, there is no need for a CHECK(is_valid()), because
base::Callback guarantees that callback with invalid pointers are not called.

[Devlin, 2016-02-10 20:30:48]

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
File extensions/renderer/render_frame_observer_natives.cc (right):

https://codereview.chromium.org/1684953002/diff/20001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:97: v8::Isolate* isolate =
context()->isolate();
On 2016/02/10 20:18:52, robwu wrote:
> On 2016/02/10 19:04:25, Devlin wrote:
> > Is there any chance that the context could be invalid at this time?  I don't
> > *think* so, but clearly this has been biting us lately.  WDYT of putting a
> > CHECK(is_valid()) here?
> 
> After your comment, I checked again when context() and |this| are exactly
> invalidated.
> context() is invalidated at the end of ScriptContext::Invalidate().
> Before that, ModuleSystem is "invalidated", which means that the Invalidate()
> method is called for all native handlers. I don't see an explicit destruction
> anywhere, so my patch so far was not sufficient.

The explicit destruction would happen with ScriptContextSet::Remove(), which
also calls Invalidate().  So I think the only way this is unsafe would be if
DidCreate/DidFail are called after the script context is released - which
hopefully wouldn't happen, but probably could given all of our post task
craziness + blink's general lifetime craziness.

> I've now added an Invalidate() override that explicitly invalidates the weak
> pointers. With that, there is no need for a CHECK(is_valid()), because
> base::Callback guarantees that callback with invalid pointers are not called.

That works too - I was torn between that and the CHECK, and figured it might be
worth seeing if it happens (because if it doesn't, it's good to assert that). 
But being defensive is probably fine in this case.

https://codereview.chromium.org/1684953002/diff/40001/extensions/renderer/ren...
File extensions/renderer/render_frame_observer_natives.cc (right):

https://codereview.chromium.org/1684953002/diff/40001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:80:
v8::Global<v8::Function> v8_callback(context()->isolate(),
Something else I just thought of:
v8::Globals are refcounted - does the fact that this is passed to a callback
that can outlive the RenderFrameObserverNatives mean that we could be
artificially prolonging the life of a context or isolate, since those are passed
to the function?

[robwu, 2016-02-10 20:45:49]

https://codereview.chromium.org/1684953002/diff/40001/extensions/renderer/ren...
File extensions/renderer/render_frame_observer_natives.cc (right):

https://codereview.chromium.org/1684953002/diff/40001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:80:
v8::Global<v8::Function> v8_callback(context()->isolate(),
On 2016/02/10 20:30:47, Devlin wrote:
> Something else I just thought of:
> v8::Globals are refcounted - does the fact that this is passed to a callback
> that can outlive the RenderFrameObserverNatives mean that we could be
> artificially prolonging the life of a context or isolate, since those are
passed
> to the function?

I don't know, I'm a complete noob with V8 (I just learned about its API
yesterday, to fix this bug).

The lifespan of a RenderFrame and its RenderFrameObserverNative (owned by the
ModuleSystem) are quite similar, so it shouldn't matter in practice.

But if you come across a V8 guru who knows the answer, please let me know. I'm
curious as well :)

[Devlin, 2016-02-10 20:59:00]

Jochen, mind helping us out with the comment in
render_frame_observer_natives.cc?

https://codereview.chromium.org/1684953002/diff/40001/extensions/renderer/ren...
File extensions/renderer/render_frame_observer_natives.cc (right):

https://codereview.chromium.org/1684953002/diff/40001/extensions/renderer/ren...
extensions/renderer/render_frame_observer_natives.cc:80:
v8::Global<v8::Function> v8_callback(context()->isolate(),
On 2016/02/10 20:45:49, robwu wrote:
> On 2016/02/10 20:30:47, Devlin wrote:
> > Something else I just thought of:
> > v8::Globals are refcounted - does the fact that this is passed to a callback
> > that can outlive the RenderFrameObserverNatives mean that we could be
> > artificially prolonging the life of a context or isolate, since those are
> passed
> > to the function?
> 
> I don't know, I'm a complete noob with V8 (I just learned about its API
> yesterday, to fix this bug).
> 
> The lifespan of a RenderFrame and its RenderFrameObserverNative (owned by the
> ModuleSystem) are quite similar, so it shouldn't matter in practice.
> 
> But if you come across a V8 guru who knows the answer, please let me know. I'm
> curious as well :)

Let's go with this for now - it's certainly better than crashing, and I'm sure
it's not the only place with a possible-maybe-leak. But cc'd jochen@ (resident
v8 guru) to educate us when's he's back in.

In the meantime,
https://developers.google.com/v8/embed#handles-and-garbage-collection offers
some good information, but doesn't really go into much detail in many areas.

[robwu, 2016-02-10 22:15:18]

The CQ bit was checked by rob@robwu.nl

[robwu, 2016-02-10 22:15:18]

The patchset sent to the CQ was uploaded after l-g-t-m from
rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1684953002/#ps40001
(title: "Nits + Invalidate weak ptrs upon Invalidate()")

[commit-bot: I haz the power, 2016-02-10 22:18:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1684953002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1684953002/40001

[commit-bot: I haz the power, 2016-02-10 22:46:06]

Description was changed from

==========
Fix re-entrancy and lifetime issue in
RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268,568130
==========

to

==========
Fix re-entrancy and lifetime issue in
RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268,568130
==========

[commit-bot: I haz the power, 2016-02-10 22:46:07]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-02-16 22:31:56]

Description was changed from

==========
Fix re-entrancy and lifetime issue in
RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268,568130
==========

to

==========
Fix re-entrancy and lifetime issue in
RenderFrameObserverNatives::OnDocumentElementCreated

BUG=585268,568130

Committed: https://crrev.com/5a15b72a270b514cd442872221a788a303bdaa88
Cr-Commit-Position: refs/heads/master@{#374758}
==========

[commit-bot: I haz the power, 2016-02-16 22:31:56]

Patchset 3 (id:??) landed as
https://crrev.com/5a15b72a270b514cd442872221a788a303bdaa88
Cr-Commit-Position: refs/heads/master@{#374758}