Ensuring interception of stream get determined by plugin path before checking mime type.

content/browser/loader/buffered_resource_handler.cc

 // Copyright (c) 2012 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 "content/browser/loader/buffered_resource_handler.h"
 
 #include <vector>
 
 #include "base/bind.h"
 #include "base/logging.h"
@@ skipping 279 matching lines @@
                          type_hint, &new_type);
 
   // SniffMimeType() returns false if there is not enough data to determine
   // the mime type. However, even if it returns false, it returns a new type
   // that is probably better than the current one.
   response_->head.mime_type.assign(new_type);
 
   return made_final_decision;
 }
 
+bool BufferedResourceHandler::IsHandledByPlugin(bool* defer, bool* result) {
+#if defined(ENABLE_PLUGINS)
+  *result = true;
+  bool stale;
+  WebPluginInfo plugin;
+  bool has_plugin = GetSupportingPlugin(&plugin, &stale);
+  if (stale) {
+    // Refresh the plugins asynchronously.
+    plugin_service_->GetPlugins(
+        base::Bind(&BufferedResourceHandler::OnPluginsLoaded,
+                   weak_ptr_factory_.GetWeakPtr()));
+    request()->LogBlockedBy("BufferedResourceHandler");
+    *defer = true;
+    return true;
+  }
+
+  if (has_plugin) {
+    if (plugin.type == WebPluginInfo::PLUGIN_TYPE_BROWSER_PLUGIN) {
+      // If it is a MimeHandlerView plugin, intercept the stream.
+      std::string payload;
+      scoped_ptr<ResourceHandler> handler(host_->MaybeInterceptAsStream(
+          plugin.path, request(), response_.get(), &payload));
+      if (handler) {
+        *result = UseAlternateNextHandler(handler.Pass(), payload);
+        return true;
+      }
+      return false;
+    } else {
+      return true;
+    }
+  }
+
+  // If we get here then we should try intercepting the stream for the old
+  // streamsPrivate extensions API. This API is deprecated and should go
+  // away.

[mmenke, 2015/03/24 14:44:31]

nit:  Don't use "we" in comments.

[raymes, 2015/03/25 00:26:32]

if execution reaches here

[Deepak, 2015/03/25 05:43:34]

Done.

+  std::string payload;
+  scoped_ptr<ResourceHandler> handler(host_->MaybeInterceptAsStream(
+      base::FilePath(), request(), response_.get(), &payload));
+  if (handler) {
+    *result = UseAlternateNextHandler(handler.Pass(), payload);

[raymes, 2015/03/25 00:26:32]

cancel_request = !UseAlternateNextHandler...

[Deepak, 2015/03/25 05:43:34]

Done.

+    return true;
+  }
+#endif
+  return false;
+}
+
 bool BufferedResourceHandler::SelectNextHandler(bool* defer) {
   DCHECK(!response_->head.mime_type.empty());
 
   ResourceRequestInfoImpl* info = GetRequestInfo();
   const std::string& mime_type = response_->head.mime_type;
 
   if (net::IsSupportedCertificateMimeType(mime_type)) {
     // Install certificate file.
     info->set_is_download(true);
     scoped_ptr<ResourceHandler> handler(
         new CertificateResourceHandler(request()));
     return UseAlternateNextHandler(handler.Pass(), std::string());
   }
 
   // Allow requests for object/embed tags to be intercepted as streams.
   if (info->GetResourceType() == content::RESOURCE_TYPE_OBJECT) {
     DCHECK(!info->allow_download());
-    std::string payload;
-    scoped_ptr<ResourceHandler> handler(
-        host_->MaybeInterceptAsStream(request(), response_.get(), &payload));
-    if (handler) {
-      DCHECK(!net::IsSupportedMimeType(mime_type));
-      return UseAlternateNextHandler(handler.Pass(), payload);
-    }
+    bool result;
+    if (IsHandledByPlugin(defer, &result))
+      return result;

[raymes, 2015/03/25 00:26:32]

Probably we should change this to:

bool cancel_request = false;
bool handled_by_plugin = IsHandledByPlugin(defer, cancel_request);
if (cancel_request)
  return false;
if (handled_by_plugin)
  return true;

mmenke: Does this sound reasonable to you?

[Deepak, 2015/03/25 05:43:34]

Done.

   }
 
   if (!info->allow_download())
     return true;
 
   // info->allow_download() == true implies
   // info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME or
   // info->GetResourceType() == RESOURCE_TYPE_SUB_FRAME.
   DCHECK(info->GetResourceType() == RESOURCE_TYPE_MAIN_FRAME ||
          info->GetResourceType() == RESOURCE_TYPE_SUB_FRAME);
 
   bool must_download = MustDownload();
   if (!must_download) {
     if (net::IsSupportedMimeType(mime_type))
       return true;
 
-    std::string payload;
-    scoped_ptr<ResourceHandler> handler(
-        host_->MaybeInterceptAsStream(request(), response_.get(), &payload));
-    if (handler) {
-      return UseAlternateNextHandler(handler.Pass(), payload);
-    }
-
-#if defined(ENABLE_PLUGINS)
-    bool stale;
-    bool has_plugin = HasSupportingPlugin(&stale);
-    if (stale) {
-      // Refresh the plugins asynchronously.
-      plugin_service_->GetPlugins(
-          base::Bind(&BufferedResourceHandler::OnPluginsLoaded,
-                     weak_ptr_factory_.GetWeakPtr()));
-      request()->LogBlockedBy("BufferedResourceHandler");
-      *defer = true;
-      return true;
-    }
-    if (has_plugin)
-      return true;
-#endif
+    bool result;
+    if (IsHandledByPlugin(defer, &result))
+      return result;
   }
 
   // Install download handler
   info->set_is_download(true);
   scoped_ptr<ResourceHandler> handler(
       host_->CreateResourceHandlerForDownload(
           request(),
           true,  // is_content_initiated
           must_download,
           DownloadItem::kInvalidId,
@@ skipping 96 matching lines @@
              host_->delegate()->ShouldForceDownloadResource(
                  request()->url(), response_->head.mime_type)) {
     must_download_ = true;
   } else {
     must_download_ = false;
   }
 
   return must_download_;
 }
 
-bool BufferedResourceHandler::HasSupportingPlugin(bool* stale) {
+bool BufferedResourceHandler::GetSupportingPlugin(WebPluginInfo* plugin,
+                                                  bool* stale) {
 #if defined(ENABLE_PLUGINS)

[mmenke, 2015/03/24 14:44:32]

Can we just wrap this method in this ifdef?  We never call it if enable plugins
is false, and I'm not a big fan of code that's never run and never tested. 
Looks fine now, but can easily imagine this method getting more complicated, and
then a future refactor unearthing a bug in the never used case.

Could either move the ifdef up here and add it in the header, or move it to an
anonymous namespace up top in this file (That does mean you'll have to pass in a
bunch of parameter, but it also makes clear that it has no side effects on the
BufferedResourceHandler - that's my preferred option, but feel free to
disagree).

Alternatively, could just inline it in your new method, as it's not a lot of
code.

[Deepak, 2015/03/25 05:43:34]

Have made the function inline.

   ResourceRequestInfoImpl* info = GetRequestInfo();
-
   bool allow_wildcard = false;
-  WebPluginInfo plugin;
   return plugin_service_->GetPluginInfo(
       info->GetChildID(), info->GetRenderFrameID(), info->GetContext(),
       request()->url(), GURL(), response_->head.mime_type, allow_wildcard,
-      stale, &plugin, NULL);
+      stale, plugin, NULL);
 #else
   if (stale)
     *stale = false;
   return false;
 #endif
 }
 
 bool BufferedResourceHandler::CopyReadBufferToNextHandler() {
   if (!read_buffer_.get())
     return true;
@@ skipping 13 matching lines @@
   request()->LogUnblocked();
   bool defer = false;
   if (!ProcessResponse(&defer)) {
     controller()->Cancel();
   } else if (!defer) {
     controller()->Resume();
   }
 }
 
 }  // namespace content