Fix PyBrowser references living forever (#330).

api/Browser.md

@@ -5,8 +5,18 @@
 
 Remember to free all browser references for the browser to shut down cleanly.
 Otherwise data such as cookies or other storage might not be flushed to disk
-when closing app, and other issues might occur as well. To free a reference
-just assign a None value to a "browser" variable.
+when closing app, and other issues might occur as well. If you store
+a reference to Frame somewhere in your code then to free it just assign
+a None value to the variable.
+
+To compare browser objects always use [GetIdentifier()](#getidentifier)
+method. Do not compare two Browser objects variables directly. There
+are some edge cases when after the OnBeforeClose event browser objects
+are no more globally referenced thus a new instance is created that
+wraps upstream CefBrowser object. Browser objects that were globally
+unreferenced do not have properties of the original Browser object,
+for example they do not have client callbacks, javascript bindings
+or user data set.
 
 
 Table of contents:

api/Frame.md

@@ -3,6 +3,19 @@
 
 # Frame (object)
 
+Remember to free all frame references for the browser to shut down cleanly.
+Otherwise data such as cookies or other storage might not be flushed to disk
+when closing app, and other issues might occur as well. If you store
+a reference to Frame somewhere in your code then to free it just assign
+a None value to the variable.
+
+To compare frame objects always use [GetIdentifier()](#getidentifier)
+method. Do not compare two Frame objects variables directly. There
+are some edge cases when after the OnBeforeClose event frame objects
+are no more globally referenced thus a new instance is created that
+wraps upstream CefFrame object. Frame objects that were globally
+unreferenced do not have properties of the original Frame object.
+
 
 Table of contents:
 * [Methods](#methods)

api/JavascriptCallback.md

@@ -15,7 +15,9 @@ See also [Issue #11](../issues/11) (Throw JS / Python exceptions according to ex
 Table of contents:
 * [Methods](#methods)
   * [Call](#call)
-  * [GetName](#getname)
+  * [GetFrame](#getframe)
+  * [GetId](#getid)
+  * [GetFunctionName](#getfunctionname)
 
 
 ## Methods
@@ -30,10 +32,29 @@ Table of contents:
 
 Call the javascript callback function.
 
-For a list of allowed types for `mixed` see [JavascriptBindings](JavascriptBindings.md).IsValueAllowed().
+For a list of allowed types for `mixed` see JavascriptBindings.[IsValueAllowed()](JavascriptBindings.md#isvalueallowed).
 
 
-### GetName
+### GetFrame
+
+| | |
+| --- | --- |
+| __Return__ | [Frame](Frame.md) |
+
+Get Frame object associated with this callback. If Browser was destroyed
+then Frame may be None.
+
+
+### GetId
+
+| | |
+| --- | --- |
+| __Return__ | int |
+
+Get this callback's identifier.
+
+
+### GetFunctionName
 
 | | |
 | --- | --- |

src/browser.pyx

@@ -17,75 +17,109 @@ MOUSEBUTTON_RIGHT = cef_types.MBT_RIGHT
 # get segmentation faults, as they will be garbage collected.
 
 cdef dict g_pyBrowsers = {}
+
+# Unreferenced browsers are added to this list in OnBeforeClose().
+# Must keep a list of unreferenced browsers so that a new reference
+# is not created in GetPyBrowser() when browser was closed.
+cdef list g_unreferenced_browsers = []  # [int identifier, ..]
+
+# Browsers that are about to be closed are added to this list in
+# CloseBrowser().
 cdef list g_closed_browsers = []  # [int identifier, ..]
 
 cdef PyBrowser GetPyBrowserById(int browserId):
+    """May return None value so always check returned value."""
     if browserId in g_pyBrowsers:
         return g_pyBrowsers[browserId]
     return None
 
-cdef PyBrowser GetPyBrowser(CefRefPtr[CefBrowser] cefBrowser):
+cdef PyBrowser GetPyBrowser(CefRefPtr[CefBrowser] cefBrowser,
+                                    callerIdStr="GetPyBrowser"):
+    """The second argument 'callerIdStr' is so that a debug
+    message can be displayed informing which CEF handler callback
+    is being called to which an incomplete PyBrowser instance is
+    provided."""
+
     global g_pyBrowsers
+
+    # This probably ain't needed, but just to be sure.
     if <void*>cefBrowser == NULL or not cefBrowser.get():
-        # noinspection PyUnresolvedReferences
-        Debug("GetPyBrowser(): returning None")
-        return None
+        raise Exception("{caller}: CefBrowser reference is NULL"
+                        .format(caller=callerIdStr))
 
     cdef PyBrowser pyBrowser
     cdef int browserId
-    cdef int identifier
-
     browserId = cefBrowser.get().GetIdentifier()
+
     if browserId in g_pyBrowsers:
         return g_pyBrowsers[browserId]
 
+    # This code probably ain't needed.
+    # ----
+    cdef list toRemove = []
+    cdef int identifier
     for identifier, pyBrowser in g_pyBrowsers.items():
         if not pyBrowser.cefBrowser.get():
-            # noinspection PyUnresolvedReferences
-            Debug("GetPyBrowser(): removing an empty CefBrowser reference, "
-                  "browserId=%s" % identifier)
-            del g_pyBrowsers[identifier]
+            toRemove.append(identifier)
+    for identifier in toRemove:
+        Debug("GetPyBrowser(): removing an empty CefBrowser reference,"
+              " browserId=%s" % identifier)
+        RemovePyBrowser(identifier)
+    # ----
 
-    # noinspection PyUnresolvedReferences
-    Debug("GetPyBrowser(): creating new PyBrowser, browserId=%s" % browserId)
     pyBrowser = PyBrowser()
     pyBrowser.cefBrowser = cefBrowser
-    g_pyBrowsers[browserId] = pyBrowser
-
-    # Inherit client callbacks and javascript bindings
-    # from parent browser.
-
-    # Checking __outerWindowHandle as we should not inherit
-    # client callbacks and javascript bindings if the browser
-    # was created explicitily by calling CreateBrowserSync().
-
-    # Popups inherit client callbacks by default.
-
-    # Popups inherit javascript bindings only when "bindToPopups"
-    # constructor param was set to True.
 
     cdef WindowHandle openerHandle
     cdef dict clientCallbacks
     cdef JavascriptBindings javascriptBindings
     cdef PyBrowser tempPyBrowser
 
-    if pyBrowser.IsPopup() and \
-            not pyBrowser.GetUserData("__outerWindowHandle"):
-        openerHandle = pyBrowser.GetOpenerWindowHandle()
-        for identifier, tempPyBrowser in g_pyBrowsers.items():
-            if tempPyBrowser.GetWindowHandle() == openerHandle:
-                clientCallbacks = tempPyBrowser.GetClientCallbacksDict()
-                if clientCallbacks:
-                    pyBrowser.SetClientCallbacksDict(clientCallbacks)
-                javascriptBindings = tempPyBrowser.GetJavascriptBindings()
-                if javascriptBindings:
-                    if javascriptBindings.GetBindToPopups():
-                        pyBrowser.SetJavascriptBindings(javascriptBindings)
+    if browserId in g_unreferenced_browsers:
+        # This browser was already unreferenced due to OnBeforeClose
+        # was already called. An incomplete new instance of Browser
+        # object is created. This instance doesn't have the client
+        # callbacks, javascript bindings or user data that was already
+        # available in the original Browser object.
+        Debug("{caller}: Browser was already globally unreferenced"
+              ", a new incomplete instance is created, browser id={id}"
+              .format(caller=callerIdStr, id=str(browserId)))
+    else:
+        # This is first creation of browser. Store a reference globally
+        # and inherit client callbacks and javascript bindings from
+        # parent browsers.
+        Debug("GetPyBrowser(): create new PyBrowser, browserId=%s"
+              % browserId)
+
+        g_pyBrowsers[browserId] = pyBrowser
+
+        # Inherit client callbacks and javascript bindings
+        # from parent browser.
+        # - Checking __outerWindowHandle as we should not inherit
+        #   client callbacks and javascript bindings if the browser
+        #   was created explicitily by calling CreateBrowserSync().
+        # - Popups inherit client callbacks by default.
+        # - Popups inherit javascript bindings only when "bindToPopups"
+        #   constructor param was set to True.
+
+        if pyBrowser.IsPopup() and \
+                not pyBrowser.GetUserData("__outerWindowHandle"):
+            openerHandle = pyBrowser.GetOpenerWindowHandle()
+            for identifier, tempPyBrowser in g_pyBrowsers.items():
+                if tempPyBrowser.GetWindowHandle() == openerHandle:
+                    clientCallbacks = tempPyBrowser.GetClientCallbacksDict()
+                    if clientCallbacks:
+                        pyBrowser.SetClientCallbacksDict(clientCallbacks)
+                    javascriptBindings = tempPyBrowser.GetJavascriptBindings()
+                    if javascriptBindings:
+                        if javascriptBindings.GetBindToPopups():
+                            pyBrowser.SetJavascriptBindings(javascriptBindings)
+
     return pyBrowser
 
 cdef void RemovePyBrowser(int browserId) except *:
     # Called from LifespanHandler_OnBeforeClose().
-    global g_pyBrowsers
+    global g_pyBrowsers, g_unreferenced_browsers
     if browserId in g_pyBrowsers:
         if len(g_pyBrowsers) == 1:
             # This is the last browser remaining.
@@ -97,6 +131,7 @@ cdef void RemovePyBrowser(int browserId) except *:
         # noinspection PyUnresolvedReferences
         Debug("del g_pyBrowsers[%s]" % browserId)
         del g_pyBrowsers[browserId]
+        g_unreferenced_browsers.append(browserId)
     else:
         # noinspection PyUnresolvedReferences
         Debug("RemovePyBrowser() FAILED: browser not found, id = %s" \
@@ -116,7 +151,7 @@ cdef public void PyBrowser_ShowDevTools(CefRefPtr[CefBrowser] cefBrowser
     # Called from ClientHandler::OnContextMenuCommand
     cdef PyBrowser pyBrowser
     try:
-        pyBrowser = GetPyBrowser(cefBrowser)
+        pyBrowser = GetPyBrowser(cefBrowser, "ShowDevTools")
         pyBrowser.ShowDevTools()
     except:
         (exc_type, exc_value, exc_trace) = sys.exc_info()

src/cefpython.pyx

@@ -838,7 +838,8 @@ def CreateBrowserSync(windowInfo=None,
             requestContextHandler.get().SetBrowser(cefBrowser)
 
     cdef PyBrowser pyBrowser = GetPyBrowser(cefBrowser)
-    pyBrowser.SetUserData("__outerWindowHandle", int(windowInfo.parentWindowHandle))
+    pyBrowser.SetUserData("__outerWindowHandle",
+                          int(windowInfo.parentWindowHandle))
 
     return pyBrowser
 
@@ -889,6 +890,10 @@ def Shutdown():
     # Run some message loop work, force closing browsers and then run
     # some message loop work again for the browsers to close cleanly.
     #
+    # UPDATE: This code needs to be rechecked. There were enhancements
+    #         to unrferencing globally stored Browser objects in
+    #         g_pyBrowsers. See Issue #330 and its commits.
+    #
     # CASE 1:
     # There might be a case when python error occured after creating
     # browser, but before any message loop was run. In such case

src/frame.pyx

@@ -3,6 +3,7 @@
 # Project website: https://github.com/cztomczak/cefpython
 
 include "cefpython.pyx"
+include "browser.pyx"
 
 cdef dict g_pyFrames = {}
 
@@ -17,25 +18,51 @@ cdef PyFrame GetPyFrameById(int browserId, object frameId):
 
 cdef PyFrame GetPyFrame(CefRefPtr[CefFrame] cefFrame):
     global g_pyFrames
+
+    # This code probably ain't needed, but just to be sure.
     if <void*>cefFrame == NULL or not cefFrame.get():
-        Debug("GetPyFrame(): returning None")
-        return
+        raise Exception("GetPyFrame(): CefFrame reference is NULL")
+
     cdef PyFrame pyFrame
     cdef object frameId = cefFrame.get().GetIdentifier()  # int64
     cdef int browserId = cefFrame.get().GetBrowser().get().GetIdentifier()
     assert (frameId and browserId), "frameId or browserId empty"
     cdef object uniqueFrameId = GetUniqueFrameId(browserId, frameId)
+
     if uniqueFrameId in g_pyFrames:
         return g_pyFrames[uniqueFrameId]
+
+    # This code probably ain't needed.
+    # ----
+    cdef list toRemove = []
     for uFid, pyFrame in g_pyFrames.items():
         if not pyFrame.cefFrame.get():
-            Debug("GetPyFrame(): removing an empty CefFrame reference, " \
-                    "uniqueFrameId = %s" % uniqueFrameId)
-            del g_pyFrames[uFid]
-    # Debug("GetPyFrame(): creating new PyFrame, frameId=%s" % frameId)
+            toRemove.append(uFid)
+    for uFid in toRemove:
+        Debug("GetPyFrame(): removing an empty CefFrame reference, "
+              "uniqueFrameId = %s" % uniqueFrameId)
+        del g_pyFrames[uFid]
+    # ----
+
     pyFrame = PyFrame(browserId, frameId)
     pyFrame.cefFrame = cefFrame
-    g_pyFrames[uniqueFrameId] = pyFrame
+
+    if browserId in g_unreferenced_browsers:
+        # Browser was already globally unreferenced in OnBeforeClose,
+        # thus all frames are globally unreferenced too. Create a new
+        # incomplete instance of PyFrame object. Read comments in
+        # browser.pyx > GetPyBrowser and in Browser.md for what
+        # "incomplete" means.
+        pass
+    else:
+        # Keep a global reference to this frame only if the browser
+        # wasn't destroyed in OnBeforeClose. Otherwise we would leave
+        # dead frames references living forever.
+        # SIDE EFFECT: two calls to GetPyFrame for the same frame object
+        #              may return two different PyFrame objects. Compare
+        #              frame objects always using GetIdentifier().
+        # Debug("GetPyFrame(): create new PyFrame, frameId=%s" % frameId)
+        g_pyFrames[uniqueFrameId] = pyFrame
     return pyFrame
 
 cdef void RemovePyFrame(int browserId, object frameId) except *:

src/handlers/display_handler.pyx

@@ -3,6 +3,7 @@
 # Project website: https://github.com/cztomczak/cefpython
 
 include "../cefpython.pyx"
+include "../browser.pyx"
 
 cdef public void DisplayHandler_OnAddressChange(
         CefRefPtr[CefBrowser] cefBrowser,
@@ -14,7 +15,7 @@ cdef public void DisplayHandler_OnAddressChange(
     cdef py_string pyUrl
     cdef object callback
     try:
-        pyBrowser = GetPyBrowser(cefBrowser)
+        pyBrowser = GetPyBrowser(cefBrowser, "OnAddressChange")
         pyFrame = GetPyFrame(cefFrame)
         pyUrl = CefToPyString(cefUrl)
         callback = pyBrowser.GetClientCallback("OnAddressChange")
@@ -32,7 +33,7 @@ cdef public void DisplayHandler_OnTitleChange(
     cdef py_string pyTitle
     cdef object callback
     try:
-        pyBrowser = GetPyBrowser(cefBrowser)
+        pyBrowser = GetPyBrowser(cefBrowser, "OnTitleChange")
         pyTitle = CefToPyString(cefTitle)
         callback = pyBrowser.GetClientCallback("OnTitleChange")
         if callback:
@@ -51,7 +52,7 @@ cdef public cpp_bool DisplayHandler_OnTooltip(
     cdef object callback
     cdef py_bool returnValue
     try:
-        pyBrowser = GetPyBrowser(cefBrowser)
+        pyBrowser = GetPyBrowser(cefBrowser, "OnTooltip")
         pyText = CefToPyString(cefText)
         pyTextOut = [pyText]
         callback = pyBrowser.GetClientCallback("OnTooltip")
@@ -73,7 +74,7 @@ cdef public void DisplayHandler_OnStatusMessage(
     cdef py_string pyValue
     cdef object callback
     try:
-        pyBrowser = GetPyBrowser(cefBrowser)
+        pyBrowser = GetPyBrowser(cefBrowser, "OnStatusMessage")
         pyValue = CefToPyString(cefValue)
         callback = pyBrowser.GetClientCallback("OnStatusMessage")
         if callback:
@@ -94,7 +95,7 @@ cdef public cpp_bool DisplayHandler_OnConsoleMessage(
     cdef py_bool returnValue
     cdef object callback
     try:
-        pyBrowser = GetPyBrowser(cefBrowser)
+        pyBrowser = GetPyBrowser(cefBrowser, "OnConsoleMessage")
         pyMessage = CefToPyString(cefMessage)
         pySource = CefToPyString(cefSource)
         callback = pyBrowser.GetClientCallback("OnConsoleMessage")