Request the icon of a Web Notification in the renderer process.

content/renderer/notification_provider.cc

Index: content/renderer/notification_provider.cc
diff --git a/content/renderer/notification_provider.cc b/content/renderer/notification_provider.cc
index c4d869c2b1caea82e31f46e5997d5e64675c4808..bf991a693f37dc64ec2f1f7b6f9669ffa764da67 100644
--- a/content/renderer/notification_provider.cc
+++ b/content/renderer/notification_provider.cc
@@ -5,13 +5,18 @@
 #include "content/renderer/notification_provider.h"
 
 #include "base/strings/string_util.h"
+#include "content/child/image_decoder.h"
 #include "content/common/desktop_notification_messages.h"
 #include "content/common/frame_messages.h"
 #include "content/renderer/render_frame_impl.h"
+#include "third_party/WebKit/public/platform/Platform.h"
 #include "third_party/WebKit/public/platform/WebURL.h"
+#include "third_party/WebKit/public/platform/WebURLLoader.h"
+#include "third_party/WebKit/public/platform/WebURLLoaderClient.h"
 #include "third_party/WebKit/public/web/WebDocument.h"
 #include "third_party/WebKit/public/web/WebLocalFrame.h"
 #include "third_party/WebKit/public/web/WebUserGestureIndicator.h"
+#include "third_party/skia/include/core/SkBitmap.h"
 
 using blink::WebDocument;
 using blink::WebNotification;
@@ -19,38 +24,197 @@ using blink::WebNotificationPresenter;
 using blink::WebSecurityOrigin;
 using blink::WebString;
 using blink::WebURL;
+using blink::WebURLError;
+using blink::WebURLLoader;
+using blink::WebURLRequest;
 using blink::WebUserGestureIndicator;
 
 namespace content {
 
-NotificationProvider::NotificationProvider(RenderFrame* render_frame)
-    : RenderFrameObserver(render_frame) {
+// NotificationProvider::IconDownloader ----------------------------------------
+
+// Downloads the icon associated with a notification and decodes the received
+// image. This must be completed before notifications are shown to the user.
+class NotificationProvider::IconDownloader : public blink::WebURLLoaderClient {
+  typedef base::Callback<void(const WebNotification&,
+                              const SkBitmap&)> DownloadCompletedCallback;
+
+ public:
+  IconDownloader(const WebNotification& notification,
+                 const DownloadCompletedCallback& callback);
+
+  virtual ~IconDownloader();
+
+  // Downloads |image_url| and invokes |callback| with the decoded SkBitmap when

[Mike West, 2014/09/18 19:48:42]

There's not an 'image_url' parameter anywhere. Perhaps "... the notification's
image"?

[Peter Beverloo, 2014/09/19 14:15:48]

Done.

+  // the download has succeeded, or invokes |callback| with an empty SkBitmap
+  // in case the download has failed.
+  void Start();
+
+  // Cancels the current image download. The callback will not be invoked.
+  void Cancel();
+
+  // blink::WebURLLoaderClient implementation.
+  virtual void didReceiveData(WebURLLoader* loader,
+                              const char* data,
+                              int data_length,
+                              int encoded_data_length) OVERRIDE;
+  virtual void didFinishLoading(WebURLLoader* loader,
+                                double finish_time,
+                                int64_t total_encoded_data_length) OVERRIDE;
+  virtual void didFail(WebURLLoader* loader, const WebURLError& error) OVERRIDE;
+
+  const WebNotification& notification() const {
+    return notification_;
+  }
+
+ private:
+  WebNotification notification_;
+  DownloadCompletedCallback callback_;
+
+  scoped_ptr<WebURLLoader> loader_;
+  bool completed_;
+
+  std::string buffer_;
+
+  DISALLOW_COPY_AND_ASSIGN(IconDownloader);
+};
+
+NotificationProvider::IconDownloader::IconDownloader(
+    const WebNotification& notification,
+    const DownloadCompletedCallback& callback)
+    : notification_(notification),
+      callback_(callback),
+      completed_(false) {}
+
+NotificationProvider::IconDownloader::~IconDownloader() {}
+
+void NotificationProvider::IconDownloader::Start() {
+  DCHECK(!loader_);
+
+  WebURLRequest request(notification_.iconURL());

[Mike West, 2014/09/18 19:48:42]

Please set the request's context. It looks like it ought to be
'WebURLRequest::RequestContextImage'.

[Peter Beverloo, 2014/09/19 14:15:48]

Done.

+
+  loader_.reset(blink::Platform::current()->createURLLoader());

[Mike West, 2014/09/18 19:48:42]

Should this be an AssociatedURLLoader? I'd imagine that an extension's CSP
should govern whether or not it could request a particular URL as a notification
icon, for instance.

[Peter Beverloo, 2014/09/19 14:15:48]

Notifications won't be associated with a RenderFrame/RenderView anymore (this
class is going to move to //content/child/), and there is no frame to associate
it with.

Does CSP apply to requests done from service workers? If so, how do I get and
apply the constraints?

[Mike West, 2014/09/19 15:00:28]

Hrm. That's a bit surprising. I thought notifications were tied to an
ExecutionContext, so that they had something to hook onto for message passing. I
assumed that meant that they were tied to a RenderFrameHost on the //content
side. Is that not the case? How is that going to work in the future?

+  loader_->loadAsynchronously(request, this);
+}
+
+void NotificationProvider::IconDownloader::Cancel() {
+  DCHECK(loader_);
+
+  completed_ = true;
+  loader_->cancel();

[Mike West, 2014/09/18 19:48:42]

You don't do anything with the contents of |buffer_| here. It seems like you
should clear it?

[Peter Beverloo, 2014/09/19 14:15:48]

buffer_ is unlikely to contain sensitive data, and since it's a member of
ImageDownloader it gets cleared when the objects itself gets destroyed (which
happens immediately after we invoke the callback, or when Cancel() gets called).

Do you want me to clear it manually anyway?

[Mike West, 2014/09/19 15:00:28]

My worry was that the load could cancel, get restarted somehow, and end up with
corrupted buffer. Less a security issue, more a correctness issue. It's not a
huge deal if we're always going to tear down the object after calling Cancel.
Could you add a comment to that effect in the header so that it's clear that
that's the intended usage?

+}
+
+void NotificationProvider::IconDownloader::didReceiveData(
+    WebURLLoader* loader,
+    const char* data,
+    int data_length,
+    int encoded_data_length) {
+  DCHECK(!completed_);
+  DCHECK(data_length > 0);
+
+  buffer_.append(data, data_length);
+}
+
+void NotificationProvider::IconDownloader::didFinishLoading(
+    WebURLLoader* loader,
+    double finish_time,
+    int64_t total_encoded_data_length) {
+  DCHECK(!completed_);
+
+  ImageDecoder decoder;
+  SkBitmap icon = decoder.Decode(
+      reinterpret_cast<const unsigned char*>(buffer_.data()), buffer_.size());
+
+  callback_.Run(notification_, icon);
+  completed_ = true;
 }
 
-NotificationProvider::~NotificationProvider() {
+void NotificationProvider::IconDownloader::didFail(
+    WebURLLoader* loader, const WebURLError& error) {
+  if (completed_)

[dewittj, 2014/09/18 17:46:49]

Is this the correct failure behavior?  In extensions we don't allow
notifications without an icon.

[Peter Beverloo, 2014/09/19 14:15:48]

The Web Notification API doesn't imply restrictions on this. It also raises
questions about what to do when the image is specified, but couldn't be decoded
successfully.

I don't think we're changing behavior here. It's worth revisiting when we start
discussions about supporting icons of multiple resolutions, though.

+    return;
+
+  callback_.Run(notification_, SkBitmap());
+  completed_ = true;

[Mike West, 2014/09/18 19:48:42]

Again, it seems like you should do something with |buffer_| here?

[Peter Beverloo, 2014/09/19 14:15:48]

Done.

 }
 
+// NotificationProvider --------------------------------------------------------
+
+NotificationProvider::NotificationProvider(RenderFrame* render_frame)
+    : RenderFrameObserver(render_frame) {}
+
+NotificationProvider::~NotificationProvider() {}
+
 bool NotificationProvider::show(const WebNotification& notification) {
+  if (notification.iconURL().isEmpty()) {
+    DisplayNotification(notification, SkBitmap());
+    return true;
+  }
+
+  scoped_ptr<IconDownloader> downloader(
+      new IconDownloader(notification,
+          base::Bind(&NotificationProvider::DisplayNotification,
+                     base::Unretained(this))));
+
+  downloader->Start();
+
+  pending_notifications_.push_back(downloader.release());
+  return true;
+}
+
+void NotificationProvider::DisplayNotification(
+    const WebNotification& notification, const SkBitmap& icon) {
   WebDocument document = render_frame()->GetWebFrame()->document();
   int notification_id = manager_.RegisterNotification(notification);
 
+  RemovePendingNotification(notification);
+
+  PendingNotifications::iterator iter = pending_notifications_.begin();
+  for (; iter != pending_notifications_.end(); ++iter) {
+    if ((*iter)->notification() != notification)
+      continue;
+
+    pending_notifications_.erase(iter);
+    break;
+  }
+
   ShowDesktopNotificationHostMsgParams params;
   params.origin = GURL(document.securityOrigin().toString());
-  params.icon_url = notification.iconURL();
+  params.icon = icon;
   params.title = notification.title();
   params.body = notification.body();
   params.direction = notification.direction();
   params.replace_id = notification.replaceId();
-  return Send(new DesktopNotificationHostMsg_Show(
-      routing_id(), notification_id, params));
+
+  Send(new DesktopNotificationHostMsg_Show(routing_id(),
+                                           notification_id,
+                                           params));
+}
+
+// Removes the IconDownload object from the list of pending notifications for
+// |notification|. If the download is still in process, it will be cancelled
+// by the blink::WebURLLoaderClient destructor.
+void NotificationProvider::RemovePendingNotification(
+    const WebNotification& notification) {
+  PendingNotifications::iterator iter = pending_notifications_.begin();
+  for (; iter != pending_notifications_.end(); ++iter) {
+    if ((*iter)->notification() != notification)
+      continue;
+
+    pending_notifications_.erase(iter);
+    break;
+  }
 }
 
 void NotificationProvider::cancel(const WebNotification& notification) {
   int id;
   bool id_found = manager_.GetId(notification, id);
-  // Won't be found if the notification has already been closed by the user.
+  // Won't be found if the notification has already been closed by the user,
+  // or if the notification's icon is still being requested.
   if (id_found)
     Send(new DesktopNotificationHostMsg_Cancel(routing_id(), id));
+  else
+    RemovePendingNotification(notification);
 }
 
 void NotificationProvider::objectDestroyed(
@@ -60,6 +224,8 @@ void NotificationProvider::objectDestroyed(
   // Won't be found if the notification has already been closed by the user.
   if (id_found)
     manager_.UnregisterNotification(id);
+  else
+    RemovePendingNotification(notification);

[dewittj, 2014/09/18 17:46:49]

Is this correct?  (I'm unfamiliar with blink code)

but it seems like the notification should continue to be shown even if the
object is destroyed before the icon downloads.  Possibly need to increment ref
count of the object while it's downloading the icon?

[Peter Beverloo, 2014/09/19 14:15:48]

We'd only cancel showing the notification if the image is still being requested,
which is extremely unlikely for document contexts.

We're going to have to be smarter with this when enabling notifications to be
displayed from workers, given that a scenario like the following isn't unlikely:

service_worker.js
=========================
onpush = function(event) {
  new Notification('You have received a message!', {
    message: event.data.fullName + ' says hello.',
    icon: 'https://my-site.com/icon.png',

    serviceWorker: serviceWorkerRegistration
  });
};
=========================

In which case the Push event would release their reference on the SW immediately
after the event finished invoking, which is likely to kill it.

Doing this cleanly requires us to separate the notification data from the
delegate, which I plan on doing as part of an upcoming Blink refactoring for Web
Worker support (I uploaded a WIP here:
https://codereview.chromium.org/584023002).

 }
 
 WebNotificationPresenter::Permission NotificationProvider::checkPermission(