Pepper: add validity checks in RendererPpapiHostImpl::GetDocumentURL

content/renderer/pepper/renderer_ppapi_host_impl.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/renderer/pepper/renderer_ppapi_host_impl.h"
 
 #include "base/bind.h"
 #include "base/files/file_path.h"
 #include "base/logging.h"
 #include "base/message_loop/message_loop.h"
@@ skipping 242 matching lines @@
         FROM_HERE,
         base::Bind(callback, std::vector<int>(nested_msgs.size(), 0)));
   } else {
     browser_connection->SendBrowserCreate(
         module_->GetPluginChildId(), instance, nested_msgs, callback);
   }
 }
 
 GURL RendererPpapiHostImpl::GetDocumentURL(PP_Instance instance) const {
   PepperPluginInstanceImpl* instance_object = GetAndValidateInstance(instance);
-  if (!instance_object || !instance_object->container())
+  if (!instance_object)
     return GURL();
 
-  return instance_object->container()->element().document().url();
+  blink::WebPluginContainer* container = instance_object->container();
+  if (!container)
+    return GURL();

[bbudge, 2015/03/16 21:50:33]

You could replace the lines above with a single call to GetContainerForInstance
above.

[dmichael (off chromium), 2015/03/16 21:57:16]

Good call, done.

+
+  blink::WebElement element = container->element();
+  if (element.isNull())
+    return GURL();
+
+  blink::WebDocument document = element.document();
+  if (document.isNull())
+    return GURL();

[bbudge, 2015/03/16 21:50:33]

Are you sure we need these checks? There are a bunch of other places where we
don't check these for null:
https://code.google.com/p/chromium/codesearch#search/&q=%22container-%3Eeleme...

[dmichael (off chromium), 2015/03/16 21:57:16]

Absolutely not sure. I don't know how we can get in this state :-(. Please see
the last comment in the bug. This is mostly speculative.
https://code.google.com/p/chromium/issues/detail?id=465765#c37

[bbudge, 2015/03/16 22:11:29]

Looking at other usages, it seems unlikely, but OK.

[dmichael (off chromium), 2015/03/16 22:16:48]

I agree with you. I'm partly doing this to get more information...  if the crash
persists, it will rule out that these are Null.

+
+  return document.url();
 }
 
 PepperPluginInstanceImpl* RendererPpapiHostImpl::GetAndValidateInstance(
     PP_Instance pp_instance) const {
   PepperPluginInstanceImpl* instance =
       HostGlobals::Get()->GetInstance(pp_instance);
   if (!instance)
     return NULL;
   if (!instance->IsValidInstanceOf(module_))
     return NULL;
   return instance;
 }
 
 }  // namespace content