Avoid using stale UserScript pointers

Avoid using stale UserScript pointers

BUG=625393
TEST=Manually. Compile Chrome with ASAN and follow the steps from the bug report.

Committed: https://crrev.com/a80776332fd8c99b58beab5d91a6675e85013628
Cr-Commit-Position: refs/heads/master@{#403725}

[robwu, 2016-07-03 05:58:39]

Description was changed from

==========
Avoid using stale UserScript pointers

BUG=625393
==========

to

==========
Avoid using stale UserScript pointers

BUG=625393
TEST=Manually. Compile Chrome with ASAN and follow the steps from the bug
report.
==========

[robwu, 2016-07-03 05:58:39]

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

[robwu, 2016-07-04 19:27:40]

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

[robwu, 2016-07-04 19:27:41]

-Devlin (OOO)
+Rockot

PTAL, thanks!

[Ken Rockot(use gerrit already), 2016-07-04 22:12:52]

Please CC me on the bug so I can view it :)

[Ken Rockot(use gerrit already), 2016-07-04 22:26:51]

lgtm

[robwu, 2016-07-04 22:27:22]

The CQ bit was checked by rob@robwu.nl

[commit-bot: I haz the power, 2016-07-04 22:27:27]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-07-04 23:45:51]

Description was changed from

==========
Avoid using stale UserScript pointers

BUG=625393
TEST=Manually. Compile Chrome with ASAN and follow the steps from the bug
report.
==========

to

==========
Avoid using stale UserScript pointers

BUG=625393
TEST=Manually. Compile Chrome with ASAN and follow the steps from the bug
report.
==========

[commit-bot: I haz the power, 2016-07-04 23:45:52]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-07-04 23:45:56]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-04 23:47:30]

Description was changed from

==========
Avoid using stale UserScript pointers

BUG=625393
TEST=Manually. Compile Chrome with ASAN and follow the steps from the bug
report.
==========

to

==========
Avoid using stale UserScript pointers

BUG=625393
TEST=Manually. Compile Chrome with ASAN and follow the steps from the bug
report.

Committed: https://crrev.com/a80776332fd8c99b58beab5d91a6675e85013628
Cr-Commit-Position: refs/heads/master@{#403725}
==========

[commit-bot: I haz the power, 2016-07-04 23:47:31]

Patchset 2 (id:??) landed as
https://crrev.com/a80776332fd8c99b58beab5d91a6675e85013628
Cr-Commit-Position: refs/heads/master@{#403725}

[Devlin, 2016-07-06 15:22:22]

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

[Devlin, 2016-07-06 15:22:22]

sorry for the delay; 4th/5th were holidays in the US.

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_injector.cc:108: script_ = nullptr;
We should call ScriptInjection::OnHostRemoved() when the extension is unloaded. 
If this only happens because of synchronous JS, would a simpler/cleaner solution
be to check injection_host_ in ScriptInjection() after we inject JS, rather than
adding a bunch of null checks here?

(optionally, you could also do also do this and then (D)CHECK script_ in most
places.)

[robwu, 2016-07-06 16:12:06]

On 2016/07/06 15:22:22, Devlin (ooo - July 6) wrote:
> sorry for the delay; 4th/5th were holidays in the US.

No problem, you've added OOO to your name, so I didn't expect a reply any
sooner. For a moment I forgot that it was July already, so I had moved the
review to Ken.

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_injector.cc:108: script_ = nullptr;
On 2016/07/06 15:22:22, Devlin (ooo - July 6) wrote:
> We should call ScriptInjection::OnHostRemoved() when the extension is
unloaded. 
> If this only happens because of synchronous JS, would a simpler/cleaner
solution
> be to check injection_host_ in ScriptInjection() after we inject JS, rather
than
> adding a bunch of null checks here?

I considered it, but because the bug is only related to user scripts, I decided
to put the checks in this class, without putting the burden of being aware of
this subtlety on the user of the class.

[Devlin, 2016-07-06 16:17:58]

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_injector.cc:108: script_ = nullptr;
On 2016/07/06 16:12:06, robwu wrote:
> On 2016/07/06 15:22:22, Devlin (ooo - July 6) wrote:
> > We should call ScriptInjection::OnHostRemoved() when the extension is
> unloaded. 
> > If this only happens because of synchronous JS, would a simpler/cleaner
> solution
> > be to check injection_host_ in ScriptInjection() after we inject JS, rather
> than
> > adding a bunch of null checks here?
> 
> I considered it, but because the bug is only related to user scripts, I
decided
> to put the checks in this class, without putting the burden of being aware of
> this subtlety on the user of the class.

I think I'd prefer checking the host in ScriptInjection for two reasons:
- We should be able to pinpoint when the host can be removed when synchronously
injecting (due to blocking) - specifically right after JS injection happens - so
there should only be a single check necessary.  In this file, it's not
immediately clear when script_ may or may not be null, and I can easily imagine
that someone will forget the null check at some point.
- This can actually affect programmatic scripts (e.g. tabs.executeScript) as
well.  While I don't think anything would necessarily break, I don't think we
should be injecting or handling scripts for removed extensions in general.

[robwu, 2016-07-06 16:26:58]

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_injector.cc:108: script_ = nullptr;
On 2016/07/06 16:17:58, Devlin wrote:
> On 2016/07/06 16:12:06, robwu wrote:
> > On 2016/07/06 15:22:22, Devlin (ooo - July 6) wrote:
> > > We should call ScriptInjection::OnHostRemoved() when the extension is
> > unloaded. 
> > > If this only happens because of synchronous JS, would a simpler/cleaner
> > solution
> > > be to check injection_host_ in ScriptInjection() after we inject JS,
rather
> > than
> > > adding a bunch of null checks here?
> > 
> > I considered it, but because the bug is only related to user scripts, I
> decided
> > to put the checks in this class, without putting the burden of being aware
of
> > this subtlety on the user of the class.
> 
> I think I'd prefer checking the host in ScriptInjection for two reasons:
> - We should be able to pinpoint when the host can be removed when
synchronously
> injecting (due to blocking) - specifically right after JS injection happens -
so
> there should only be a single check necessary.  In this file, it's not
> immediately clear when script_ may or may not be null, and I can easily
imagine
> that someone will forget the null check at some point.
> - This can actually affect programmatic scripts (e.g. tabs.executeScript) as
> well.  While I don't think anything would necessarily break, I don't think we
> should be injecting or handling scripts for removed extensions in general.

That's a good one. I will submit a separate CL since the current patch already
landed, and not injecting scripts after removal is a behavioral change (opposed
to the current patch, which fixes the bug without other side effects).

[Devlin, 2016-07-06 16:40:56]

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
File extensions/renderer/user_script_injector.cc (right):

https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/use...
extensions/renderer/user_script_injector.cc:108: script_ = nullptr;
On 2016/07/06 16:26:57, robwu wrote:
> On 2016/07/06 16:17:58, Devlin wrote:
> > On 2016/07/06 16:12:06, robwu wrote:
> > > On 2016/07/06 15:22:22, Devlin (ooo - July 6) wrote:
> > > > We should call ScriptInjection::OnHostRemoved() when the extension is
> > > unloaded. 
> > > > If this only happens because of synchronous JS, would a simpler/cleaner
> > > solution
> > > > be to check injection_host_ in ScriptInjection() after we inject JS,
> rather
> > > than
> > > > adding a bunch of null checks here?
> > > 
> > > I considered it, but because the bug is only related to user scripts, I
> > decided
> > > to put the checks in this class, without putting the burden of being aware
> of
> > > this subtlety on the user of the class.
> > 
> > I think I'd prefer checking the host in ScriptInjection for two reasons:
> > - We should be able to pinpoint when the host can be removed when
> synchronously
> > injecting (due to blocking) - specifically right after JS injection happens
-
> so
> > there should only be a single check necessary.  In this file, it's not
> > immediately clear when script_ may or may not be null, and I can easily
> imagine
> > that someone will forget the null check at some point.
> > - This can actually affect programmatic scripts (e.g. tabs.executeScript) as
> > well.  While I don't think anything would necessarily break, I don't think
we
> > should be injecting or handling scripts for removed extensions in general.
> 
> That's a good one. I will submit a separate CL since the current patch already
> landed, and not injecting scripts after removal is a behavioral change
(opposed
> to the current patch, which fixes the bug without other side effects).

Sounds good, thanks!