bindings: Add empty checks for toV8()

Source/web/WebPluginContainerImpl.cpp

 /*
  * Copyright (C) 2009 Google Inc. All rights reserved.
  * Copyright (C) 2014 Opera Software ASA. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions are
  * met:
  *
  *     * Redistributions of source code must retain the above copyright
  * notice, this list of conditions and the following disclaimer.
@@ skipping 445 matching lines @@
         return v8::Local<v8::Object>();
 
     if (!frame->script().canExecuteScripts(NotAboutToExecuteScript))
         return v8::Local<v8::Object>();
 
     ScriptState* scriptState = ScriptState::forMainWorld(frame);
     if (!scriptState->contextIsValid())
         return v8::Local<v8::Object>();
 
     v8::Handle<v8::Value> v8value = toV8(m_element.get(), scriptState->context()->Global(), scriptState->isolate());
-    ASSERT(v8value->IsObject());
+    ASSERT(!v8value.IsEmpty() && v8value->IsObject());

[haraken, 2015/04/28 04:24:59]

Who guarantees it is not empty?

Also we can remove the IsObject check.

[bashi, 2015/04/28 05:41:53]

The existing ASSERT() implies this should not be empty, maybe?

[haraken, 2015/04/28 05:50:34]

I think that is exactly what we're fixing with introducing the Maybe version. We
need to assume that any V8 API that returns a Maybe can return an empty handle.

[bashi, 2015/04/28 06:41:14]

Yeah, let's use !IsEmpty() assertion to see this assumption is correct. I'll do
immediate revert if this is a problem.

[Yuki, 2015/04/28 06:43:46]

The problem is, ASSERT will be noop in release build, I think.
How can we detect the problem in the real world?

[haraken, 2015/04/28 06:46:08]

If you argue that we can go with ASSERT here, that means we can go with ASSERTs
in many other cases. (And the ASSERTs will rarely hit.)

IMHO, we should avoid using ASSERTs (or ToLocalChecked) unless we're pretty sure
that the API should not return an empty handle. Also we should limit the places.

[bashi, 2015/04/28 06:55:49]

Ok, changed to return empty handle.

That's said, I'd prefer using ASSERTs or immediate crashes if possible because
returning empty handles spreads out the need for empty checks. That's what I'm
suffering from now :(

 
     return v8::Handle<v8::Object>::Cast(v8value);
 }
 
 WebString WebPluginContainerImpl::executeScriptURL(const WebURL& url, bool popupsAllowed)
 {
     LocalFrame* frame = m_element->document().frame();
     if (!frame)
         return WebString();
 
@@ skipping 534 matching lines @@
     for (size_t i = 0; i < cutOutRects.size(); i++)
         cutOutRects[i].move(-frameRect().x(), -frameRect().y());
 }
 
 bool WebPluginContainerImpl::pluginShouldPersist() const
 {
     return m_webPlugin->shouldPersist();
 }
 
 } // namespace blink