Fix errors due to NULL frames sent from Renderer process (#431).

cztomczak · cztomczak · commit a010e6819e93 · 2018-08-17T17:40:46.000+02:00
Remove Python callback references properly.

Fix PyCharm code inspection warnings in main_test.py.
diff --git a/src/client_handler/client_handler.cpp b/src/client_handler/client_handler.cpp
@@ -30,6 +30,7 @@ bool ClientHandler::OnProcessMessageReceived(
                                         CefProcessId source_process,
                                         CefRefPtr<CefProcessMessage> message)
 {
+    // Return true if message was handled.
     if (source_process != PID_RENDERER) {
         return false;
     }
@@ -42,6 +43,12 @@ bool ClientHandler::OnProcessMessageReceived(
         if (arguments->GetSize() == 1 && arguments->GetType(0) == VTYPE_INT) {
             int64 frameId = arguments->GetInt(0);
             CefRefPtr<CefFrame> frame = browser->GetFrame(frameId);
+            if (!frame.get()) {
+                // Frame was already destroyed while IPC messaging was
+                // executing. Issue #431. User callback will not be
+                // executed in such case.
+                return true;
+            }
             V8ContextHandler_OnContextCreated(browser, frame);
             return true;
         } else {
@@ -56,6 +63,11 @@ bool ClientHandler::OnProcessMessageReceived(
                 && arguments->GetType(1) == VTYPE_INT) {
             int browserId = arguments->GetInt(0);
             int64 frameId = arguments->GetInt(1);
+            // Even if frame was alrady destroyed (Issue #431) you still
+            // want to call V8ContextHandler_OnContextReleased as it releases
+            // some resources. Thus passing IDs instead of actual
+            // objects. Cython code in V8ContextHandler_OnContextReleased
+            // will handle a case when frame is already destroyed.
             V8ContextHandler_OnContextReleased(browserId, frameId);
             return true;
         } else {
@@ -75,8 +87,13 @@ bool ClientHandler::OnProcessMessageReceived(
             int64 frameId = arguments->GetInt(0);
             CefString functionName = arguments->GetString(1);
             CefRefPtr<CefListValue> functionArguments = arguments->GetList(2);
-            CefRefPtr<CefFrame> frame = browser->GetFrame(frameId);
-            V8FunctionHandler_Execute(browser, frame, functionName,
+            // Even if frame was already destroyed (Issue #431) you still
+            // want to call V8FunctionHandler_Execute, as it can run
+            // Python code without issues and doesn't require an actual
+            // frame. Thus passing IDs instead of actual objects. Cython
+            // code in V8FunctionHandler_Execute will handle a case when
+            // frame is already destroyed.
+            V8FunctionHandler_Execute(browser, frameId, functionName,
                                       functionArguments);
             return true;
         } else {
@@ -100,18 +117,6 @@ bool ClientHandler::OnProcessMessageReceived(
                           " messageName=ExecutePythonCallback";
             return false;
         }
-    } else if (messageName == "RemovePythonCallbacksForFrame") {
-        CefRefPtr<CefListValue> arguments = message->GetArgumentList();
-        if (arguments->GetSize() == 1 && arguments->GetType(0) == VTYPE_INT) {
-            int frameId = arguments->GetInt(0);
-            RemovePythonCallbacksForFrame(frameId);
-            return true;
-        } else {
-            LOG(ERROR) << "[Browser process] OnProcessMessageReceived():"
-                          " invalid arguments,"
-                          " messageName=ExecutePythonCallback";
-            return false;
-        }
     }
     return false;
 }
diff --git a/src/frame.pyx b/src/frame.pyx
@@ -93,6 +93,7 @@ cdef void RemovePyFrame(int browserId, object frameId) except *:
         del pyFrame
         del g_pyFrames[uniqueFrameId]
         g_unreferenced_frames.append(uniqueFrameId)
+        RemovePythonCallbacksForFrame(frameId)
     else:
         Debug("RemovePyFrame() FAILED: uniqueFrameId = %s" % uniqueFrameId)
 
@@ -112,6 +113,8 @@ cdef void RemovePyFramesForBrowser(int browserId) except *:
         del pyFrame
         del g_pyFrames[uniqueFrameId]
         g_unreferenced_frames.append(uniqueFrameId)
+    # RemovePythonCallbacksForBrowser already called
+    # in LifespanHandler_OnBeforeClose.
 
 cdef class PyFrame:
     cdef CefRefPtr[CefFrame] cefFrame
diff --git a/src/handlers/v8context_handler.pyx b/src/handlers/v8context_handler.pyx
@@ -10,6 +10,7 @@
 
 include "../cefpython.pyx"
 include "../browser.pyx"
+include "../frame.pyx"
 
 cdef public void V8ContextHandler_OnContextCreated(
         CefRefPtr[CefBrowser] cefBrowser,
@@ -23,7 +24,7 @@ cdef public void V8ContextHandler_OnContextCreated(
         pyBrowser = GetPyBrowser(cefBrowser, "OnContextCreated")
         pyBrowser.SetUserData("__v8ContextCreated", True)
         pyFrame = GetPyFrame(cefFrame)
-        # User defined callback.
+        # User defined callback
         clientCallback = pyBrowser.GetClientCallback("OnContextCreated")
         if clientCallback:
             clientCallback(browser=pyBrowser, frame=pyFrame)
@@ -52,19 +53,15 @@ cdef public void V8ContextHandler_OnContextReleased(
         if not pyBrowser:
             Debug("OnContextReleased: Browser doesn't exist anymore, id={id}"
                   .format(id=str(browserId)))
+            RemovePyFrame(browserId, frameId)
             return
         pyFrame = GetPyFrameById(browserId, frameId)
-        if pyBrowser and pyFrame:
+        # Frame may already be destroyed while IPC messaging was executing
+        # (Issue #431).
+        if pyFrame:
             clientCallback = pyBrowser.GetClientCallback("OnContextReleased")
             if clientCallback:
                 clientCallback(browser=pyBrowser, frame=pyFrame)
-        else:
-            if not pyBrowser:
-                Debug("V8ContextHandler_OnContextReleased() WARNING: "
-                        "PyBrowser not found")
-            if not pyFrame:
-                Debug("V8ContextHandler_OnContextReleased() WARNING: "
-                        "PyFrame not found")
         RemovePyFrame(browserId, frameId)
     except:
         (exc_type, exc_value, exc_trace) = sys.exc_info()
diff --git a/src/handlers/v8function_handler.pyx b/src/handlers/v8function_handler.pyx
@@ -4,46 +4,48 @@
 
 include "../cefpython.pyx"
 include "../browser.pyx"
+include "../frame.pyx"
 
 cdef public void V8FunctionHandler_Execute(
         CefRefPtr[CefBrowser] cefBrowser,
-        CefRefPtr[CefFrame] cefFrame,
-        CefString& cefFunctionName,
-        CefRefPtr[CefListValue] cefFunctionArguments
+        int64 frameId,
+        CefString& cefFuncName,
+        CefRefPtr[CefListValue] cefFuncArgs
         ) except * with gil:
     cdef PyBrowser pyBrowser
-    cdef PyFrame pyFrame
-    cdef py_string functionName
-    cdef object function
-    cdef list functionArguments
+    cdef CefRefPtr[CefFrame] cefFrame
+    cdef PyFrame pyFrame  # may be None
+    cdef py_string funcName
+    cdef object func
+    cdef list funcArgs
     cdef object returnValue
-    cdef py_string jsErrorMessage
+    cdef py_string errorMessage
     try:
         pyBrowser = GetPyBrowser(cefBrowser, "V8FunctionHandler_Execute")
-        pyFrame = GetPyFrame(cefFrame)
-        functionName = CefToPyString(cefFunctionName)
-        Debug("V8FunctionHandler_Execute(): functionName=%s" % functionName)
+        cefFrame = cefBrowser.get().GetFrame(frameId)
+        if cefFrame.get():
+            pyFrame = GetPyFrame(cefFrame)
+        else:
+            pyFrame = None
+        funcName = CefToPyString(cefFuncName)
+        Debug("V8FunctionHandler_Execute(): funcName=%s" % funcName)
         jsBindings = pyBrowser.GetJavascriptBindings()
-        function = jsBindings.GetFunctionOrMethod(functionName)
-        if not function:
+        func = jsBindings.GetFunctionOrMethod(funcName)
+        if not func:
             # The Renderer process already checks whether function
             # name is valid before calling V8FunctionHandler_Execute(),
             # but it is possible for the javascript bindings to change
             # during execution, so it's possible for the Browser/Renderer
             # bindings to be out of sync due to delay in process messaging.
-            jsErrorMessage = "V8FunctionHandler_Execute() FAILED: " \
-                    "python function not found: %s" % functionName
-            Debug(jsErrorMessage)
-            # Raise a javascript exception in that frame.
-            pyFrame.ExecuteJavascript("throw '%s';" % jsErrorMessage)
+            errorMessage = "V8FunctionHandler_Execute() FAILED: " \
+                           "python function not found: %s" % funcName
+            NonCriticalError(errorMessage)
+            # Raise a javascript exception in that frame if it still exists
+            if pyFrame:
+                pyFrame.ExecuteJavascript("throw '%s';" % errorMessage)
             return
-        functionArguments = CefListValueToPyList(cefBrowser, 
-                cefFunctionArguments)
-        returnValue = function(*functionArguments)
-        if returnValue is not None:
-            Debug("V8FunctionHandler_Execute() WARNING: function returned" \
-                    "value, but returning values to javascript is not " \
-                    "supported, functionName=%s" % functionName)
+        funcArgs = CefListValueToPyList(cefBrowser, cefFuncArgs)
+        func(*funcArgs)
     except:
         (exc_type, exc_value, exc_trace) = sys.exc_info()
         sys.excepthook(exc_type, exc_value, exc_trace)
diff --git a/src/python_callback.pyx b/src/python_callback.pyx
@@ -5,18 +5,20 @@
 include "cefpython.pyx"
 
 cdef int g_pythonCallbackMaxId = 0
+# [callbackId] = (browserId, frameId, func)
 cdef dict g_pythonCallbacks = {}
 
 # TODO: send callbackId using CefBinaryNamedValue, see:
-# http://www.magpcss.org/ceforum/viewtopic.php?f=6&t=10881
+#       http://www.magpcss.org/ceforum/viewtopic.php?f=6&t=10881
+
 cdef struct PythonCallback:
     int callbackId
     char uniqueCefBinaryValueSize[16]
 
 cdef CefRefPtr[CefBinaryValue] PutPythonCallback(
         object browserId,
         object frameId, 
-        object function
+        object func
         ) except *:
     global g_pythonCallbacks
     global g_pythonCallbackMaxId
@@ -29,12 +31,11 @@ cdef CefRefPtr[CefBinaryValue] PutPythonCallback(
     pyCallback.callbackId = g_pythonCallbackMaxId
     cdef CefRefPtr[CefBinaryValue] binaryValue = CefBinaryValue_Create(
             &pyCallback, sizeof(pyCallback))
-    # [0] browserId, [1] frameId, [2] function.
-    g_pythonCallbacks[g_pythonCallbackMaxId] = (browserId, frameId, function)
+    g_pythonCallbacks[g_pythonCallbackMaxId] = (browserId, frameId, func)
     return binaryValue
 
 cdef public void RemovePythonCallbacksForFrame(
-        int frameId
+        object frameId
         ) except * with gil:
     # Cannot remove elements from g_pythonCallbacks (dict) while iterating.
     cdef list toRemove = []
@@ -46,8 +47,8 @@ cdef public void RemovePythonCallbacksForFrame(
         for callbackId in toRemove:
             del g_pythonCallbacks[callbackId]
             Debug("RemovePythonCallbacksForFrame(): " \
-                    "removed python callback, callbackId = %s" \
-                    % callbackId)
+                  "removed python callback, callbackId = %s" \
+                  % callbackId)
     except:
         (exc_type, exc_value, exc_trace) = sys.exc_info()
         sys.excepthook(exc_type, exc_value, exc_trace)
@@ -62,33 +63,28 @@ cdef void RemovePythonCallbacksForBrowser(
     for callbackId in toRemove:
         del g_pythonCallbacks[callbackId]
         Debug("RemovePythonCallbacksForBrowser(): " \
-                "removed python callback, callbackId = %s" \
-                % callbackId)
+              "removed python callback, callbackId = %s" \
+              % callbackId)
 
 cdef public cpp_bool ExecutePythonCallback(
         CefRefPtr[CefBrowser] cefBrowser,
         int callbackId, 
-        CefRefPtr[CefListValue] cefFunctionArguments,
+        CefRefPtr[CefListValue] cefFuncArgs,
         ) except * with gil:
-    cdef object function
-    cdef list functionArguments
+    cdef object func
+    cdef list funcArgs
     cdef object returnValue
     try:
         global g_pythonCallbacks
         if callbackId in g_pythonCallbacks:
-            # [0] browserId, [1] frameId, [2] function.
-            function = g_pythonCallbacks[callbackId][2]
-            functionArguments = CefListValueToPyList(
-                    cefBrowser, cefFunctionArguments)
-            returnValue = function(*functionArguments)
-            if returnValue is not None:
-                Debug("ExecutePythonCallback() WARNING: function returned" \
-                        "value, but returning values to javascript is not " \
-                        "supported, function name = %s" % function.__name__)
+            func = g_pythonCallbacks[callbackId][2]
+            funcArgs = CefListValueToPyList(
+                    cefBrowser, cefFuncArgs)
+            func(*funcArgs)
             return True
         else:
             Debug("ExecutePythonCallback() FAILED: callback not found, " \
-                    "callbackId = %s" % callbackId)
+                  "callbackId = %s" % callbackId)
             return False
     except:
         (exc_type, exc_value, exc_trace) = sys.exc_info()
diff --git a/src/subprocess/cefpython_app.cpp b/src/subprocess/cefpython_app.cpp
@@ -337,15 +337,10 @@ void CefPythonApp::OnContextReleased(CefRefPtr<CefBrowser> browser,
     // ------------------------------------------------------------------------
     // 2. Remove python callbacks for a frame.
     // ------------------------------------------------------------------------
-    // If this is the main frame then the message won't arrive
-    // to the browser process, as browser is being destroyed,
-    // but it doesn't matter because in LifespanHandler_BeforeClose()
+    // This is already done via RemovePyFrame called from
+    // V8ContextHandler_OnContextReleased.
+    // If this is the main frame then in LifespanHandler_BeforeClose()
     // we're calling RemovePythonCallbacksForBrowser().
-    message = CefProcessMessage::Create("RemovePythonCallbacksForFrame");
-    arguments = message->GetArgumentList();
-    // TODO: int64 precision lost
-    arguments->SetInt(0, (int)(frame->GetIdentifier()));
-    browser->SendProcessMessage(PID_BROWSER, message);
     // ------------------------------------------------------------------------
     // 3. Clear javascript callbacks.
     // ------------------------------------------------------------------------
diff --git a/unittests/main_test.py b/unittests/main_test.py
@@ -154,13 +154,15 @@ def test_main(self):
         # supports passing functions as callbacks when called from
         # javascript, and as a side effect any value and in this case
         # a property can also be a function.
-        bindings.SetProperty("test_property3_function", external.test_property3_function)
+        bindings.SetProperty("test_property3_function",
+                             external.test_property3_function)
         bindings.SetProperty("cefpython_version", cef.GetVersion())
         bindings.SetObject("external", external)
         browser.SetJavascriptBindings(bindings)
         subtest_message("browser.SetJavascriptBindings() ok")
 
         # Test Request.SetPostData(list)
+        # noinspection PyArgumentList
         req = cef.Request.CreateRequest()
         req_file = os.path.dirname(os.path.abspath(__file__))
         req_file = os.path.join(req_file, "main_test.py")
@@ -173,6 +175,7 @@ def test_main(self):
         subtest_message("cef.Request.SetPostData(list) ok")
 
         # Test Request.SetPostData(dict)
+        # noinspection PyArgumentList
         req = cef.Request.CreateRequest()
         req_data = {b"key": b"value"}
         req.SetMethod("POST")
@@ -199,6 +202,7 @@ def test_main(self):
             time.sleep(0.01)
 
         # Automatic check of asserts in handlers and in external
+        # noinspection PyTypeChecker
         for obj in [] + client_handlers + [global_handler, external]:
             test_for_True = False  # Test whether asserts are working correctly
             for key, value in obj.__dict__.items():
