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

Issue 2116923002: Avoid using stale UserScript pointers (Closed)

Created:
3 years, 2 months ago by robwu
Modified:
3 years, 2 months ago
CC:
chromium-apps-reviews_chromium.org, chromium-reviews, 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

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}

Patch Set 1 #

Patch Set 2 : Remove browser test, see bug for details #

Total comments: 5
Unified diffs Side-by-side diffs Delta from patch set Stats (+20 lines, -4 lines) Patch
M extensions/renderer/user_script_injector.cc View 6 chunks +20 lines, -4 lines 5 comments Download

Messages

Total messages: 20 (7 generated)
robwu
3 years, 2 months ago (2016-07-03 05:58:55 UTC) #3
robwu
-Devlin (OOO) +Rockot PTAL, thanks!
3 years, 2 months ago (2016-07-04 19:27:41 UTC) #5
Ken Rockot(use gerrit already)
Please CC me on the bug so I can view it :)
3 years, 2 months ago (2016-07-04 22:12:52 UTC) #6
Ken Rockot(use gerrit already)
lgtm
3 years, 2 months ago (2016-07-04 22:26:51 UTC) #7
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2116923002/20001
3 years, 2 months ago (2016-07-04 22:27:27 UTC) #9
commit-bot: I haz the power
Committed patchset #2 (id:20001)
3 years, 2 months ago (2016-07-04 23:45:52 UTC) #11
commit-bot: I haz the power
CQ bit was unchecked.
3 years, 2 months ago (2016-07-04 23:45:56 UTC) #12
commit-bot: I haz the power
Patchset 2 (id:??) landed as https://crrev.com/a80776332fd8c99b58beab5d91a6675e85013628 Cr-Commit-Position: refs/heads/master@{#403725}
3 years, 2 months ago (2016-07-04 23:47:31 UTC) #14
Devlin
sorry for the delay; 4th/5th were holidays in the US. https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/user_script_injector.cc File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/user_script_injector.cc#newcode108 ...
3 years, 2 months ago (2016-07-06 15:22:22 UTC) #16
robwu
On 2016/07/06 15:22:22, Devlin (ooo - July 6) wrote: > sorry for the delay; 4th/5th ...
3 years, 2 months ago (2016-07-06 16:12:06 UTC) #17
Devlin
https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/user_script_injector.cc File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/user_script_injector.cc#newcode108 extensions/renderer/user_script_injector.cc:108: script_ = nullptr; On 2016/07/06 16:12:06, robwu wrote: > ...
3 years, 2 months ago (2016-07-06 16:17:58 UTC) #18
robwu
https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/user_script_injector.cc File extensions/renderer/user_script_injector.cc (right): https://codereview.chromium.org/2116923002/diff/20001/extensions/renderer/user_script_injector.cc#newcode108 extensions/renderer/user_script_injector.cc:108: script_ = nullptr; On 2016/07/06 16:17:58, Devlin wrote: > ...
3 years, 2 months ago (2016-07-06 16:26:58 UTC) #19
Devlin
3 years, 2 months ago (2016-07-06 16:40:56 UTC) #20
Message was sent while issue was closed.
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!

Powered by Google App Engine
This is Rietveld 408576698