Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(54)

Issue 2134613002: Stop injection when injections are invalidated

Created:
3 years, 2 months ago by robwu
Modified:
3 years, 2 months ago
Reviewers:
Devlin
CC:
chromium-reviews, chromium-apps-reviews_chromium.org, extensions-reviews_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Stop injection when injections are invalidated BUG=625393

Patch Set 1 #

Total comments: 4

Patch Set 2 : Expand scope of ScriptInjectionWatchers #

Total comments: 2
Unified diffs Side-by-side diffs Delta from patch set Stats (+112 lines, -33 lines) Patch
M extensions/renderer/script_injection.h View 2 chunks +3 lines, -0 lines 0 comments Download
M extensions/renderer/script_injection.cc View 1 5 chunks +13 lines, -3 lines 0 comments Download
M extensions/renderer/script_injection_manager.h View 1 3 chunks +5 lines, -3 lines 0 comments Download
M extensions/renderer/script_injection_manager.cc View 1 12 chunks +82 lines, -12 lines 2 comments Download
M extensions/renderer/user_script_injector.cc View 5 chunks +9 lines, -15 lines 0 comments Download

Messages

Total messages: 6 (1 generated)
robwu
Tested locally using the same steps as the other patch.
3 years, 2 months ago (2016-07-08 09:12:37 UTC) #2
Devlin
https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_injection.cc File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_injection.cc#newcode225 extensions/renderer/script_injection.cc:225: if (!injection_host_) A comment explaining why/how this could happen ...
3 years, 2 months ago (2016-07-08 20:51:09 UTC) #3
robwu
https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_injection.cc File extensions/renderer/script_injection.cc (right): https://codereview.chromium.org/2134613002/diff/1/extensions/renderer/script_injection.cc#newcode225 extensions/renderer/script_injection.cc:225: if (!injection_host_) On 2016/07/08 20:51:09, Devlin wrote: > A ...
3 years, 2 months ago (2016-07-09 05:22:02 UTC) #4
Devlin
https://codereview.chromium.org/2134613002/diff/20001/extensions/renderer/script_injection_manager.cc File extensions/renderer/script_injection_manager.cc (right): https://codereview.chromium.org/2134613002/diff/20001/extensions/renderer/script_injection_manager.cc#newcode110 extensions/renderer/script_injection_manager.cc:110: class ScriptInjectionManager::ScriptInjectionWatcher { I'm still not quite happy with ...
3 years, 2 months ago (2016-07-11 23:30:55 UTC) #5
robwu
3 years, 2 months ago (2016-07-12 08:59:36 UTC) #6
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).

Powered by Google App Engine
This is Rietveld 408576698