Stop injection when injections are invalidated

Stop injection when injections are invalidated

BUG=625393

[robwu, 2016-07-08 09:12:36]

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

[robwu, 2016-07-08 09:12:37]

Tested locally using the same steps as the other patch.

[Devlin, 2016-07-08 20:51:09]

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection.cc:225: if (!injection_host_)
A comment explaining why/how this could happen would be useful.

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection_manager.cc:446: ScriptInjectionWatcher
watcher(injection.get(), this);
Hmm... I think this isn't quite sufficient.  Consider the following scenario:
Extensions A and B both want to inject on a page.
Extension A injects, and blocks.
During blocking, Extension B is removed
  We only have a watcher for Extension A (the injecting one)
Extension B starts injecting, and blows up

Unless I'm missing some way that's mitigated?

Instead, I think we have to keep track of both pending and active injections
during these times, and notify both of extension host removal.  This will
probably render the Watcher class unnecessary.

[robwu, 2016-07-09 05:22:02]

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection.cc (right):

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection.cc:225: if (!injection_host_)
On 2016/07/08 20:51:09, Devlin wrote:
> A comment explaining why/how this could happen would be useful.

Done.

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_...
extensions/renderer/script_injection_manager.cc:446: ScriptInjectionWatcher
watcher(injection.get(), this);
On 2016/07/08 20:51:09, Devlin wrote:
> Hmm... I think this isn't quite sufficient.  Consider the following scenario:
> Extensions A and B both want to inject on a page.
> Extension A injects, and blocks.
> During blocking, Extension B is removed
>   We only have a watcher for Extension A (the injecting one)
> Extension B starts injecting, and blows up
> 
> Unless I'm missing some way that's mitigated?

You're right - I solved the issue by constructing a queue of
ScriptInjectionWatchers. I also moved the frame logic to
active_injection_frames_, so that we can have an early out in the
ScriptInjection::Inject (similarly to |injection_host_|).

> Instead, I think we have to keep track of both pending and active injections
> during these times, and notify both of extension host removal.  This will
> probably render the Watcher class unnecessary.

I added the class to avoid spreeing push_back with a balanced find & erase all
over the place. With the watcher class, the watcher is set up when it's
constructed and automatically removed upon destruction.

[Devlin, 2016-07-11 23:30:55]

https://codereview.chromium.org/2134613002/diff/20001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/2134613002/diff/20001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:110: class
ScriptInjectionManager::ScriptInjectionWatcher {
I'm still not quite happy with this implementation.  It seems a little like it's
more complicated than it should be (I'm worried we're going to forget to put one
of these somewhere, etc), while also not fully addressing the concerns (e.g.
what if a ScriptInjection blocks, and finishes asynchronously? The injection is
stored in running_injections_, but it doesn't look like we'll update the
extension or frame for those).

I wonder if there's a way that we can put a little more of this logic into the
ScriptInjection/InjectionHost.  For one thing, we now have the
RendererExtensionRegistry(), which we could check for the presence of the
extension.  Then rather than having these updates, we can just check
injection_host_->IsValid() at set times throughout the injection process. 
Unfortunately, that doesn't answer the question of frames.

I'll think on this a bit more and circle back, but if you had any thoughts,
happy to hear them. :)

[robwu, 2016-07-12 08:59:36]

https://codereview.chromium.org/2134613002/diff/20001/extensions/renderer/scr...
File extensions/renderer/script_injection_manager.cc (right):

https://codereview.chromium.org/2134613002/diff/20001/extensions/renderer/scr...
extensions/renderer/script_injection_manager.cc:110: class
ScriptInjectionManager::ScriptInjectionWatcher {
On 2016/07/11 23:30:55, Devlin wrote:
> I'm still not quite happy with this implementation.  It seems a little like
it's
> more complicated than it should be (I'm worried we're going to forget to put
one
> of these somewhere, etc), while also not fully addressing the concerns (e.g.
> what if a ScriptInjection blocks, and finishes asynchronously? The injection
is
> stored in running_injections_, but it doesn't look like we'll update the
> extension or frame for those).

We only need to be careful when InjectJs() is called.
Currently, when a script is *not* being injected, it is stored in
|pending_injections_| or |running_injections_|. If it *is* about to be injected,
there will always be a ScriptInjectionWatcher on the stack, which was designed
to be resilient against unexpected reentrancy (with respect to the same
frame/injection).


> I wonder if there's a way that we can put a little more of this logic into the
> ScriptInjection/InjectionHost.  For one thing, we now have the
> RendererExtensionRegistry(), which we could check for the presence of the
> extension.  Then rather than having these updates, we can just check
> injection_host_->IsValid() at set times throughout the injection process. 
> Unfortunately, that doesn't answer the question of frames.

Scripts injection should be aborted in the following cases:

1. Extension unloads.
2. Extension reloads.
3. Frame unloads (includes frame reloads).

Using RendererExtensionRegistry seems to only cover case 1, not case 2.

> I'll think on this a bit more and circle back, but if you had any thoughts,
happy to hear them. :)

I only have half-baked thoughts for now. The issue is that InjectionHost and
ScriptInjection can outlive each other, so there needs to be some mediator that
outlives both (or some token that is reset whenever either of the invalidation
events occur). I found this role in the UserScriptManager (and the new watcher).