[Extensions] Wait until the Window object is cleared before classifying contexts

[Extensions] Wait until the Window object is cleared before classifying contexts

If the window object of a frame hasn't been cleared, then there's no way of
reliably accessing the URL, and we fail to classify the context correctly.
Wait until the window object clears, adding added contexts to pending contexts.

BUG=340382

[Devlin, 2015-11-12 01:10:44]

Description was changed from

==========
[Extensions] Wait until the Window object is cleared before classifying contexts

If the window object of a frame hasn't been cleared, then there's no way of
reliably accessing the URL, and we fail to classify the context correctly.
Wait until the window object clears, adding added contexts to pending contexts.

BUG=
==========

to

==========
[Extensions] Wait until the Window object is cleared before classifying contexts

If the window object of a frame hasn't been cleared, then there's no way of
reliably accessing the URL, and we fail to classify the context correctly.
Wait until the window object clears, adding added contexts to pending contexts.

BUG=340382
==========

[Devlin, 2015-11-12 01:12:03]

rdevlin.cronin@chromium.org changed reviewers:
+ dcheng@chromium.org

[Devlin, 2015-11-12 01:12:03]

Hey Daniel - looks like https://codereview.chromium.org/1431963002/ is shot
down.  This is plan B.

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:156: for (PendingContext* context
: pending_contexts_) {
This would be a lot cleaner if we could have a
WebFrame::getCreatedScriptContexts method - but there isn't one.  Any feelings
on adding one?

[dcheng, 2015-11-13 00:26:33]

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

[dcheng, 2015-11-13 00:26:34]

+haraken: is there a guide about v8 handles and when/how to use Global/Local
etc?

https://codereview.chromium.org/1433343003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/bindings/chromei18nrepro/iframe.js
(right):

https://codereview.chromium.org/1433343003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/bindings/chromei18nrepro/iframe.js:2: //
Use of this source code is governed by a BSD-style license that can be
Maybe use --no-find-copies? This is completely unrelated.

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:156: for (PendingContext* context
: pending_contexts_) {
On 2015/11/12 at 01:12:03, Devlin wrote:
> This would be a lot cleaner if we could have a
WebFrame::getCreatedScriptContexts method - but there isn't one.  Any feelings
on adding one?

I don't think we should add one. At the end of the day, we need this patch to
work around bad callback ordering that no one really understands how it happens.
The right long-term fix is to understand what's going on and fix the callbacks.

Since you're investigating this, any chance you can capture some stack traces
for me? I'd be interested in comparing the stack trace for when
DidCreateScriptContext is called after DidClearWindowObject vs before.

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:171: for (auto iter =
pending_contexts_.begin(); iter != pending_contexts_.end();
Maybe pending_context_ should be a map of v8::Global<v8::Context>'s to the other
info?

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.h (right):

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.h:105: ScopedVector<PendingContext>
pending_contexts_;
std::vector<scoped_ptr<PendingContext>>? =)

[haraken, 2015-11-13 12:19:45]

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

[haraken, 2015-11-13 12:19:46]

> +haraken: is there a guide about v8 handles and when/how to use Global/Local
etc?

+jochen

[Devlin, 2015-11-13 19:50:05]

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:156: for (PendingContext* context
: pending_contexts_) {
On 2015/11/13 00:26:33, dcheng wrote:
> On 2015/11/12 at 01:12:03, Devlin wrote:
> > This would be a lot cleaner if we could have a
> WebFrame::getCreatedScriptContexts method - but there isn't one.  Any feelings
> on adding one?
> 
> I don't think we should add one. At the end of the day, we need this patch to
> work around bad callback ordering that no one really understands how it
happens.
> The right long-term fix is to understand what's going on and fix the
callbacks.
> 
> Since you're investigating this, any chance you can capture some stack traces
> for me? I'd be interested in comparing the stack trace for when
> DidCreateScriptContext is called after DidClearWindowObject vs before.

Stack traces: https://paste.googleplex.com/6093038275264512

Grabbed a few sample traces of DidCreateScriptContext before vs after
DidClearWindowObject, as well as the stack traces of two script contexts for the
frame that would otherwise be incorrectly categorized (listed here as "possible
culprit").  Note that I didn't do any de-duping, so multiple stack traces could
be the same.

[jochen (gone - plz use gerrit), 2015-11-16 11:56:14]

On 2015/11/13 at 12:19:46, haraken wrote:
> > +haraken: is there a guide about v8 handles and when/how to use Global/Local
etc?
> 
> +jochen

https://developers.google.com/v8/embed#handles-and-garbage-collection

[Devlin, 2015-11-16 22:45:35]

Patchset #2 (id:20001) has been deleted

[Devlin, 2015-11-16 22:46:46]

Daniel's comments addressed.  Daniel, is this still worth landing with the other
bug filed to clean up these callbacks?  Do you have any idea when that work
might take place?

https://codereview.chromium.org/1433343003/diff/1/chrome/test/data/extensions...
File chrome/test/data/extensions/api_test/bindings/chromei18nrepro/iframe.js
(right):

https://codereview.chromium.org/1433343003/diff/1/chrome/test/data/extensions...
chrome/test/data/extensions/api_test/bindings/chromei18nrepro/iframe.js:2: //
Use of this source code is governed by a BSD-style license that can be
On 2015/11/13 00:26:33, dcheng wrote:
> Maybe use --no-find-copies? This is completely unrelated.

Done (sorry, extension folks are just used to ignoring the copy detection for
chrome/test/data)

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.cc:171: for (auto iter =
pending_contexts_.begin(); iter != pending_contexts_.end();
On 2015/11/13 00:26:33, dcheng wrote:
> Maybe pending_context_ should be a map of v8::Global<v8::Context>'s to the
other
> info?

I don't know if that would work very well... using an object like a v8 handle as
a key seems dangerous.  And since this is the only place we look up by context,
I think it's probably best as a vector.

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
File extensions/renderer/extension_frame_helper.h (right):

https://codereview.chromium.org/1433343003/diff/1/extensions/renderer/extensi...
extensions/renderer/extension_frame_helper.h:105: ScopedVector<PendingContext>
pending_contexts_;
On 2015/11/13 00:26:34, dcheng wrote:
> std::vector<scoped_ptr<PendingContext>>? =)

Yay for c++11 support!

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:51: v8::Global<v8::Context>
context;
(responding to the question of global vs local vs foo)

From the guide Jochen provided (thanks, Jochen! That actually cleared a lot up
:)) [1], it seems like a Global is the proper thing to use here.  In the guide,
this is called a UniquePersistent, and handles lifetime without needing the
HandleScope object.  And, in v8.h, UniquePersistent is an alias for Global - so
I *think* Global is the more up-to-date terminology (please correct me if I'm
wrong, Jochen).

[1] https://developers.google.com/v8/embed#handles-and-garbage-collection

[dcheng, 2015-11-18 00:57:22]

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:59: :
context(context->GetIsolate(), context),
I'd suggest just capturing context here (you can always get the isolate from it
again later)

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:158:
v8::Local<v8::Context>::New(context->isolate, context->context));
I'm assuming this is required because DidCreateScriptContext() ends up executing
script?

[dcheng, 2015-11-18 00:58:37]

Also, do extensions have anything like RenderViewTest? I'm guessing not, but if
you had that, you could write a much simpler test.

[Devlin, 2015-11-18 18:02:11]

On 2015/11/18 00:58:37, dcheng wrote:
> Also, do extensions have anything like RenderViewTest? I'm guessing not, but
if
> you had that, you could write a much simpler test.

Nothing good enough to cut it, because given what a pain this bug has been, I
think we should have a test that fully reproduces the bug.

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:59: :
context(context->GetIsolate(), context),
On 2015/11/18 00:57:22, dcheng wrote:
> I'd suggest just capturing context here (you can always get the isolate from
it
> again later)

Except that we store the context as a global - so in order to access it, we need
an isolate.  (Globals aren't like Locals where you can just use -> and have it
work.)  So we'd need an isolate in order to get a context that we could use to
call GetIsolate().

Unless I'm missing something - which, with v8 apis, is entirely possible.

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:158:
v8::Local<v8::Context>::New(context->isolate, context->context));
On 2015/11/18 00:57:22, dcheng wrote:
> I'm assuming this is required because DidCreateScriptContext() ends up
executing
> script?

Right - we set up the extension bindings, which involves at least instantiating
new objects, and I think also running a bit of (trusted) JS.

[dcheng, 2015-11-18 18:19:53]

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:59: :
context(context->GetIsolate(), context),
On 2015/11/18 at 18:02:11, Devlin wrote:
> On 2015/11/18 00:57:22, dcheng wrote:
> > I'd suggest just capturing context here (you can always get the isolate from
it
> > again later)
> 
> Except that we store the context as a global - so in order to access it, we
need an isolate.  (Globals aren't like Locals where you can just use -> and have
it work.)  So we'd need an isolate in order to get a context that we could use
to call GetIsolate().
> 
> Unless I'm missing something - which, with v8 apis, is entirely possible.

Right, makes sense. v8 is magic.

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:155: for (const auto& context :
pending_contexts_) {
Nit: s/context/pending_context/? To avoid the name collision between with the v8
context below (or I guess you could just call it v8_context).

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:156: v8::HandleScope
scope(context->isolate);
Nit: v8::Local<v8::Context> context(context->context.Get(context->isolate));

and then just use context on line 158 and line 161.

[Devlin, 2015-11-18 18:36:28]

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:155: for (const auto& context :
pending_contexts_) {
On 2015/11/18 18:19:53, dcheng wrote:
> Nit: s/context/pending_context/? To avoid the name collision between with the
v8
> context below (or I guess you could just call it v8_context).

Done.

https://codereview.chromium.org/1433343003/diff/40001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:156: v8::HandleScope
scope(context->isolate);
On 2015/11/18 18:19:53, dcheng wrote:
> Nit: v8::Local<v8::Context> context(context->context.Get(context->isolate));
> 
> and then just use context on line 158 and line 161.

heh - just what we needed - another context variable! ;)

Done.

[dcheng, 2015-11-18 18:42:51]

Basically, my main concern about this test is that it won't actually trigger the
failure condition at some point in the future and we'll lose this coverage.
That's why I asked about something simpler like a RenderViewTest (but thinking
about it more, even that has problems, since it depends on what order the
observers run in!) So this is fine.

Hopefully we make this situation better (https://crbug.com/555773 documents the
current situation around callbacks and how we can hopefully improve the
situation, but the work isn't yet scheduled).

lgtm

https://codereview.chromium.org/1433343003/diff/60001/chrome/test/data/extens...
File chrome/test/data/extensions/api_test/bindings/chromei18nrepro/window.js
(right):

https://codereview.chromium.org/1433343003/diff/60001/chrome/test/data/extens...
chrome/test/data/extensions/api_test/bindings/chromei18nrepro/window.js:8:
(background) ? 'defined' : 'undefined';
Btw, is it typical to wrap the condition in ( ) in a ternary operator?

https://codereview.chromium.org/1433343003/diff/60001/extensions/renderer/ext...
File extensions/renderer/extension_frame_helper.cc (right):

https://codereview.chromium.org/1433343003/diff/60001/extensions/renderer/ext...
extensions/renderer/extension_frame_helper.cc:158: v8::Local<v8::Context>
context_local =
Sorry: my comment was unclear. You can just call this "context" (and the struct
member can stay named context). My only concern was a collision between the loop
variable and the name of this local. I leave it up to you what looks best.