New Task Manager - Phase 1.3.5: Implement Extension Task Providing

chrome/browser/task_management/providers/web_contents/extension_task.cc

+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "chrome/browser/task_management/providers/web_contents/extension_task.h"
+
+#include "base/strings/utf_string_conversions.h"
+#include "content/public/browser/browser_context.h"
+#include "content/public/browser/web_contents.h"
+#include "extensions/browser/view_type_utils.h"
+#include "extensions/common/extension.h"
+#include "extensions/common/view_type.h"
+#include "grit/theme_resources.h"
+#include "ui/base/resource/resource_bundle.h"
+
+namespace task_management {
+
+namespace {
+
+gfx::ImageSkia* g_default_icon = nullptr;
+
+gfx::ImageSkia* GetDefaultIcon() {
+  if (!g_default_icon && ResourceBundle::HasSharedInstance()) {

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

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?

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

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.

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

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?

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

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.

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

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.)

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

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

+    g_default_icon = ResourceBundle::GetSharedInstance().GetImageSkiaNamed(
+        IDR_PLUGINS_FAVICON);

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

Why use plugins, instead of extensions?

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

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.

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

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)

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

Done.

+  }
+
+  return g_default_icon;
+}
+
+}  // namespace
+
+ExtensionTask::ExtensionTask(content::WebContents* web_contents,
+                             const extensions::Extension* extension)
+    : RendererTask(GetExtensionTitle(web_contents, extension),
+                   GetDefaultIcon(),
+                   web_contents) {
+}
+
+ExtensionTask::~ExtensionTask() {
+}
+
+void ExtensionTask::OnTitleChanged(content::NavigationEntry* entry) {
+  // The title of the extension should not change as a result of title change
+  // in its WebContents, so we ignore this.
+}
+
+void ExtensionTask::OnFaviconChanged() {
+  // We never change the favicon of the extension, we always use the default

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

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.

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

I agree. What do you think, Nick?

Devlin, Can we get that from the WebContents?

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

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

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

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.

+  // one.
+}
+
+Task::Type ExtensionTask::GetType() const {
+  return Task::EXTENSION;
+}
+
+base::string16 ExtensionTask::GetExtensionTitle(
+    content::WebContents* web_contents,
+    const extensions::Extension* extension) const {
+  DCHECK(web_contents);
+  DCHECK(extension);
+
+  base::string16 title = base::UTF8ToUTF16(extension->name());
+
+  DCHECK(!title.empty());
+
+  extensions::ViewType view_type = extensions::GetViewType(web_contents);
+  bool is_background =
+      (view_type == extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE);

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

nitty nit: Can we inline this a bit, to be
bool is_background = extensions::GetViewType(web_contents) ==
extensions::VIEW_TYPE_EXTENSION_BACKGROUND_PAGE;
?

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

Done.

+
+  return RendererTask::PrefixRendererTitle(
+      title,
+      extension->is_app(),
+      true,  // is_extension
+      web_contents->GetBrowserContext()->IsOffTheRecord(),
+      is_background);
+}
+
+}  // namespace task_management