Skip to content

Commit

Permalink
V8ContextHandler: fix GetPyFrame: CefFrame reference is NULL in OnCon…
Browse files Browse the repository at this point in the history 
…textCreated
  • Loading branch information
@jakogut
jakogut committed May 22, 2018
1 parent ee4d8a2 commit 016f77d
Showing 1 changed file with 4 additions and 1 deletion.
5 changes: 4 additions & 1 deletion src/handlers/v8context_handler.pyx
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,10 @@ cdef public void V8ContextHandler_OnContextCreated(
Debug("V8ContextHandler_OnContextCreated()")
pyBrowser = GetPyBrowser(cefBrowser, "OnContextCreated")
pyBrowser.SetUserData("__v8ContextCreated", True)
pyFrame = GetPyFrame(cefFrame)
if cefFrame.get():
pyFrame = GetPyFrame(cefFrame)
else:
pyFrame = None
# User defined callback.
clientCallback = pyBrowser.GetClientCallback("OnContextCreated")
if clientCallback:
Expand Down

9 comments on commit 016f77d

@cztomczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks like a bug in upstream CEF that should be reported in its tracker. CEF docs for OnContextCreated do not state that frame is optional and may be NULL. See https://github.com/chromiumembedded/cef/blob/5caccda56ce81fff0c51a60bc2dbb692e3010a61/include/cef_render_process_handler.h#L95

@cztomczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakogut What is the context of this issue?

@jakogut
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cztomczak Here's a traceback of the problem: https://gist.github.com/jakogut/136004b4d851fdf7c3bcdd7902ba535e

This might be a bug in my application, but it's easily reproducible with a particular website.

@cztomczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jakogut Can you provide steps to reproduce? Reported problem on the CEF Forum:
https://magpcss.org/ceforum/viewtopic.php?f=6&t=16007

@jakogut
Copy link
Owner Author

@jakogut jakogut commented on 016f77d May 23, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can see if I can make a standalone example.

The website is https://cnn.com, and the fastest way I've found to trigger this issue is to create a browser instance using CreateBrowserSync, then a few seconds after the browser is created, destroy it, and recreate it in a loop.

It seems the page loads a lot of Javascript, including a ton of advertisements, and each one is a frame. It seems what's likely happening is that the page loads a lot of external content, and creates a lot of frames, and if the browser is destroyed at the right time, the OnContextCreated callback is fired after the frame is already destroyed.

Calling StopLoad before destroying the browser may stop this from happening, if my hypothesis is correct. I'm not sure if CEF supports destroying a browser that's currently loading content.

@cztomczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That is what I suspected, that frame is being destroyed. So I think the right fix would be to return immediately in OnContextCreated when CefFrame is NULL. Do not call user callback in such case.

@cztomczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Created issue for this:
cztomczak#431

@jakogut
Copy link
Owner Author

@jakogut jakogut commented on 016f77d Jun 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cztomczak I just triggered this issue with V8FunctionHandler_Execute as well.

@cztomczak
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All cases should be fixed in client_handler.cpp as suggested in cztomczak#431.

Please sign in to comment.