New Task Manager - Phase 1.3.5: Implement Extension Task Providing

New Task Manager - Phase 1.3.5: Implement Extension Task Providing

CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
R=nick@chromium.org
BUG=470994
TEST=

Committed: https://crrev.com/31b34aacae32988cc01a0b249722973407912445
Cr-Commit-Position: refs/heads/master@{#342377}

[afakhry, 2015-07-25 00:50:40]

Hey Nick, Could you please take a look at this CL and my comments below.
I still have to land the guest & printing tasks in before I can write any
meaningful tests here.

https://codereview.chromium.org/1254103002/diff/1/chrome/browser/extensions/c...
File chrome/browser/extensions/chrome_extension_web_contents_observer.cc
(right):

https://codereview.chromium.org/1254103002/diff/1/chrome/browser/extensions/c...
chrome/browser/extensions/chrome_extension_web_contents_observer.cc:93:
extension);
Man, there must be a better place to do the tagging. Tagging here forced me to
have the condition that I don't like in WebContentsTags::CreateForExtension().

Other possible tagging places that are possibly covered by this one but they
might help us avoid potential collisions that result from tagging here:
1-
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

2-
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/ui/...

3-
https://code.google.com/p/chromium/codesearch#chromium/src/apps/custom_launch...

4-
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...

https://codereview.chromium.org/1254103002/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/web_contents_tags.cc:113: return;
Not liking having to do this check every time this function is called! :(

[ncarter (slow), 2015-07-27 19:42:35]

https://codereview.chromium.org/1254103002/diff/1/chrome/browser/task_managem...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/1/chrome/browser/task_managem...
chrome/browser/task_management/web_contents_tags.cc:113: return;
On 2015/07/25 00:50:40, afakhry wrote:
> Not liking having to do this check every time this function is called! :(

I agree, this isn't great. At the very least we could try to push this check up
into the extensions code.

Let's open a discussion with an extensions expert (maybe asergent). This might
be a case where communicating face-to-face (or VC) will converge faster than an
email based discussion.

[afakhry, 2015-08-03 19:03:33]

How about this implementation, Nick?

[ncarter (slow), 2015-08-03 21:12:18]

I think we're on the right track.

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:63:
contents->GetRenderViewHost()->GetSiteInstance()->GetSiteURL();
question for devlin -- how well-established is the pattern of extracting the
extension ID from the siteinstance url?

I'm somewhat worried about the fragility of this lookup. It relies on the caller
having created their initial SiteInstance via SiteInstance::CreateForURL with a
chrome-extension extension URL. Generally speaking (that is, for the drive-by
web), GetSiteURL can return surprising values that have nothing to do with the
current page. The way that extensions use it is a special case where it's
typically correct, but only because of fairly strong assumptions about the way
that extension processes are isolated from other types of processes.

So it's probably best to let the existing function do the lifting here:

extensions::ProcessManager::Get(web_contents->GetBrowserContext())->GetExtensionForWebContents(web_contents)

Or, even better, we can just pass the extension_id in. It is in easy reach for
all callers of SetViewType that we care about. Doing that likely means plumbing
a new function that's not SetViewType(), but that's called in the same places as
SetViewType() is today (and not called in the background contents/panel cases).

[afakhry, 2015-08-03 23:36:44]

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

[afakhry, 2015-08-03 23:36:45]

I added a couple of browser tests.

rdevlin.cronin@chromium.org: Could you please review the below files, as well as
Nick's comment in chrome/browser/task_management/web_contents_tags.cc in patch
set 2.

 chrome/browser/extensions/chrome_extensions_browser_client.cc
 chrome/browser/extensions/chrome_extensions_browser_client.h
 extensions/browser/extensions_browser_client.h
 extensions/browser/view_type_utils.cc

Thanks!

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:63:
contents->GetRenderViewHost()->GetSiteInstance()->GetSiteURL();
On 2015/08/03 21:12:18, ncarter wrote:
> question for devlin -- how well-established is the pattern of extracting the
> extension ID from the siteinstance url?
> 
> I'm somewhat worried about the fragility of this lookup. It relies on the
caller
> having created their initial SiteInstance via SiteInstance::CreateForURL with
a
> chrome-extension extension URL. Generally speaking (that is, for the drive-by
> web), GetSiteURL can return surprising values that have nothing to do with the
> current page. The way that extensions use it is a special case where it's
> typically correct, but only because of fairly strong assumptions about the way
> that extension processes are isolated from other types of processes.
> 
> So it's probably best to let the existing function do the lifting here:
> 
>
extensions::ProcessManager::Get(web_contents->GetBrowserContext())->GetExtensionForWebContents(web_contents)
> 
> Or, even better, we can just pass the extension_id in. It is in easy reach for
> all callers of SetViewType that we care about. Doing that likely means
plumbing
> a new function that's not SetViewType(), but that's called in the same places
as
> SetViewType() is today (and not called in the background contents/panel
cases).

This is the exact way followed in this code here:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
And also by the method you pointed out above:
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
But you're right, I think we might be better off by using an existing method
like the one you pointed out. I'll leave this question to devlin.

rdevlin.cronin@chromium.org: Could you please take a look at this part and
Nick's comment?

[Devlin, 2015-08-04 16:05:07]

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:63:
contents->GetRenderViewHost()->GetSiteInstance()->GetSiteURL();
On 2015/08/03 23:36:45, afakhry wrote:
> On 2015/08/03 21:12:18, ncarter wrote:
> > question for devlin -- how well-established is the pattern of extracting the
> > extension ID from the siteinstance url?
> > 
> > I'm somewhat worried about the fragility of this lookup. It relies on the
> caller
> > having created their initial SiteInstance via SiteInstance::CreateForURL
with
> a
> > chrome-extension extension URL. Generally speaking (that is, for the
drive-by
> > web), GetSiteURL can return surprising values that have nothing to do with
the
> > current page. The way that extensions use it is a special case where it's
> > typically correct, but only because of fairly strong assumptions about the
way
> > that extension processes are isolated from other types of processes.
> > 
> > So it's probably best to let the existing function do the lifting here:
> > 
> >
>
extensions::ProcessManager::Get(web_contents->GetBrowserContext())->GetExtensionForWebContents(web_contents)
> > 
> > Or, even better, we can just pass the extension_id in. It is in easy reach
for
> > all callers of SetViewType that we care about. Doing that likely means
> plumbing
> > a new function that's not SetViewType(), but that's called in the same
places
> as
> > SetViewType() is today (and not called in the background contents/panel
> cases).
> 
> This is the exact way followed in this code here:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> And also by the method you pointed out above:
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> But you're right, I think we might be better off by using an existing method
> like the one you pointed out. I'll leave this question to devlin.
> 
> mailto:rdevlin.cronin@chromium.org: Could you please take a look at this part
and
> Nick's comment?

For better or worse, we get the extension id from the site instance rather a
lot.  I'm not keen on passing in the extension id, because it doesn't make sense
for all view types (e.g., tab), and the idea of having yet another semi-mystical
function to call when creating an extension web contents is less attractive to
me.  I don't mind this approach, modulo using the existing functionality in
ProcessManager (if we need to change from using site instance some day, may as
well try and confine the usages as much as possible).

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:366: case
VIEW_TYPE_TAB_CONTENTS:
I think it would be good to document which tags these are (or will be) tracked
by.  Tab is obvious, but where does panel fall?

Also, do we really have one for invalid?  If so, that sounds like a bug.  If
not, I think we should NOTREACHED it.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:370: default:
Best to not have a default case.  That way, if we do add a new view type, we get
a compile error, rather than a runtime error.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:12:
class ExtensionTagsTest : public ExtensionBrowserTest {
Browser tests make me so sad.  Why can't this be a unit test?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:23: if
(!g_default_icon && ResourceBundle::HasSharedInstance()) {
When would HasSharedInstance() fail?  And, if it *can* fail, since the Skia is
owned by the resource bundle, doesn't that mean a lot of badness for returning
it, if it goes away after the skia is initialized?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:25:
IDR_PLUGINS_FAVICON);
Why use plugins, instead of extensions?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:49: //
We never change the favicon of the extension, we always use the default
Probably out of scope for this change, but why don't we want to use the
extension's icon?  Seems like it would be better for attribution.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:69:
(view_type == extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
nitty nit: Can we inline this a bit, to be
bool is_background = extensions::GetViewType(web_contents) ==
extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE;
?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/panel_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/panel_task.cc:56: const
bool is_app = extension ? extension->is_app() : false;
drive-by: this would be cleaner as is_app = extension && extension->is_app(). :)

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/renderer_task.h
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/renderer_task.h:62: //
whether it's an app, an extension, incognito or a background.
"a background"?  We should probably be more clear about what that means.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/renderer_task.h:64: bool
is_app,
the fact that we have is_app and is_extension is weird to me.  If you're up for
a change (possibly in a follow-up), this would probably be a lot cleaner if we
passed in an enum.  But, not directly related to this change.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:177: if
(!WebContentsTag::FromWebContents(web_contents)) {
For my own edification, in what circumstance could we have tagged the web
contents already?

[afakhry, 2015-08-04 18:31:08]

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/20001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:63:
contents->GetRenderViewHost()->GetSiteInstance()->GetSiteURL();
On 2015/08/04 16:05:07, Devlin wrote:
> On 2015/08/03 23:36:45, afakhry wrote:
> > On 2015/08/03 21:12:18, ncarter wrote:
> > > question for devlin -- how well-established is the pattern of extracting
the
> > > extension ID from the siteinstance url?
> > > 
> > > I'm somewhat worried about the fragility of this lookup. It relies on the
> > caller
> > > having created their initial SiteInstance via SiteInstance::CreateForURL
> with
> > a
> > > chrome-extension extension URL. Generally speaking (that is, for the
> drive-by
> > > web), GetSiteURL can return surprising values that have nothing to do with
> the
> > > current page. The way that extensions use it is a special case where it's
> > > typically correct, but only because of fairly strong assumptions about the
> way
> > > that extension processes are isolated from other types of processes.
> > > 
> > > So it's probably best to let the existing function do the lifting here:
> > > 
> > >
> >
>
extensions::ProcessManager::Get(web_contents->GetBrowserContext())->GetExtensionForWebContents(web_contents)
> > > 
> > > Or, even better, we can just pass the extension_id in. It is in easy reach
> for
> > > all callers of SetViewType that we care about. Doing that likely means
> > plumbing
> > > a new function that's not SetViewType(), but that's called in the same
> places
> > as
> > > SetViewType() is today (and not called in the background contents/panel
> > cases).
> > 
> > This is the exact way followed in this code here:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > And also by the method you pointed out above:
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
> > But you're right, I think we might be better off by using an existing method
> > like the one you pointed out. I'll leave this question to devlin.
> > 
> > mailto:rdevlin.cronin@chromium.org: Could you please take a look at this
part
> and
> > Nick's comment?
> 
> For better or worse, we get the extension id from the site instance rather a
> lot.  I'm not keen on passing in the extension id, because it doesn't make
sense
> for all view types (e.g., tab), and the idea of having yet another
semi-mystical
> function to call when creating an extension web contents is less attractive to
> me.  I don't mind this approach, modulo using the existing functionality in
> ProcessManager (if we need to change from using site instance some day, may as
> well try and confine the usages as much as possible).

Done.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/extensio...
File chrome/browser/extensions/chrome_extensions_browser_client.cc (right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:366: case
VIEW_TYPE_TAB_CONTENTS:
On 2015/08/04 16:05:07, Devlin wrote:
> I think it would be good to document which tags these are (or will be) tracked
> by.  Tab is obvious, but where does panel fall?
> 
> Also, do we really have one for invalid?  If so, that sounds like a bug.  If
> not, I think we should NOTREACHED it.

The PanelTag will be created in Panel::Initialize(). I added more comments.
Please let me know if they're still not obvious.

Done for the NOTREACHED() the invalid type.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/extensio...
chrome/browser/extensions/chrome_extensions_browser_client.cc:370: default:
On 2015/08/04 16:05:07, Devlin wrote:
> Best to not have a default case.  That way, if we do add a new view type, we
get
> a compile error, rather than a runtime error.

Done.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:12:
class ExtensionTagsTest : public ExtensionBrowserTest {
On 2015/08/04 16:05:07, Devlin wrote:
> Browser tests make me so sad.  Why can't this be a unit test?

We want to simulate reality as much as we can. Is there a reason to avoid
browser tests?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:23: if
(!g_default_icon && ResourceBundle::HasSharedInstance()) {
On 2015/08/04 16:05:07, Devlin wrote:
> When would HasSharedInstance() fail?  And, if it *can* fail, since the Skia is
> owned by the resource bundle, doesn't that mean a lot of badness for returning
> it, if it goes away after the skia is initialized?

This happened to me once in the past. I opened the task manager immediately
right after I signed in to ChromeOS and that caused a crash. I'm not sure if
that could happen again. Here we avoid this problem - if it doesn't have an
instance, we just return nullptr, which will be taken care of by the Task ctor
and use a default empty ImageSkia.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:25:
IDR_PLUGINS_FAVICON);
On 2015/08/04 16:05:07, Devlin wrote:
> Why use plugins, instead of extensions?

Indeed there's an IDR_EXTENSIONS_FAVICON not sure why the old task manager was
using the PLUGIN's. Maybe it wasn't there yet.


Done.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:49: //
We never change the favicon of the extension, we always use the default
On 2015/08/04 16:05:07, Devlin wrote:
> Probably out of scope for this change, but why don't we want to use the
> extension's icon?  Seems like it would be better for attribution.

I agree. What do you think, Nick?

Devlin, Can we get that from the WebContents?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:69:
(view_type == extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);
On 2015/08/04 16:05:07, Devlin wrote:
> nitty nit: Can we inline this a bit, to be
> bool is_background = extensions::GetViewType(web_contents) ==
> extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE;
> ?

Done.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/panel_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/panel_task.cc:56: const
bool is_app = extension ? extension->is_app() : false;
On 2015/08/04 16:05:07, Devlin wrote:
> drive-by: this would be cleaner as is_app = extension && extension->is_app().
:)

Much better, really! Thanks! :) 
Done.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/renderer_task.h
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/renderer_task.h:62: //
whether it's an app, an extension, incognito or a background.
On 2015/08/04 16:05:07, Devlin wrote:
> "a background"?  We should probably be more clear about what that means.

background here means either view type of BACKGROUND_CONTENTS or
BACKGROUND_PAGE.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/renderer_task.h:64: bool
is_app,
On 2015/08/04 16:05:07, Devlin wrote:
> the fact that we have is_app and is_extension is weird to me.  If you're up
for
> a change (possibly in a follow-up), this would probably be a lot cleaner if we
> passed in an enum.  But, not directly related to this change.

I'm not sure I totally get your suggestion here. However, I will defer
commenting on this to Nick, because this is how the old task manager used to do
the prefix. I remember Nick telling me about the desire to have the WebContents'
app-ness take precedence over its extension-ness and tab-ness.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:177: if
(!WebContentsTag::FromWebContents(web_contents)) {
On 2015/08/04 16:05:07, Devlin wrote:
> For my own edification, in what circumstance could we have tagged the web
> contents already?

Some web_contents change their ownership. For example, web_contents that are
pre-rendered will change be released by the PrerenderManager and then will be
owned by the TabStripModel.
Maybe that's not applicable for the extensions case?

[ncarter (slow), 2015-08-04 18:55:19]

lgtm

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:25:
IDR_PLUGINS_FAVICON);
On 2015/08/04 16:05:07, Devlin wrote:
> Why use plugins, instead of extensions?

We ought to switch to IDR_EXTENSIONS_FAVICON here. But note that in
chrome/app/theme/theme_resources.grd they resolve to the same png
(chrome\app\theme\default_100_percent\common\favicon_extensions.png, the little
black puzzle piece)

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/renderer_task.h
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/renderer_task.h:64: bool
is_app,
On 2015/08/04 18:31:08, afakhry wrote:
> On 2015/08/04 16:05:07, Devlin wrote:
> > the fact that we have is_app and is_extension is weird to me.  If you're up
> for
> > a change (possibly in a follow-up), this would probably be a lot cleaner if
we
> > passed in an enum.  But, not directly related to this change.
> 
> I'm not sure I totally get your suggestion here. However, I will defer
> commenting on this to Nick, because this is how the old task manager used to
do
> the prefix. I remember Nick telling me about the desire to have the
WebContents'
> app-ness take precedence over its extension-ness and tab-ness.

I think devlin means that "app && !extension" ought to be nonsensical, and
replacing the pair of bools with a a tri-state enum would eliminate that case.
Which I agree with. But given how the code flows, I this function might actually
be a tad easier to call with two bools -- unless there's an existing tri-state
enum that we leverage. Note that is_background also implies is_extension, so
there are other nonsensical ways to call this function, but that's probably
okay; we return something reasonable in those cases too.

https://codereview.chromium.org/1254103002/diff/100001/chrome/browser/task_ma...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/100001/chrome/browser/task_ma...
chrome/browser/task_management/web_contents_tags.cc:159:
GetExtensionForWebContents(web_contents);
Based on devlin's comments and a chat with creis, I'm okay with reliance on
GetSiteURL(). Charlie explained that it has to work that way because of
effective url mappings (apps), and given that devlin says it's the common
practice, it seems like the right thing to do here.

[afakhry, 2015-08-04 19:02:26]

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/renderer_task.h
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/renderer_task.h:64: bool
is_app,
On 2015/08/04 18:55:19, ncarter wrote:
> On 2015/08/04 18:31:08, afakhry wrote:
> > On 2015/08/04 16:05:07, Devlin wrote:
> > > the fact that we have is_app and is_extension is weird to me.  If you're
up
> > for
> > > a change (possibly in a follow-up), this would probably be a lot cleaner
if
> we
> > > passed in an enum.  But, not directly related to this change.
> > 
> > I'm not sure I totally get your suggestion here. However, I will defer
> > commenting on this to Nick, because this is how the old task manager used to
> do
> > the prefix. I remember Nick telling me about the desire to have the
> WebContents'
> > app-ness take precedence over its extension-ness and tab-ness.
> 
> I think devlin means that "app && !extension" ought to be nonsensical, and
> replacing the pair of bools with a a tri-state enum would eliminate that case.
> Which I agree with. But given how the code flows, I this function might
actually
> be a tad easier to call with two bools -- unless there's an existing tri-state
> enum that we leverage. Note that is_background also implies is_extension, so
> there are other nonsensical ways to call this function, but that's probably
> okay; we return something reasonable in those cases too.

I got it. We can definitely defer this to a later change.

[Devlin, 2015-08-04 20:22:40]

Looks like you have a few failing tests...

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:12:
class ExtensionTagsTest : public ExtensionBrowserTest {
On 2015/08/04 18:31:08, afakhry wrote:
> On 2015/08/04 16:05:07, Devlin wrote:
> > Browser tests make me so sad.  Why can't this be a unit test?
> 
> We want to simulate reality as much as we can. Is there a reason to avoid
> browser tests?

Browser tests are a lot more expensive, and quite a bit more flaky.  In general,
unittests are better.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:23: if
(!g_default_icon && ResourceBundle::HasSharedInstance()) {
On 2015/08/04 18:31:08, afakhry wrote:
> On 2015/08/04 16:05:07, Devlin wrote:
> > When would HasSharedInstance() fail?  And, if it *can* fail, since the Skia
is
> > owned by the resource bundle, doesn't that mean a lot of badness for
returning
> > it, if it goes away after the skia is initialized?
> 
> This happened to me once in the past. I opened the task manager immediately
> right after I signed in to ChromeOS and that caused a crash. I'm not sure if
> that could happen again. Here we avoid this problem - if it doesn't have an
> instance, we just return nullptr, which will be taken care of by the Task ctor
> and use a default empty ImageSkia.

This doesn't really answer the question of "What if the resource bundle goes
away after the icon is initialized", but maybe that'll never happen?

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:49: //
We never change the favicon of the extension, we always use the default
On 2015/08/04 18:31:08, afakhry wrote:
> On 2015/08/04 16:05:07, Devlin wrote:
> > Probably out of scope for this change, but why don't we want to use the
> > extension's icon?  Seems like it would be better for attribution.
> 
> I agree. What do you think, Nick?
> 
> Devlin, Can we get that from the WebContents?

Sure, indirectly (more like the browser context).  But we might wanna save that
for another patch.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:177: if
(!WebContentsTag::FromWebContents(web_contents)) {
On 2015/08/04 18:31:08, afakhry wrote:
> On 2015/08/04 16:05:07, Devlin wrote:
> > For my own edification, in what circumstance could we have tagged the web
> > contents already?
> 
> Some web_contents change their ownership. For example, web_contents that are
> pre-rendered will change be released by the PrerenderManager and then will be
> owned by the TabStripModel.
> Maybe that's not applicable for the extensions case?

Hmm... so if a web contents *does* change ownership, then should we update the
tag as well?

[afakhry, 2015-08-05 17:20:07]

Honestly, I'm not liking passing the extension, but it solves some problems but
not all of them. See my comment here:
https://codereview.chromium.org/1254103002/diff/160001/chrome/browser/task_ma...

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:12:
class ExtensionTagsTest : public ExtensionBrowserTest {
On 2015/08/04 20:22:39, Devlin wrote:
> On 2015/08/04 18:31:08, afakhry wrote:
> > On 2015/08/04 16:05:07, Devlin wrote:
> > > Browser tests make me so sad.  Why can't this be a unit test?
> > 
> > We want to simulate reality as much as we can. Is there a reason to avoid
> > browser tests?
> 
> Browser tests are a lot more expensive, and quite a bit more flaky.  In
general,
> unittests are better.

I totally agree, in this case however, it made sense to be able to reuse the
large amount of code available in ExtensionBrowserTest for setting the
environment up, and for loading/unloading/enabling/disabling extensions. I think
it would be unwise to rewrite all of those.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:23: if
(!g_default_icon && ResourceBundle::HasSharedInstance()) {
On 2015/08/04 20:22:40, Devlin wrote:
> On 2015/08/04 18:31:08, afakhry wrote:
> > On 2015/08/04 16:05:07, Devlin wrote:
> > > When would HasSharedInstance() fail?  And, if it *can* fail, since the
Skia
> is
> > > owned by the resource bundle, doesn't that mean a lot of badness for
> returning
> > > it, if it goes away after the skia is initialized?
> > 
> > This happened to me once in the past. I opened the task manager immediately
> > right after I signed in to ChromeOS and that caused a crash. I'm not sure if
> > that could happen again. Here we avoid this problem - if it doesn't have an
> > instance, we just return nullptr, which will be taken care of by the Task
ctor
> > and use a default empty ImageSkia.
> 
> This doesn't really answer the question of "What if the resource bundle goes
> away after the icon is initialized", but maybe that'll never happen?

This should never happen. If it does, that's an indication of a real problem.
the shared instance is reset to null at ResourceBundle::CleanupSharedInstance()
which should be called at shutdown.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:25:
IDR_PLUGINS_FAVICON);
On 2015/08/04 18:55:19, ncarter wrote:
> On 2015/08/04 16:05:07, Devlin wrote:
> > Why use plugins, instead of extensions?
> 
> We ought to switch to IDR_EXTENSIONS_FAVICON here. But note that in
> chrome/app/theme/theme_resources.grd they resolve to the same png
> (chrome\app\theme\default_100_percent\common\favicon_extensions.png, the
little
> black puzzle piece)

Done.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:49: //
We never change the favicon of the extension, we always use the default
On 2015/08/04 20:22:40, Devlin wrote:
> On 2015/08/04 18:31:08, afakhry wrote:
> > On 2015/08/04 16:05:07, Devlin wrote:
> > > Probably out of scope for this change, but why don't we want to use the
> > > extension's icon?  Seems like it would be better for attribution.
> > 
> > I agree. What do you think, Nick?
> > 
> > Devlin, Can we get that from the WebContents?
> 
> Sure, indirectly (more like the browser context).  But we might wanna save
that
> for another patch.

Let me see if I can do that in a follow-up patch. We have several other task
manager tasks in which we get the favicon from the web_contents. I will
investigate if that works for the extensions too.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/web_contents_tags.cc:177: if
(!WebContentsTag::FromWebContents(web_contents)) {
On 2015/08/04 20:22:40, Devlin wrote:
> On 2015/08/04 18:31:08, afakhry wrote:
> > On 2015/08/04 16:05:07, Devlin wrote:
> > > For my own edification, in what circumstance could we have tagged the web
> > > contents already?
> > 
> > Some web_contents change their ownership. For example, web_contents that are
> > pre-rendered will change be released by the PrerenderManager and then will
be
> > owned by the TabStripModel.
> > Maybe that's not applicable for the extensions case?
> 
> Hmm... so if a web contents *does* change ownership, then should we update the
> tag as well?

Yes, that's why we have WebContentsTags::ClearTag(). We have to clear the tag
and have the new owner retag the WebContents with its appropriate tag. take a
look at:
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/browser/pre...

https://codereview.chromium.org/1254103002/diff/160001/chrome/browser/task_ma...
File chrome/browser/task_management/web_contents_tags.cc (right):

https://codereview.chromium.org/1254103002/diff/160001/chrome/browser/task_ma...
chrome/browser/task_management/web_contents_tags.cc:167: return;
Even with the above change and passing the extension if available, I was still
hitting the DCHECK() that used to be here in some tests. For that I removed it
and simply returned if extension is still nullptr. 

It seemed to me the pattern followed here
https://code.google.com/p/chromium/codesearch#chromium/src/extensions/browser...
which handles the case that an extension could possibly be not registered.

[afakhry, 2015-08-06 15:32:36]

afakhry@chromium.org changed reviewers:
+ lazyboy@chromium.org

[afakhry, 2015-08-06 15:32:36]

Finally all bots are passing! Now I'm not passing the extension (thanks to a
suggestion made by Nick), instead we defer getting the extension from the
WebContents until actually the task needs to be created (which happens when the
RenderView is ready or the RenderFrameHost is changed). By then most probably
the extension will be retrievable from the ProcessManager.

If for some reason, the extension was not retrievable from the WebContents, we
use the WebContents itself to get the title of the task.

Nick, and Devlin, please take a look.

+lazyboy@chromium.org: Could you please take a look at web_view_browsertest.cc?
We needed to handle the TODO we left earlier to update the tests once we start
tracking the rest of extensions. Now is the time! Thanks!

[lazyboy, 2015-08-06 17:17:45]

web_view_browsertest.cc LGTM

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/web_contents/extension_tag.h
(right):

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/extension_tag.h:29: // The
extension whose WebContents this tag is attached to.
Update comment.

[afakhry, 2015-08-06 17:34:56]

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/web_contents/extension_tag.h
(right):

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/extension_tag.h:29: // The
extension whose WebContents this tag is attached to.
On 2015/08/06 17:17:45, lazyboy (OOO Aug 10 - 18) wrote:
> Update comment.

Done.

[ncarter (slow), 2015-08-06 17:36:33]

lgtm

[Devlin, 2015-08-06 17:43:44]

Sorry for the delay on this.  A few last comments, but LGTM.  If you want to
take a stab at the unittest, I think it'd be good, but I won't block you on that
if it proves troublesome.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:12:
class ExtensionTagsTest : public ExtensionBrowserTest {
On 2015/08/05 17:20:07, afakhry wrote:
> On 2015/08/04 20:22:39, Devlin wrote:
> > On 2015/08/04 18:31:08, afakhry wrote:
> > > On 2015/08/04 16:05:07, Devlin wrote:
> > > > Browser tests make me so sad.  Why can't this be a unit test?
> > > 
> > > We want to simulate reality as much as we can. Is there a reason to avoid
> > > browser tests?
> > 
> > Browser tests are a lot more expensive, and quite a bit more flaky.  In
> general,
> > unittests are better.
> 
> I totally agree, in this case however, it made sense to be able to reuse the
> large amount of code available in ExtensionBrowserTest for setting the
> environment up, and for loading/unloading/enabling/disabling extensions. I
think
> it would be unwise to rewrite all of those.

Take a look at, e.g., ExtensionServiceTestBase.  We already have infrastructure
for extension unit tests :)

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:23: if
(!g_default_icon && ResourceBundle::HasSharedInstance()) {
On 2015/08/05 17:20:07, afakhry wrote:
> On 2015/08/04 20:22:40, Devlin wrote:
> > On 2015/08/04 18:31:08, afakhry wrote:
> > > On 2015/08/04 16:05:07, Devlin wrote:
> > > > When would HasSharedInstance() fail?  And, if it *can* fail, since the
> Skia
> > is
> > > > owned by the resource bundle, doesn't that mean a lot of badness for
> > returning
> > > > it, if it goes away after the skia is initialized?
> > > 
> > > This happened to me once in the past. I opened the task manager
immediately
> > > right after I signed in to ChromeOS and that caused a crash. I'm not sure
if
> > > that could happen again. Here we avoid this problem - if it doesn't have
an
> > > instance, we just return nullptr, which will be taken care of by the Task
> ctor
> > > and use a default empty ImageSkia.
> > 
> > This doesn't really answer the question of "What if the resource bundle goes
> > away after the icon is initialized", but maybe that'll never happen?
> 
> This should never happen. If it does, that's an indication of a real problem.
> the shared instance is reset to null at
ResourceBundle::CleanupSharedInstance()
> which should be called at shutdown.

This is probably edge case of edge case, but I dislike use-after-frees, so, :)

My concern is the following flow:
g_default_icon is initialized while ResourceBundle is alive.
ResourceBundle shuts down.
GetDefaultIcon() is called, and returns the initialized g_default_icon, which is
a pointer to an image owned by the resource bundle, which has been destroyed -
UAF.

Can you guarantee that ResourceBundle will be destructed after every possible
call of GetDefaultIcon()?  (My inclination is "probably", but if it's possible
for this to be called before ResourceBundle is initialized, it may be possible
for it to be called after it's shut down.)

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/extension_task.cc:50: //
We never change the favicon of the extension, we always use the default
Can you add a TODO to use the extension's icon, just so it doesn't get lost?

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/extension_task.cc:64:
base::string16 title;
nit: ternaries are nice. :)
base::string16 title = extension ?
    base::UTF8ToUTF16(extension->name()) :
    RendererTask::GetTitleFromWebContents(web_contents);

[afakhry, 2015-08-06 18:01:27]

Thanks!

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_tag_browsertest.cc:12:
class ExtensionTagsTest : public ExtensionBrowserTest {
On 2015/08/06 17:43:44, Devlin wrote:
> On 2015/08/05 17:20:07, afakhry wrote:
> > On 2015/08/04 20:22:39, Devlin wrote:
> > > On 2015/08/04 18:31:08, afakhry wrote:
> > > > On 2015/08/04 16:05:07, Devlin wrote:
> > > > > Browser tests make me so sad.  Why can't this be a unit test?
> > > > 
> > > > We want to simulate reality as much as we can. Is there a reason to
avoid
> > > > browser tests?
> > > 
> > > Browser tests are a lot more expensive, and quite a bit more flaky.  In
> > general,
> > > unittests are better.
> > 
> > I totally agree, in this case however, it made sense to be able to reuse the
> > large amount of code available in ExtensionBrowserTest for setting the
> > environment up, and for loading/unloading/enabling/disabling extensions. I
> think
> > it would be unwise to rewrite all of those.
> 
> Take a look at, e.g., ExtensionServiceTestBase.  We already have
infrastructure
> for extension unit tests :)

Thanks for pointing that out. I will take a look at this, probably in a
follow-up CL if necessary.

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/60001/chrome/browser/task_man...
chrome/browser/task_management/providers/web_contents/extension_task.cc:23: if
(!g_default_icon && ResourceBundle::HasSharedInstance()) {
On 2015/08/06 17:43:44, Devlin wrote:
> On 2015/08/05 17:20:07, afakhry wrote:
> > On 2015/08/04 20:22:40, Devlin wrote:
> > > On 2015/08/04 18:31:08, afakhry wrote:
> > > > On 2015/08/04 16:05:07, Devlin wrote:
> > > > > When would HasSharedInstance() fail?  And, if it *can* fail, since the
> > Skia
> > > is
> > > > > owned by the resource bundle, doesn't that mean a lot of badness for
> > > returning
> > > > > it, if it goes away after the skia is initialized?
> > > > 
> > > > This happened to me once in the past. I opened the task manager
> immediately
> > > > right after I signed in to ChromeOS and that caused a crash. I'm not
sure
> if
> > > > that could happen again. Here we avoid this problem - if it doesn't have
> an
> > > > instance, we just return nullptr, which will be taken care of by the
Task
> > ctor
> > > > and use a default empty ImageSkia.
> > > 
> > > This doesn't really answer the question of "What if the resource bundle
goes
> > > away after the icon is initialized", but maybe that'll never happen?
> > 
> > This should never happen. If it does, that's an indication of a real
problem.
> > the shared instance is reset to null at
> ResourceBundle::CleanupSharedInstance()
> > which should be called at shutdown.
> 
> This is probably edge case of edge case, but I dislike use-after-frees, so, :)
> 
> My concern is the following flow:
> g_default_icon is initialized while ResourceBundle is alive.
> ResourceBundle shuts down.
> GetDefaultIcon() is called, and returns the initialized g_default_icon, which
is
> a pointer to an image owned by the resource bundle, which has been destroyed -
> UAF.
> 
> Can you guarantee that ResourceBundle will be destructed after every possible
> call of GetDefaultIcon()?  (My inclination is "probably", but if it's possible
> for this to be called before ResourceBundle is initialized, it may be possible
> for it to be called after it's shut down.)

I changed it to return nullptr whenever !ResourceBundle::HasSharedInstance().
nullptr is handled cleanly by the Task's ctor.

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
File chrome/browser/task_management/providers/web_contents/extension_task.cc
(right):

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/extension_task.cc:50: //
We never change the favicon of the extension, we always use the default
On 2015/08/06 17:43:44, Devlin wrote:
> Can you add a TODO to use the extension's icon, just so it doesn't get lost?

Done.

https://codereview.chromium.org/1254103002/diff/180001/chrome/browser/task_ma...
chrome/browser/task_management/providers/web_contents/extension_task.cc:64:
base::string16 title;
On 2015/08/06 17:43:44, Devlin wrote:
> nit: ternaries are nice. :)
> base::string16 title = extension ?
>     base::UTF8ToUTF16(extension->name()) :
>     RendererTask::GetTitleFromWebContents(web_contents);

Absolutely! Thanks! :)
Done.

[afakhry, 2015-08-06 18:02:27]

The CQ bit was checked by afakhry@chromium.org

[afakhry, 2015-08-06 18:02:27]

The patchset sent to the CQ was uploaded after l-g-t-m from
lazyboy@chromium.org, nick@chromium.org, rdevlin.cronin@chromium.org
Link to the patchset: https://codereview.chromium.org/1254103002/#ps220001
(title: "Devlin's comments")

[commit-bot: I haz the power, 2015-08-06 18:02:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254103002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254103002/220001

[commit-bot: I haz the power, 2015-08-06 19:55:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-08-06 19:55:22]

Try jobs failed on following builders:
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[afakhry, 2015-08-07 16:29:42]

The CQ bit was checked by afakhry@chromium.org

[commit-bot: I haz the power, 2015-08-07 16:29:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1254103002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1254103002/220001

[commit-bot: I haz the power, 2015-08-07 17:56:01]

Committed patchset #12 (id:220001)

[commit-bot: I haz the power, 2015-08-07 17:56:49]

Patchset 12 (id:??) landed as
https://crrev.com/31b34aacae32988cc01a0b249722973407912445
Cr-Commit-Position: refs/heads/master@{#342377}