Implement global v8 exception handler

[GoogleCodeExporter]

Currently there is no way to handle javascript errors along with their stack 
trace programmatically, you can only do this through developer tools. You can 
use window.onerror to handle errors, but it runs in a different context and you 
can't get stack trace of the error in that context. Without stack trace the 
error message is mostly useless in a complex application, so practically there 
is no way to handle exceptions programmaticaly in CEF.

There was a CEF thread regarding this issue:
http://magpcss.org/ceforum/viewtopic.php?f=6&t=3408

You can have a global exception handler in v8 engine by calling 
v8::V8::AddMessageHandler().

Original issue reported on code.google.com by czarek.t...@gmail.com on 29 Sep 2012 at 2:17

[GoogleCodeExporter]

@comment#50: MessageLoop::current()->PostTask to post a task on the currently 
executing thread.

Original comment by magreenb...@gmail.com on 23 Oct 2012 at 4:13

[GoogleCodeExporter]

Also, host_id == routing_id(), and uniquely identifies the browser in the 
render process.

Original comment by magreenb...@gmail.com on 23 Oct 2012 at 4:14

[GoogleCodeExporter]

I am so confused with this now. What thread is it running when I call 
MessageLoop::current()->PostTask()? It definitely isn't RT as 
CEF_CURRENTLY_ON_RT returns false. What threads are running in the renderer 
process?

What I did is:
1. In OnDevToolsAgentDetach() call MessageLoop::current()->PostTask(FROM_HERE, 
SomeThread)
2. In SomeThread call 
CEF_POST_TASK_RT(base::Bind(&CefRenderMessageFilter::OnDevToolsAgentDetach_RT, 
this));
3. TADA, works.

Call log:

  CefRenderMessageFilter::OnDevToolsAgentDetach()
  CefRenderMessageFilter::SomeThread()
  ScriptController::setCaptureCallStackForUncaughtExceptions(0)
  content/renderer/DevToolsAgent::OnDetach()
  CefRenderMessageFilter::OnDevToolsAgentDetach_RT()

In comment#52 you mention "host_id==routing_id()" (message->routing_id()?), 
what do I need it for?

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 5:36

[GoogleCodeExporter]

@comment#53: MessageLoop::current() is your current thread, whatever that 
happens to be. If you're in CefRenderMessageFilter::OnMessageReceived (or a 
function called from that method) then this will be the render process IO 
thread. You can retrieve the host_id or routing_id from the IPC::Message it 
match a particular RenderView in the render process.

Original comment by magreenb...@gmail.com on 23 Oct 2012 at 5:43

[GoogleCodeExporter]

Should I validate whether SomeThread() runs on IO thead? (don't see any macro 
for that)

I understand what routing_id() is, but I am confused because you mention it and 
I don't  know what should I do with that new knowledge, it works now, I think 
the problem is solved.

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 6:33

[GoogleCodeExporter]

@comment#55:
> Should I validate whether SomeThread() runs on IO thead?

Probably not necessary.

> I understand what routing_id() is, but I am confused because you mention it

I mentioned routing_id() because you asked "how do we know to which browser 
post this task?". If you don't need it that's fine.

Original comment by magreenb...@gmail.com on 23 Oct 2012 at 9:16

[GoogleCodeExporter]

I have this code:

  void CefRenderMessageFilter::OnDevToolsAgentAttach() {
    CefContentRendererClient* crClient = CefContentRendererClient::Get();
    CEF_POST_TASK_RT(base::Bind(&CefContentRendererClient::DevToolsAgentAttached,
        crClient));
  }

The error I get:

  base/bind_helpers.h(465): error C2039: 'Release' : is not a member of 'CefContentRendererClient'

Do I need to create a separate method to just post this task? Like this:

  void CefRenderMessageFilter::OnDevToolsAgentAttach() {
    CEF_POST_TASK_RT(
        base::Bind(&CefRenderMessageFilter::OnDevToolsAgentAttach_RT, this));
  }

  void CefRenderMessageFilter::OnDevToolsAgentAttach_RT() {
    CEF_REQUIRE_RT();
    CefContentRendererClient::Get()->DevToolsAgentAttached();
  }

Original comment by czarek.t...@gmail.com on 24 Oct 2012 at 1:31

[GoogleCodeExporter]

> Do I need to create a separate method to just post this task?

Yes, that would be fine.

Original comment by magreenb...@gmail.com on 24 Oct 2012 at 1:59

[GoogleCodeExporter]

More tests started failing:

[  FAILED  ] 11 tests, listed below:
[  FAILED  ] V8Test.OnUncaughtExceptionDevTools
[  FAILED  ] SchemeHandlerTest.CustomStandardXHRDifferentOriginWithWhitelist
[  FAILED  ] NavigationTest.FrameNameIdent
[  FAILED  ] URLRequestTest.RendererGET
[  FAILED  ] URLRequestTest.RendererGETNoData
[  FAILED  ] URLRequestTest.RendererGETAllowCookies
[  FAILED  ] URLRequestTest.RendererGETRedirect
[  FAILED  ] URLRequestTest.RendererPOST
[  FAILED  ] URLRequestTest.RendererPOSTWithProgress
[  FAILED  ] URLRequestTest.RendererHEAD
[  FAILED  ] ProcessMessageTest.SendRecvJavaScript

Original comment by czarek.t...@gmail.com on 24 Oct 2012 at 10:06

[GoogleCodeExporter]

Problem with new tests failing is solved, it was my fault, I've added 
OnProcessMessageReceived() to RenderDelegate and it returned TRUE to all 
messages.

--

There is some other matter I would like to discuss. What do you think about 
adding new program switches to ease debugging in Release mode? It would be 
useful if I could enable logging failed DCHECKs, also display them to the 
console, I did this by editing renderer/main_delegete.cc -> 
CefMainDelegate::BasicStartupComplete():

  >logging_dest = logging::LOG_ONLY_TO_FILE;

  replaced with:

  >logging_dest = logging::LOG_TO_BOTH_FILE_AND_SYSTEM_DEBUG_LOG;

and

  >logging::DISABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS

  replaced with:

  >logging::ENABLE_DCHECK_FOR_NON_OFFICIAL_RELEASE_BUILDS

By changing these I've easily found an error in my unittest code, this message 
was displayed:

  [1025/012247:ERROR_REPORT:browser_impl.cc(72)] Check failed: false. GetHost cannot be called from the render process

It would be very useful in CEF Python project, as I provide dll binaries only 
in Release mode, mistakes like above would be easier to catch.

Original comment by czarek.t...@gmail.com on 24 Oct 2012 at 11:30

[GoogleCodeExporter]

@comment#60: Seems reasonable to make those options configurable. Please file 
separate bugs for things unrelated to this issue.

Original comment by magreenb...@gmail.com on 24 Oct 2012 at 11:37

[GoogleCodeExporter]

There is a problem in devtools test, OnLoadEnd() event fires while DevTools 
window is still loading, it is not yet fully loaded, the DevToolsAgent 
attach/detach events have not yet happened, we're closing the browser thinking 
everything is fine, but the test is broken. One way of fixing this I think is 
to add event listener DOMContentLoaded in js and call c++ function by creating 
a  javascript binding.

This bug probably also affects OnUncaughtExceptionDevToolsTest unittest in CEF 
1.

What do you think about creating OnDOMContentLoaded() callback in 
CefLoadHandler? I think that there is a need for such a built-in function, 
jQuery.ready() uses DOMContentLoaded event and that is what most people look 
for.

Original comment by czarek.t...@gmail.com on 25 Oct 2012 at 10:44

[GoogleCodeExporter]

Or maybe I'm wrong and OnLoadEnd already works similar to DOMContentLoaded, but 
the problem is that DevTools needs something like "window.onload"? I need to do 
some more testing.

Original comment by czarek.t...@gmail.com on 25 Oct 2012 at 10:46

[GoogleCodeExporter]

Hm. Since web page loads content via ajax - it never ends. :)
OnLoadEnd it is event about when browser finish parsing HTML document (and 
inline scripts executed). So at this stage you have subscriptions to onload 
event, but they still not fired.

If you interested get a callback after onload events handled by page - 
simpliest way is from OnLoadEnd execute: setTimeout(nativeCallback, 0); << this 
timeout will be fired after onload events handled by page.
Try it first with simple js and logging events.

<body onload="onload()">
<script>
    function onload() { console.log("onload()"); }
    function afteronload() { console.log("afteronload()"); }
    setTimeout(afteronload, 0);
</script>
</body>

Original comment by fdd...@gmail.com on 25 Oct 2012 at 10:57

[GoogleCodeExporter]

Oh, it is unclean trick, in general it can be broken in any time, but i'm not 
believe in this. But using of this technique allow get callback without page 
modification / event subscription.
And of course if DevToolsAgent attached later - it is becomes unusable.

Original comment by fdd...@gmail.com on 25 Oct 2012 at 11:04

[GoogleCodeExporter]

I can confirm that OnLoadEnd() is the same as DOMContentLoaded.

In DevTools popup window.onload event happens at the same time as OnLoadEnd(), 
so this is not a solution. It is as Fddima says, the content is being loaded 
using ajax-like techniques so it's hard to detect when it's finished loading.

The solution is to use InspectorFrontendAPI.loadCompleted() that is defined in 
inspector/resources/DevTools.js and used to run unit tests after the inspector 
finished loading.

The patch for CEF 3 is almost ready, I only have to cleanup my debug code in v8 
unittest. There is one thing that bothers me, I had to use CefPostDelayedTask() 
for 1 second to trigger the exception after the window was destroyed, the 
problem happens with calls to 
ScriptController::setCaptureCallStackForUncaughtExceptions(), they are delayed 
in some way and occur during next unit test, I've tried posting a message to 
browser process and then back to renderer or calling window.close from within 
browser process, but no luck, looks like the timeframe of ScriptController 
calls to setCapture() cannot be predicted.

Original comment by czarek.t...@gmail.com on 25 Oct 2012 at 2:57

[GoogleCodeExporter]

I've tried triggering the exception by posting a task in OnContextReleased(), 
OnBrowserDestroyed(), still no luck, 
ScriptController::setCaptureCallStackForUncaughtExceptions(0) is executed after 
the exception happens.

Marshall, is it acceptable to use CefPostDelayedTask() to trigger exception 
with 1 second delay after the call to window.close() for the devtools popup? Or 
should we keep trying to find better solution to this problem?

The only minor improvement I see at the moment is to post the delayed task from 
within OnBrowserDestroyed() callback.

Original comment by czarek.t...@gmail.com on 25 Oct 2012 at 6:00

[GoogleCodeExporter]

@comment#67: It's hard to give exact guidance without seeing your code. Can you 
post a patch with your changes so far?

Original comment by magreenb...@gmail.com on 25 Oct 2012 at 6:05

[GoogleCodeExporter]

Here goes the patch :-)

Btw. there is one DCHECK failing:

  ERROR_REPORT:main_delegate.cc(423)] Check failed: !locale.empty().

Original comment by czarek.t...@gmail.com on 25 Oct 2012 at 7:05

Attachments:

• OnUncaughtException_cef3_trunk_r877.patch

[GoogleCodeExporter]

Marshall, have you looked at the patch?

Original comment by czarek.t...@gmail.com on 30 Oct 2012 at 10:38

[GoogleCodeExporter]

@comment#69: Thanks for the CEF3 patch. Overall, it looks good. I'll need to 
play with it a bit to fully understand the problem in comment #66. If you post 
a new patch please also update to the newest trunk revision.

A few comments:

1. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
69000&name=OnUncaughtException_cef3_trunk_r877.patch&token=ZOLCwqqIJpJ9Zr-x-axMK
4fIhFc%3A1351701059004#855

- sprintf(buff, "%d", g_current_test_mode);
+ sprintf_s(buff, 33, "%d", g_current_test_mode);

The sprintf_s function is only available on Windows, so we need to stick with 
sprintf for cross-platform support.

2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
69000&name=OnUncaughtException_cef3_trunk_r877.patch&token=ZOLCwqqIJpJ9Zr-x-axMK
4fIhFc%3A1351701059004#1008

+          EXPECT_STREQ(kFuncName, name.ToString().c_str());

These lines are indented too many spaces.

3. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
69000&name=OnUncaughtException_cef3_trunk_r877.patch&token=ZOLCwqqIJpJ9Zr-x-axMK
4fIhFc%3A1351701059004#909

+      if (test_mode_ == V8TEST_ON_UNCAUGHT_EXCEPTION ||

These lines are indented too many spaces.

Original comment by magreenb...@gmail.com on 31 Oct 2012 at 4:48

[GoogleCodeExporter]

Committed in CEF3 trunk revision 890, CEF3 1271 branch revision 891 and CEF3 
1180 branch revision 892 with minor changes.

> is it acceptable to use CefPostDelayedTask() to trigger exception with 1 
second 
> delay after the call to window.close() for the devtools popup?

I think this is fine for the test. We know when 
setCaptureCallStackForUncaughtExceptions will be called (before 
CefContentRendererClient::DevToolsAgentDetached). So, if we find a use case for 
DevTools attach/detach notifications in the future, we can add API callbacks.

Original comment by magreenb...@gmail.com on 2 Nov 2012 at 6:19

• Changed state: Fixed

[GoogleCodeExporter]

Great news :-)

I see that you are handling new event DevToolsAgentMsg_Reattach > 
OnDevToolsAgentReattach(), why did you add this? Attach and Detach events are 
called the same number of times, in reattach you call attach and now it breaks 
the devtools counting, we should not handle reattach event, reattach will be 
called only after attach and before detach, we don't need this event.

We should also fix the DevTools test for CEF 1, it uses OnLoadEnd() method to 
determine that devtools window completed loading, but this method is wrong, it 
was explained in comment #62, we should use the same way as in CEF 3 patch, 
which is to create a javascript binding and make a call from within 
InspectorFrontendAPI.loadCompleted(). I will work on a patch for this, but 
first I need to prepare VS environment on my new computer, the old one stayed 
in Poland, I've moved to Canada this week.

Original comment by czarek.t...@gmail.com on 2 Nov 2012 at 9:24

[GoogleCodeExporter]

@comment#73: Looking at the DevToolsAgent code it appeared that reattach should 
be handled the same was attach. My mistake, I'll fix it.

Congrats on the move to Canada :-).

Original comment by magreenb...@gmail.com on 2 Nov 2012 at 9:29

[GoogleCodeExporter]

@comment #74: Fixed in CEF3 trunk revision 894, CEF3 1271 branch revision 893 
and CEF3 1180 branch revision 895.

Original comment by magreenb...@gmail.com on 2 Nov 2012 at 9:40

[GoogleCodeExporter]

Attaching a fix for CEF 1 v8 unittest for CEF 1, it used OnLoadEnd to detect 
the end of document loading, but this is invalid for the DevTools window as it 
makes ajax calls even after document is complete.

There is also some minor issue in CEF 3 unittest, in function 
DevToolsLoadHook(), it revealed in CEF 1 when devtools window loaded very fast, 
CEF 3 uses remote debugging and it loads slowly so this bug is not appearing 
currently. The problem is in the javascript code, we hook up to the 
InspectorFrontEndAPI.loadCompleted, but it might already be executed, the 
solution is to check InspectorFrontendAPI._isLoaded, so the code should be:

    std::string jsCode = "(function(){"
      "  var oldLoadCompleted = InspectorFrontendAPI.loadCompleted;"
      "  if (InspectorFrontendAPI._isLoaded) {"
      "      window.DevToolsLoaded();"
      "  } else {"
      "    InspectorFrontendAPI.loadCompleted = function(){"
      "      oldLoadCompleted.call(InspectorFrontendAPI);"
      "      window.DevToolsLoaded();"
      "    };"
      "  }"
      "})();";

The code above is in the patch that I'm attaching for CEF 1, but it should also 
be applied for CEF 3.

Btw. 3 tests are failing in CEF 1 rev 906:

[  FAILED  ] GeolocationTest.HandlerAllow
[  FAILED  ] GeolocationTest.HandlerAllowAsync
[  FAILED  ] GeolocationTest.GetGeolocation

Original comment by czarek.t...@gmail.com on 15 Nov 2012 at 7:20

Attachments:

• OnUncaughtExceptionDevToolsTest_fix_CEF1_r906.patch

[GoogleCodeExporter]

@comment #76: Thanks. CEF3 fixed in trunk revision 913, 1271 branch revision 
914 and 1180 branch revision 915. CEF1 fixed in trunk revision 916, 1271 branch 
revision 917 and 1180 branch revision 918.

Original comment by magreenb...@gmail.com on 20 Nov 2012 at 8:51

[GoogleCodeExporter]

That was a long issue :-) It was nice working on this with you, my first 
serious c++ programming.

Original comment by czarek.t...@gmail.com on 20 Nov 2012 at 9:36

[GoogleCodeExporter]

@comment#78: Thanks for following through to the end, you did a good job :-). 
Now you can pick a new issue and submit more patches :D :D.

Original comment by magreenb...@gmail.com on 20 Nov 2012 at 9:40

[GoogleCodeExporter]

I definitely will :-)

Original comment by czarek.t...@gmail.com on 20 Nov 2012 at 9:42