Skip to content

Commit 36faf91

Browse files
cztomczakcztomczak
committed
Fix PyBrowser references living forever (cztomczak#330).
1 parent 41fa14f commit 36faf91

18 files changed

+257
-107
lines changed

‎api/Browser.md‎

Lines changed: 12 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5,8 +5,18 @@
55

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

1121

1222
Table of contents:

‎api/Frame.md‎

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,19 @@
33

44
# Frame (object)
55

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

720
Table of contents:
821
* [Methods](#methods)

‎api/JavascriptCallback.md‎

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,9 @@ See also [Issue #11](../issues/11) (Throw JS / Python exceptions according to ex
1515
Table of contents:
1616
* [Methods](#methods)
1717
* [Call](#call)
18-
* [GetName](#getname)
18+
* [GetFrame](#getframe)
19+
* [GetId](#getid)
20+
* [GetFunctionName](#getfunctionname)
1921

2022

2123
## Methods
@@ -30,10 +32,29 @@ Table of contents:
3032

3133
Call the javascript callback function.
3234

33-
For a list of allowed types for `mixed` see [JavascriptBindings](JavascriptBindings.md).IsValueAllowed().
35+
For a list of allowed types for `mixed` see JavascriptBindings.[IsValueAllowed()](JavascriptBindings.md#isvalueallowed).
3436

3537

36-
### GetName
38+
### GetFrame
39+
40+
| | |
41+
| --- | --- |
42+
| __Return__ | [Frame](Frame.md) |
43+
44+
Get Frame object associated with this callback. If Browser was destroyed
45+
then Frame may be None.
46+
47+
48+
### GetId
49+
50+
| | |
51+
| --- | --- |
52+
| __Return__ | int |
53+
54+
Get this callback's identifier.
55+
56+
57+
### GetFunctionName
3758

3859
| | |
3960
| --- | --- |

‎src/browser.pyx‎

Lines changed: 74 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -17,75 +17,109 @@ MOUSEBUTTON_RIGHT = cef_types.MBT_RIGHT
1717
# get segmentation faults, as they will be garbage collected.
1818

1919
cdef dict g_pyBrowsers = {}
20+
21+
# Unreferenced browsers are added to this list in OnBeforeClose().
22+
# Must keep a list of unreferenced browsers so that a new reference
23+
# is not created in GetPyBrowser() when browser was closed.
24+
cdef list g_unreferenced_browsers = [] # [int identifier, ..]
25+
26+
# Browsers that are about to be closed are added to this list in
27+
# CloseBrowser().
2028
cdef list g_closed_browsers = [] # [int identifier, ..]
2129

2230
cdef PyBrowser GetPyBrowserById(int browserId):
31+
"""May return None value so always check returned value."""
2332
if browserId in g_pyBrowsers:
2433
return g_pyBrowsers[browserId]
2534
return None
2635

27-
cdef PyBrowser GetPyBrowser(CefRefPtr[CefBrowser] cefBrowser):
36+
cdef PyBrowser GetPyBrowser(CefRefPtr[CefBrowser] cefBrowser,
37+
callerIdStr="GetPyBrowser"):
38+
"""The second argument 'callerIdStr' is so that a debug
39+
message can be displayed informing which CEF handler callback
40+
is being called to which an incomplete PyBrowser instance is
41+
provided."""
42+
2843
global g_pyBrowsers
44+
45+
# This probably ain't needed, but just to be sure.
2946
if <void*>cefBrowser == NULL or not cefBrowser.get():
30-
# noinspection PyUnresolvedReferences
31-
Debug("GetPyBrowser(): returning None")
32-
return None
47+
raise Exception("{caller}: CefBrowser reference is NULL"
48+
.format(caller=callerIdStr))
3349

3450
cdef PyBrowser pyBrowser
3551
cdef int browserId
36-
cdef int identifier
37-
3852
browserId = cefBrowser.get().GetIdentifier()
53+
3954
if browserId in g_pyBrowsers:
4055
return g_pyBrowsers[browserId]
4156

57+
# This code probably ain't needed.
58+
# ----
59+
cdef list toRemove = []
60+
cdef int identifier
4261
for identifier, pyBrowser in g_pyBrowsers.items():
4362
if not pyBrowser.cefBrowser.get():
44-
# noinspection PyUnresolvedReferences
45-
Debug("GetPyBrowser(): removing an empty CefBrowser reference, "
46-
"browserId=%s" % identifier)
47-
del g_pyBrowsers[identifier]
63+
toRemove.append(identifier)
64+
for identifier in toRemove:
65+
Debug("GetPyBrowser(): removing an empty CefBrowser reference,"
66+
" browserId=%s" % identifier)
67+
RemovePyBrowser(identifier)
68+
# ----
4869

49-
# noinspection PyUnresolvedReferences
50-
Debug("GetPyBrowser(): creating new PyBrowser, browserId=%s" % browserId)
5170
pyBrowser = PyBrowser()
5271
pyBrowser.cefBrowser = cefBrowser
53-
g_pyBrowsers[browserId] = pyBrowser
54-
55-
# Inherit client callbacks and javascript bindings
56-
# from parent browser.
57-
58-
# Checking __outerWindowHandle as we should not inherit
59-
# client callbacks and javascript bindings if the browser
60-
# was created explicitily by calling CreateBrowserSync().
61-
62-
# Popups inherit client callbacks by default.
63-
64-
# Popups inherit javascript bindings only when "bindToPopups"
65-
# constructor param was set to True.
6672

6773
cdef WindowHandle openerHandle
6874
cdef dict clientCallbacks
6975
cdef JavascriptBindings javascriptBindings
7076
cdef PyBrowser tempPyBrowser
7177

72-
if pyBrowser.IsPopup() and \
73-
not pyBrowser.GetUserData("__outerWindowHandle"):
74-
openerHandle = pyBrowser.GetOpenerWindowHandle()
75-
for identifier, tempPyBrowser in g_pyBrowsers.items():
76-
if tempPyBrowser.GetWindowHandle() == openerHandle:
77-
clientCallbacks = tempPyBrowser.GetClientCallbacksDict()
78-
if clientCallbacks:
79-
pyBrowser.SetClientCallbacksDict(clientCallbacks)
80-
javascriptBindings = tempPyBrowser.GetJavascriptBindings()
81-
if javascriptBindings:
82-
if javascriptBindings.GetBindToPopups():
83-
pyBrowser.SetJavascriptBindings(javascriptBindings)
78+
if browserId in g_unreferenced_browsers:
79+
# This browser was already unreferenced due to OnBeforeClose
80+
# was already called. An incomplete new instance of Browser
81+
# object is created. This instance doesn't have the client
82+
# callbacks, javascript bindings or user data that was already
83+
# available in the original Browser object.
84+
Debug("{caller}: Browser was already globally unreferenced"
85+
", a new incomplete instance is created, browser id={id}"
86+
.format(caller=callerIdStr, id=str(browserId)))
87+
else:
88+
# This is first creation of browser. Store a reference globally
89+
# and inherit client callbacks and javascript bindings from
90+
# parent browsers.
91+
Debug("GetPyBrowser(): create new PyBrowser, browserId=%s"
92+
% browserId)
93+
94+
g_pyBrowsers[browserId] = pyBrowser
95+
96+
# Inherit client callbacks and javascript bindings
97+
# from parent browser.
98+
# - Checking __outerWindowHandle as we should not inherit
99+
# client callbacks and javascript bindings if the browser
100+
# was created explicitily by calling CreateBrowserSync().
101+
# - Popups inherit client callbacks by default.
102+
# - Popups inherit javascript bindings only when "bindToPopups"
103+
# constructor param was set to True.
104+
105+
if pyBrowser.IsPopup() and \
106+
not pyBrowser.GetUserData("__outerWindowHandle"):
107+
openerHandle = pyBrowser.GetOpenerWindowHandle()
108+
for identifier, tempPyBrowser in g_pyBrowsers.items():
109+
if tempPyBrowser.GetWindowHandle() == openerHandle:
110+
clientCallbacks = tempPyBrowser.GetClientCallbacksDict()
111+
if clientCallbacks:
112+
pyBrowser.SetClientCallbacksDict(clientCallbacks)
113+
javascriptBindings = tempPyBrowser.GetJavascriptBindings()
114+
if javascriptBindings:
115+
if javascriptBindings.GetBindToPopups():
116+
pyBrowser.SetJavascriptBindings(javascriptBindings)
117+
84118
return pyBrowser
85119

86120
cdef void RemovePyBrowser(int browserId) except *:
87121
# Called from LifespanHandler_OnBeforeClose().
88-
global g_pyBrowsers
122+
global g_pyBrowsers, g_unreferenced_browsers
89123
if browserId in g_pyBrowsers:
90124
if len(g_pyBrowsers) == 1:
91125
# This is the last browser remaining.
@@ -97,6 +131,7 @@ cdef void RemovePyBrowser(int browserId) except *:
97131
# noinspection PyUnresolvedReferences
98132
Debug("del g_pyBrowsers[%s]" % browserId)
99133
del g_pyBrowsers[browserId]
134+
g_unreferenced_browsers.append(browserId)
100135
else:
101136
# noinspection PyUnresolvedReferences
102137
Debug("RemovePyBrowser() FAILED: browser not found, id = %s" \
@@ -116,7 +151,7 @@ cdef public void PyBrowser_ShowDevTools(CefRefPtr[CefBrowser] cefBrowser
116151
# Called from ClientHandler::OnContextMenuCommand
117152
cdef PyBrowser pyBrowser
118153
try:
119-
pyBrowser = GetPyBrowser(cefBrowser)
154+
pyBrowser = GetPyBrowser(cefBrowser, "ShowDevTools")
120155
pyBrowser.ShowDevTools()
121156
except:
122157
(exc_type, exc_value, exc_trace) = sys.exc_info()

‎src/cefpython.pyx‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -838,7 +838,8 @@ def CreateBrowserSync(windowInfo=None,
838838
requestContextHandler.get().SetBrowser(cefBrowser)
839839

840840
cdef PyBrowser pyBrowser = GetPyBrowser(cefBrowser)
841-
pyBrowser.SetUserData("__outerWindowHandle", int(windowInfo.parentWindowHandle))
841+
pyBrowser.SetUserData("__outerWindowHandle",
842+
int(windowInfo.parentWindowHandle))
842843

843844
return pyBrowser
844845

@@ -889,6 +890,10 @@ def Shutdown():
889890
# Run some message loop work, force closing browsers and then run
890891
# some message loop work again for the browsers to close cleanly.
891892
#
893+
# UPDATE: This code needs to be rechecked. There were enhancements
894+
# to unrferencing globally stored Browser objects in
895+
# g_pyBrowsers. See Issue #330 and its commits.
896+
#
892897
# CASE 1:
893898
# There might be a case when python error occured after creating
894899
# browser, but before any message loop was run. In such case

‎src/frame.pyx‎

Lines changed: 34 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# Project website: https://github.com/cztomczak/cefpython
44

55
include "cefpython.pyx"
6+
include "browser.pyx"
67

78
cdef dict g_pyFrames = {}
89

@@ -17,25 +18,51 @@ cdef PyFrame GetPyFrameById(int browserId, object frameId):
1718

1819
cdef PyFrame GetPyFrame(CefRefPtr[CefFrame] cefFrame):
1920
global g_pyFrames
21+
22+
# This code probably ain't needed, but just to be sure.
2023
if <void*>cefFrame == NULL or not cefFrame.get():
21-
Debug("GetPyFrame(): returning None")
22-
return
24+
raise Exception("GetPyFrame(): CefFrame reference is NULL")
25+
2326
cdef PyFrame pyFrame
2427
cdef object frameId = cefFrame.get().GetIdentifier() # int64
2528
cdef int browserId = cefFrame.get().GetBrowser().get().GetIdentifier()
2629
assert (frameId and browserId), "frameId or browserId empty"
2730
cdef object uniqueFrameId = GetUniqueFrameId(browserId, frameId)
31+
2832
if uniqueFrameId in g_pyFrames:
2933
return g_pyFrames[uniqueFrameId]
34+
35+
# This code probably ain't needed.
36+
# ----
37+
cdef list toRemove = []
3038
for uFid, pyFrame in g_pyFrames.items():
3139
if not pyFrame.cefFrame.get():
32-
Debug("GetPyFrame(): removing an empty CefFrame reference, " \
33-
"uniqueFrameId = %s" % uniqueFrameId)
34-
del g_pyFrames[uFid]
35-
# Debug("GetPyFrame(): creating new PyFrame, frameId=%s" % frameId)
40+
toRemove.append(uFid)
41+
for uFid in toRemove:
42+
Debug("GetPyFrame(): removing an empty CefFrame reference, "
43+
"uniqueFrameId = %s" % uniqueFrameId)
44+
del g_pyFrames[uFid]
45+
# ----
46+
3647
pyFrame = PyFrame(browserId, frameId)
3748
pyFrame.cefFrame = cefFrame
38-
g_pyFrames[uniqueFrameId] = pyFrame
49+
50+
if browserId in g_unreferenced_browsers:
51+
# Browser was already globally unreferenced in OnBeforeClose,
52+
# thus all frames are globally unreferenced too. Create a new
53+
# incomplete instance of PyFrame object. Read comments in
54+
# browser.pyx > GetPyBrowser and in Browser.md for what
55+
# "incomplete" means.
56+
pass
57+
else:
58+
# Keep a global reference to this frame only if the browser
59+
# wasn't destroyed in OnBeforeClose. Otherwise we would leave
60+
# dead frames references living forever.
61+
# SIDE EFFECT: two calls to GetPyFrame for the same frame object
62+
# may return two different PyFrame objects. Compare
63+
# frame objects always using GetIdentifier().
64+
# Debug("GetPyFrame(): create new PyFrame, frameId=%s" % frameId)
65+
g_pyFrames[uniqueFrameId] = pyFrame
3966
return pyFrame
4067

4168
cdef void RemovePyFrame(int browserId, object frameId) except *:

‎src/handlers/display_handler.pyx‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@
33
# Project website: https://github.com/cztomczak/cefpython
44

55
include "../cefpython.pyx"
6+
include "../browser.pyx"
67

78
cdef public void DisplayHandler_OnAddressChange(
89
CefRefPtr[CefBrowser] cefBrowser,
@@ -14,7 +15,7 @@ cdef public void DisplayHandler_OnAddressChange(
1415
cdef py_string pyUrl
1516
cdef object callback
1617
try:
17-
pyBrowser = GetPyBrowser(cefBrowser)
18+
pyBrowser = GetPyBrowser(cefBrowser, "OnAddressChange")
1819
pyFrame = GetPyFrame(cefFrame)
1920
pyUrl = CefToPyString(cefUrl)
2021
callback = pyBrowser.GetClientCallback("OnAddressChange")
@@ -32,7 +33,7 @@ cdef public void DisplayHandler_OnTitleChange(
3233
cdef py_string pyTitle
3334
cdef object callback
3435
try:
35-
pyBrowser = GetPyBrowser(cefBrowser)
36+
pyBrowser = GetPyBrowser(cefBrowser, "OnTitleChange")
3637
pyTitle = CefToPyString(cefTitle)
3738
callback = pyBrowser.GetClientCallback("OnTitleChange")
3839
if callback:
@@ -51,7 +52,7 @@ cdef public cpp_bool DisplayHandler_OnTooltip(
5152
cdef object callback
5253
cdef py_bool returnValue
5354
try:
54-
pyBrowser = GetPyBrowser(cefBrowser)
55+
pyBrowser = GetPyBrowser(cefBrowser, "OnTooltip")
5556
pyText = CefToPyString(cefText)
5657
pyTextOut = [pyText]
5758
callback = pyBrowser.GetClientCallback("OnTooltip")
@@ -73,7 +74,7 @@ cdef public void DisplayHandler_OnStatusMessage(
7374
cdef py_string pyValue
7475
cdef object callback
7576
try:
76-
pyBrowser = GetPyBrowser(cefBrowser)
77+
pyBrowser = GetPyBrowser(cefBrowser, "OnStatusMessage")
7778
pyValue = CefToPyString(cefValue)
7879
callback = pyBrowser.GetClientCallback("OnStatusMessage")
7980
if callback:
@@ -94,7 +95,7 @@ cdef public cpp_bool DisplayHandler_OnConsoleMessage(
9495
cdef py_bool returnValue
9596
cdef object callback
9697
try:
97-
pyBrowser = GetPyBrowser(cefBrowser)
98+
pyBrowser = GetPyBrowser(cefBrowser, "OnConsoleMessage")
9899
pyMessage = CefToPyString(cefMessage)
99100
pySource = CefToPyString(cefSource)
100101
callback = pyBrowser.GetClientCallback("OnConsoleMessage")

0 commit comments

Comments
 (0)