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]

I already have a working example.

In v8_impl.cc:

    void CefV8MessageHandler(v8::Handle<v8::Message> message, v8::Handle<v8::Value> data)
    {
      CEF_REQUIRE_VALID_CONTEXT(void(0));
      CEF_REQUIRE_UI_THREAD(void(0));

      CefRefPtr<CefV8Context> context = CefV8Context::GetCurrentContext();
      CefRefPtr<CefBrowser> browser = context->GetBrowser();
      CefRefPtr<CefFrame> frame = context->GetFrame();

      // TEST ONLY: do alert() on exception
      std::ostringstream messageStream;
      CefString cefMessage;
      GetCefString(message->Get(), cefMessage);
      messageStream << "window.alert(\"CefV8MessageHandler: message: " << cefMessage.c_str()  << "\")";
      frame->ExecuteJavaScript(CefString(messageStream.str()), CefString(""), 0);

      CefRefPtr<CefClient> client = browser->GetClient();
      if (!client.get())
        return;

      CefRefPtr<CefV8ContextHandler> handler = client->GetV8ContextHandler();
      if (!handler.get())
        return;  

      CefRefPtr<CefV8Exception> exception = new CefV8ExceptionImpl(message);
      handler->OnScriptException(browser, frame, context, exception);
    }

In cef_v8_context_handler.h:

      ///
      // Global V8 exception handler. To get stack trace of the exception call 
      // CefV8StackTrace::GetCurrent().
      ///
      /*--cef()--*/
      virtual void OnScriptException(CefRefPtr<CefBrowser> browser,
                     CefRefPtr<CefFrame> frame,
                     CefRefPtr<CefV8Context> context,
                     CefRefPtr<CefV8Exception> exception) {}

In cef_context.cc > CefContext::Initialize():

      v8::V8::AddMessageListener(&CefV8MessageHandler);

I run test.html:

      <script>
      asd();
      </script>

And I got an alert:

      ---------------------------
      ---------------------------
      CefV8MessageHandler: message: Uncaught ReferenceError: asd is not defined
      ---------------------------
      OK   
      ---------------------------

I wondered whether it is necessary to call 
v8::V8::RemoveMessageListener(&CefV8MessageHandler) in 
CefContext::UIT_FinishShutdown, but I'm not sure, I don't think we need to do 
call it. What are your thoughts?

Let me know Marshall if this code looks fine and I'll create a patch. I will 
also create a unittest in v8_unittest.cc.

Original comment by czarek.t...@gmail.com on 29 Sep 2012 at 2:27

[GoogleCodeExporter]

There seems to be the same problem with empty stack trace as mentioned on CEF 
forum, when CefV8MessageHandler() is called the context seems to be unwound. 
The solution is to set capturing uncaugh exception in CefContext::Initialize():

v8::V8::SetCaptureStackTraceForUncaughtExceptions(true, 10, 
v8::StackTrace::kDetailed);

I test it and it works fine. Marshall, in CEF thread you mentioned that it 
might cause integration problems with Inspector, I'm not sure what meant by 
that, I run developer tools and look at the console and everything seems fine, 
exception is also thrown in the devtools console. But I suspect that there are 
some good reasons why this is disabled by default.

How about a new option in CefSettings called "enable_global_exception_handler", 
SetCaptureStackTrace & OnScriptException are called only when this option is 
enabled? For those that need to use Inspector will have it turned off by 
default, for those that want to handle errors programmatically will have such 
an option. We would also need another setting called "stack_trace_frame_limit" 
as it needs to be passed to SetCaptureStackTrace().

Probably the only other way to get stack trace of an error is to use 
try..catch, but I have no idea how to do that for each frame so that it works 
similar to a global handler.

Marshall, let me know what you think, how should we name these CefSettings 
options for enabling global exception handler?

Original comment by czarek.t...@gmail.com on 29 Sep 2012 at 6:17

[GoogleCodeExporter]

I thought about it and I think we can do fine with only 1 new option in 
CefSettings:

    int capture_stack_trace_for_script_uncaught_exceptions

    Whether to capture stack trace for script uncaught exceptions. This option is for use with CefV8ContextHandler::OnScriptException(). Value of 0 (default) means there will bo no stack trace available. Value of 10 means to capture 10 frames of stack trace. This option is off by default as it might cause integration problems with Inspector.

Original comment by czarek.t...@gmail.com on 30 Sep 2012 at 7:01

[GoogleCodeExporter]

There is something very strange happening when I try to add new option to 
CefSettings, what I do is:

in cef_types.h > cef_settings_t at the end adding:

    int capture_stack_trace_for_script_uncaught_exceptions;

in cef_types_wrappers.h > CefSettingsTraits > set() at the end adding:

    target->capture_stack_trace_for_script_uncaught_exceptions = src->capture_stack_trace_for_script_uncaught_exceptions;

After I add these 2 lines of code an rebuild solution, I run cefclient.exe and 
get a strange exception:

    Unhandled exception at 0x5d16b5da (libcef.dll) in cefclient.exe: 0xC0000005: Access violation reading location 0x00000094.

The exception is in scheme_impl.cc on this line:

    net::URLRequestJobFactory* job_factory =
        const_cast<net::URLRequestJobFactory*>(
            _Context->request_context()->job_factory());

Stack trace is:

    libcef.dll!`anonymous namespace'::CefUrlRequestManager::AddFactory(const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & scheme, const std::basic_string<char,std::char_traits<char>,std::allocator<char> > & domain, CefRefPtr<CefSchemeHandlerFactory> factory)  Line 619 + 0xc bytes  C++
    libcef.dll!CefRegisterSchemeHandlerFactory(const CefStringBase<CefStringTraitsUTF16> & scheme_name, const CefStringBase<CefStringTraitsUTF16> & domain_name, CefRefPtr<CefSchemeHandlerFactory> factory)  Line 834 + 0x4b bytes C++
    libcef.dll!base::internal::Invoker<3,base::internal::BindState<base::internal::RunnableAdapter<bool (__cdecl*)(CefStringBase<CefStringTraitsUTF16> const &,CefStringBase<CefStringTraitsUTF16> const &,CefRefPtr<CefSchemeHandlerFactory>)>,void __cdecl(CefStringBase<CefStringTraitsUTF16> const &,CefStringBase<CefStringTraitsUTF16> const &,CefRefPtr<CefSchemeHandlerFactory>),void __cdecl(CefStringBase<CefStringTraitsUTF16>,CefStringBase<CefStringTraitsUTF16>,CefRefPtr<CefSchemeHandlerFactory>)>,void __cdecl(CefStringBase<CefStringTraitsUTF16> const &,CefStringBase<CefStringTraitsUTF16> const &,CefRefPtr<CefSchemeHandlerFactory>)>::Run(base::internal::BindStateBase * base)  Line 1386 + 0x2b bytes C++
    libcef.dll!MessageLoop::RunTask(const base::PendingTask & pending_task)  Line 462   C++
    libcef.dll!MessageLoop::DoWork()  Line 649  C++
    libcef.dll!base::MessagePumpForIO::DoRunLoop()  Line 512 + 0xc bytes    C++
    libcef.dll!base::MessagePumpWin::Run(base::MessagePump::Delegate * delegate)  Line 47 + 0x3e bytes  C++
    libcef.dll!MessageLoop::RunInternal()  Line 419 + 0xa bytes C++
    libcef.dll!base::RunLoop::Run()  Line 46    C++
    libcef.dll!MessageLoop::Run()  Line 300 C++
    libcef.dll!base::Thread::Run(MessageLoop * message_loop)  Line 134  C++
    libcef.dll!base::Thread::ThreadMain()  Line 173 C++
    libcef.dll!base::`anonymous namespace'::ThreadFunc(void * params)  Line 60  C++
    kernel32.dll!756e3677()     
    [Frames below may be incorrect and/or missing, no symbols loaded for kernel32.dll]  
    ntdll.dll!77da9f42()    
    ntdll.dll!77da9f15()    

When I remove these 2 lines of code in cef_types.h and cef_type_wrappers.h the 
exception is not happening.

Attaching debugger screenshot, _Context pointer is in red color.

Original comment by czarek.t...@gmail.com on 30 Sep 2012 at 10:36

Attachments:

• cef-exception-issue-736.png

[GoogleCodeExporter]

Problem solved, there seems to be a bug in Visual Studio, when you make changes 
to cef_types.h > cef_settings_t, then some parts of code that use it are 
recompiled, but some are not, in the debugger I checked _Context->settings_ and 
"capture_stack_trace_for_script_uncaught_exceptions" was missing in there. To 
fix it I had to clean libcef_static project and build it again.

Original comment by czarek.t...@gmail.com on 30 Sep 2012 at 12:57

[GoogleCodeExporter]

I was creating a unittest, I tried overriding OnScriptException, but there was 
always a method from the header file being called, not the overriden one. The 
solution was to run "tools/translator.bat". The 
include/capi/cef_v8context_handler_capi.h had to be updated by that tool, 
otherwise you couldn't OVERRIDE OnScriptException.

Shouldn't this translator tool be run automatically when building solution in 
Visual Studio? Or at least mentioned somwehere on the wiki, I lost almost whole 
day trying this to make work.

Original comment by czarek.t...@gmail.com on 1 Oct 2012 at 8:55

[GoogleCodeExporter]

Patch is ready, attaching "OnScriptException_cef1_trunk.patch".

Changes:
- added OnScriptException(browser, frame, context, exception, stackTrace) in 
include/cef_v8context_handler.h
- added "int capture_stack_trace_for_script_uncaught_exceptions" to 
cef_settings_t in internal/cef_types.h
- a call to v8::AddMessageListener() and 
v8::SetCaptureStackTraceForUncaughtExceptions() depending on 
settings.capture_stack_trace_for_script_uncaught_exceptions in 
libcef/cef_context.cc > CefContext::Initialize().
- added CefV8MessageHandler() to libcef/v8_impl.cc
- added CefV8EmptyStackTraceImpl() to libcef/v8_impl.h
- a unittest in v8_unittest.cc: V8TEST_ON_SCRIPT_EXCEPTION, added command line 
switch "-capture-stack-trace-for-script-uncaugh-exceptions=10"

There is a unit test that tests 2 cases, one when stack trace is empty 
(capturing exceptions is disabled), the other case the stack should containt 2 
frames, so you should run cef_unittests.exe twice:
- cef_unittests.exe (for empty stack trace)
- cef_unittests.exe -capture-stack-trace-for-script-uncaught-exceptions=10

Original comment by czarek.t...@gmail.com on 1 Oct 2012 at 2:11

Attachments:

• OnScriptException_cef1_trunk_r823.patch

[GoogleCodeExporter]

Thanks for working on this. Some comments about your patch:

1. V8::SetCaptureStackTraceForUncaughtExceptions is also called by 
InspectorConsoleAgent 
(WebKit/Source/WebCore/inspector/InspectorConsoleAgent.cpp) via 
ScriptController::setCaptureCallStackForUncaughtExceptions. We need to 
understand the circumstances under which InspectorConsoleAgent is used and how 
this will affect our own usage of V8::SetCaptureStackTraceForUncaughtExceptions.

2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
07000&name=OnScriptException_cef1_trunk_r823.patch&token=PLi2u-gYycvsuSnmKivsr1G
VoUY%3A1349100781974#196

+ int capture_stack_trace_for_script_uncaught_exceptions;

This name sounds more like a boolean than a size, and it's pretty long. Perhaps 
call it 'uncaught_exception_stack_size' instead?

3. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
07000&name=OnScriptException_cef1_trunk_r823.patch&token=PLi2u-gYycvsuSnmKivsr1G
VoUY%3A1349100781974#159

+ virtual void OnScriptException(CefRefPtr<CefBrowser> browser,

We should probably call this method OnUncaughtException to match the name in #2.

4. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
07000&name=OnScriptException_cef1_trunk_r823.patch&token=PLi2u-gYycvsuSnmKivsr1G
VoUY%3A1349100781974#292

+ CefRefPtr<CefV8Context> context = CefV8Context::GetCurrentContext();
+ CefRefPtr<CefBrowser> browser = context->GetBrowser();
+ CefRefPtr<CefFrame> frame = context->GetFrame();

Improve the new V8 test to verify that this is the correct context (the context 
that originally caused the exception).

5. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
07000&name=OnScriptException_cef1_trunk_r823.patch&token=PLi2u-gYycvsuSnmKivsr1G
VoUY%3A1349100781974#427

+ settings.capture_stack_trace_for_script_uncaught_exceptions =
+ atoi(commandline_->GetSwitchValueASCII(
+ cefclient::kCaptureStackTraceForScriptUncaughtExceptions).c_str());

Add command line argument support in the cefclient.cpp AppGetSettings function 
as well.

And some style-related problems. Most of these can be caught by running the 
tools/check_style.py tool.

4. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
07000&name=OnScriptException_cef1_trunk_r823.patch&token=PLi2u-gYycvsuSnmKivsr1G
VoUY%3A1349100781974#287

+void CefV8MessageHandler(v8::Handle<v8::Message> message, 
v8::Handle<v8::Value> data)
+{

Opening bracket should be on the same line as the function definition.

5. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
07000&name=OnScriptException_cef1_trunk_r823.patch&token=PLi2u-gYycvsuSnmKivsr1G
VoUY%3A1349100781974#373

+ EXPECT_STREQ("Uncaught ReferenceError: asd is not defined", 
onScriptException_message_.c_str());

Wrap lines to 80 characters.

Original comment by magreenb...@gmail.com on 1 Oct 2012 at 2:58

• Changed state: Accepted
• Added labels: Type-Enhancement
• Removed labels: Type-Defect

[GoogleCodeExporter]

Point 1.

I've traced calls to 
ScriptController::setCaptureCallStackForUncaughtExceptions(), when you open 
devtools window it sets capturing to True, when closing devtools window 
capturing is disabled. It counts how many devtool windows are opened, so it 
disables capturing only when all of them are closed.

When capturing stack trace for uncaught exception the only thing it changes is 
that you can access that stack through v8::Message->GetStackTrace(). It is 
disabled by default, probably for performance reasons, or others, you just 
don't need the stack trace unless you have devtools opened, so it's off when 
there is no need to debug.

Our usage of v8::SetCapture does not affect Inspector in any way (almost). 
Inspector by default sets the stack size to 200, so our only interference is 
that we may lower that limit. We use StackOptions.kDetailed, it means the most 
details about the stack trace, so there is not going to be a problem with some 
info missing. In our app we only set capturing ON, so it will not break 
Inspector in any way, there is only some more info available, that's all. There 
would be a problem only if we set capturing off, then stack would be not 
available in Inspector, but we only set it on, so there is no harm.

The purpose of OnUncaughtException() is to catch exceptions programmatically, 
the case is that we do not want to run Inspector to see errors and stack trace, 
we want to be able to do it through some other automated way. The only 
integration problem that might occur is that even when 
settings.uncaught_exceptions_stack_size is set then the stack trace might be 
not available when devtools window was opened and then close. Our calls to 
v8::setCapture do not interfere in any way with Inspector, it is only the other 
way, when all Inspectors are being closed, it disables capturing.

I think that we should only mention in documentation comments for the 
OnUncaughtException callback, that when you open DevTools and then closie it, 
then stack trace will be unavailable further when this function is called.

Original comment by czarek.t...@gmail.com on 1 Oct 2012 at 6:12

[GoogleCodeExporter]

@comment#9: Thanks for the analysis. I don't think documenting the problem is 
sufficient. We should instead identify when all of the inspector windows have 
been closed and then re-enable the stack trace functionality. This will also 
allow us to restore the stack trace size to the value specified by the user.

We can use CefContext to track how many inspector windows are open. Increment 
the count in UIT_CreateDevToolsClient and decrement it in 
UIT_DestroyDevToolsClient.

Original comment by magreenb...@gmail.com on 1 Oct 2012 at 6:32

[GoogleCodeExporter]

The order of calls is following:

    ScriptController::setCaptureCallStackForUncaughtExceptions(1)
    CefBrowserImpl::UIT_CreateDevToolsClient()
    ScriptController::setCaptureCallStackForUncaughtExceptions(0)
    CefBrowserImpl::UIT_DestroyDevToolsClient()

So it will work fine.

Should we also create a test case for opening devtools > closing and test 
whether stack trace is captured?

Original comment by czarek.t...@gmail.com on 1 Oct 2012 at 6:56

[GoogleCodeExporter]

@comment#11: Yes, adding a test for this would be good.

Original comment by magreenb...@gmail.com on 1 Oct 2012 at 7:01

[GoogleCodeExporter]

I've made a little addition to CefTestSuite, it checks whether command line 
switches are valid, take this scenario for example:

    cef_unittests.exe -uncaught-exception-stack-sizee=10

There is a typo "-sizee", cef_unittest.exe runs fine, but our test case for 
non-empty stack trace will not be tested. I've added a new function to 
CefTestSuite called "CefTestSuite::CheckCommandLineSwitches()", I iterate 
through all command line switches using CommandLine::GetSwitches(), then pass 
these switches to a python script "tools/check_cefclient_switches.py", it is 
called using ShellExecuteEx, python script parses switches in 
"tests/cefclient/cefclient_switches.cpp" using Regexp and exits using code: 0 
(ok) or -1(invalid switch).

So when I run:

    cef_unittests.exe -uncaught-exception-stack-sizee=10

Application exists and I see this:

    ERROR: invalid command line switch (exit code=-1)

It works only on Windows unfortunately:

   #if defined(OS_WIN)
       CefTestSuite::CheckCommandLineSwitches();
   #endif

Have a look at patch, attaching 
"CheckCommandLineSwitches_cef1_trunk_r828.patch". 2 files modified 
test_suite.cc and test_suite.h, 1 new file tools/check_cefclient_switches.py.

Original comment by czarek.t...@gmail.com on 2 Oct 2012 at 11:14

Attachments:

• CheckCommandLineSwitches_cef1_trunk_r828.patch

[GoogleCodeExporter]

After a second thought... Don't look at the patch above, I will rewrite it in 
C++, calling python to do regular expression is a bit stupid. But how do I 
include that re2? When I do this in test_suite.cc:

    #include "third_party/re2/re2/re2.h"

I get error:

    fatal error C1083: Cannot open include file: 're2/stringpiece.h': No such file or directory

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

[GoogleCodeExporter]

@comment#13: Adding a test to verify command line arguments seems out of scope 
for this issue. I think it would be sufficient to default the 
uncaught_exception_stack_size value to something reasonable in test_suite.cc if 
it's not specified on the command line.

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

[GoogleCodeExporter]

I'll create 2 tests OnUncaughtExceptionNoStack, OnUncaughtExceptionWithStack 
for which I set the stack size manually, unless there is a command line switch, 
then OnUncaughtExceptionWithStack will use that command line setting.

Original comment by czarek.t...@gmail.com on 2 Oct 2012 at 3:43

[GoogleCodeExporter]

Here is the new patch with all Marshall remarks taken into account. Attaching 
"OnScriptException2_cef1_trunk_r828.patch".

There are 3 tests: no stack, with stack, with stack and devtools opened then 
closed.

Re-enabling stack trace functionality after DevTools window has been closed can 
be tested in V8 unittests only with command line switch 
"-uncaught-exception-stack-size=10". This is because re-enabling stack trace is 
implemented in CefContext class, and we have no way to overwrite 
CefContext::settings_ member from within unit tests.

Marshall, how do you force Visual Studio 2010 to strip automatically any 
redundant whitespace when saving file? After changes I make in the code there 
are almost always some spaces left.

Original comment by czarek.t...@gmail.com on 2 Oct 2012 at 7:24

Attachments:

• OnScriptException2_cef1_trunk_r828.patch

[GoogleCodeExporter]

Attaching new patch, a minor code cleanup in v8_unittest.cc, also updated to 
revision 834.

Original comment by czarek.t...@gmail.com on 3 Oct 2012 at 6:58

Attachments:

• OnScriptException3_cef1_trunk_r834.patch

[GoogleCodeExporter]

Some comments about your most recent patch:

http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
18000&name=OnScriptException3_cef1_trunk_r834.patch&token=amnKXD43ahOgTb_qx9h-aT
jKpWA%3A1349295082718#254

+void CefV8EnableGlobalExceptionHandler() {
+  v8::V8::AddMessageListener(&CefV8MessageHandler);
+}
+
+void CefV8SetUncaughtExceptionStackSize(int frameLimit) {
+  if (frameLimit > 0) {
+    v8::V8::SetCaptureStackTraceForUncaughtExceptions(true, frameLimit,
+                                                    v8::StackTrace::kDetailed);
+  } else {
+    v8::V8::SetCaptureStackTraceForUncaughtExceptions(false, 0,
+                                                    v8::StackTrace::kOverview);
+  }
+}

I don't think we need to expose a separate CefV8EnableGlobalExceptionHandler 
function. Instead, add the message listener when |frameLimit| > 0 and remove it 
when |frameLimit| == 0.

For global functions we need to (a) verify that the CEF context is in a valid 
state and (b) post the request to the correct thread (in this case the UI 
thread) for execution if necessary. See other global functions for an example.

Do we need both the global function and the CefSettings value?

Original comment by magreenb...@gmail.com on 3 Oct 2012 at 8:45

[GoogleCodeExporter]

CefV8EnableGlobalExceptionHandler() is called only in CefContext::Initialize(), 
so no, there is no need for that global function. This function is not exposed 
in api.

Only CefV8SetUncaughtExceptionStackSize() is exposed via api (in 
include/cef_v8.h), as it is needed for the unit tests, so we can test 2 cases 
(no stack, with stack) without having to run another cef_unittest.exe with a 
command line switch. I exposed it as you mentioned in a comment that we could 
do such. But it didn't solve all the problems, as testing the DevTools case 
still needs to be run via command line switch.

Remove both of them? Allow to unit test the stack only with command line 
switches?

Original comment by czarek.t...@gmail.com on 4 Oct 2012 at 7:41

[GoogleCodeExporter]

There is also another way, testing a case when there is no stack isn't really 
needed, so we could manually set settings.uncaught_exception_stack_size to 10 
in run_all_unittests.cc > main(). If we do so, should we make settings a global 
variable and create GetSettings() in run_all_unittests.cc function that we call 
in unit test to check whether stack size is set? Or maybe there should be such 
an api? CefGetAppSettings() that returns CefContext::settings_?

Original comment by czarek.t...@gmail.com on 4 Oct 2012 at 8:03

[GoogleCodeExporter]

> CefV8EnableGlobalExceptionHandler() is called only in 
CefContext::Initialize(), so
> no, there is no need for that global function. This function is not exposed 
in api.

Good that this is not exposed via the API. CefV8EnableGlobalExceptionHandler 
should only called in CefContext when uncaught_exception_stack_size > 0. 
Otherwise, there might be unexpected calls to OnUncaughtException (for example, 
the callback will be executed when DevTools is visible even though the user 
specified uncaught_exception_stack_size == 0).

> There is also another way, testing a case when there is no stack isn't really 
needed,
> so we could manually set settings.uncaught_exception_stack_size to 10 in
> run_all_unittests.cc > main().

Agreed. We don't need to test the case where uncaught_exception_stack_size == 0.

> If we do so, should we make settings a global variable and create 
GetSettings() in
> run_all_unittests.cc function that we call in unit test to check whether 
stack 
> size is set? Or maybe there should be such an api? CefGetAppSettings() that 
> returns CefContext::settings_?

I think we should hard-code the uncaught_exception_stack_size value for unit 
tests. If it's not configurable via the command line for unit tests then it 
removes the above complexity. Also, we no longer need to expose 
CefV8SetUncaughtExceptionStackSize() via the API.

Original comment by magreenb...@gmail.com on 4 Oct 2012 at 7:54

[GoogleCodeExporter]

Are you proposing that OnCaughtException callback should only be called when 
stack size is set > 0? Currently this option is only about stack trace, it does 
not affect whether OnUncaughtException is called. So this option should also be 
a on/off for global exception handler? That is fine I think, as there is no 
need for an exception handler without providing the stack trace. This would 
make things simple in unit tests.

Just bo be clear. By default this option will be disabled, stack_size = 0. In 
unit tests it will be enabled.

Original comment by czarek.t...@gmail.com on 4 Oct 2012 at 11:02

[GoogleCodeExporter]

I've updated to revision 844, updated chromium to 160122. Stack trace test 
passes, but when testing stack trace while closing dev tools I get some errors.

First, there seems to be some resource missing:

    [1006/204917:ERROR:cef_context.cc(549)] No data resource available for id 12053
    [1006/204917:ERROR:cef_context.cc(549)] No data resource available for id 12049

Context seems to be invalid:

    tests\unittests\v8_unittest.cc(1564): error: Value of: onUncaughtException_context_->IsSame(context)

Because the error happens in Developer Tools window:

    tests\unittests\v8_unittest.cc(1566): error: Value of: exception->GetMessageW().ToString().c_str()

  Actual: "Uncaught TypeError: Cannot read property 'styleId' of undefined"
Expected: "Uncaught ReferenceError: asd is not defined"

Full log:

    [ RUN      ] V8Test.OnUncaughtExceptionDevTools
    [1006/204917:ERROR:cef_context.cc(549)] No data resource available for id 12053
    [1006/204917:ERROR:cef_context.cc(549)] No data resource available for id 12049
    tests\unittests\v8_unittest.cc(1564): error: Value of: onUncaughtException_context_->IsSame(context)
      Actual: false
    Expected: true
    tests\unittests\v8_unittest.cc(1566): error: Value of: exception->GetMessageW().ToString().c_str()
      Actual: "Uncaught TypeError: Cannot read property 'styleId' of undefined"
    Expected: "Uncaught ReferenceError: asd is not defined"
    tests\unittests\v8_unittest.cc(1576): error: Value of: stackFormatted.str().c_str()
      Actual: "at WebInspector.CSSStyleDeclaration() in  on line 14104\nat WebInspector.CSSStyleDeclaration.parsePayload() i
    n  on line 14144\nat WebInspector.CSSRule() in  on line 14275\nat WebInspector.CSSRule.parsePayload() in  on line 14285\
        nat WebInspector.CSSStyleModel.parseRuleArrayPayload() in  on line 13775\nat callback() in  on line 13801\nat InspectorB
        ackendClass.dispatch() in  on line 4867\n"
    Expected: stackFormattedShouldBe
            Which is: "at test() in http://tests/V8Test.OnUncaughtException on line 2\nat () in http://tests/V8Test.OnUncaughtExcept
        ion on line 3\n"
    [  FAILED  ] V8Test.OnUncaughtExceptionDevTools (443 ms)

Attaching new patch "OnUncaughtException4_cef1_trunk_r844.patch".

Original comment by czarek.t...@gmail.com on 6 Oct 2012 at 7:07

Attachments:

• OnUncaughtException4_cef1_trunk_r844.patch

[GoogleCodeExporter]

When running cefclient.exe I do not get any errors in DevTools window, strange.

Original comment by czarek.t...@gmail.com on 6 Oct 2012 at 7:10

[GoogleCodeExporter]

Hmm I've rebuilt cefclient and cef_unittests and it works fine now, the error 
does not appear anymore, all tests PASS OK, seems like my fault when rebuilding.

Marshall, have a look at patch in comment #24, it addresses issues you 
mentioned in comment #22.

Original comment by czarek.t...@gmail.com on 6 Oct 2012 at 7:16

[GoogleCodeExporter]

Overall looks good. Some comments on your new patch:

1. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
24000&name=OnUncaughtException4_cef1_trunk_r844.patch&token=wftVH9PXsouPthMgkilR
pH-HA2A%3A1349713830883#210

+ if (v8Stack.IsEmpty())
+ stackTrace = new CefV8EmptyStackTraceImpl();

The v8Stack should never be empty now. Let's DCHECK this and remove the 
CefV8EmptyStackTraceImpl class.

Original comment by magreenb...@gmail.com on 8 Oct 2012 at 5:02

[GoogleCodeExporter]

Done, attaching new patch.

Original comment by czarek.t...@gmail.com on 8 Oct 2012 at 5:39

Attachments:

• OnUncaughtException5_cef1_trunk_r844.patch

[GoogleCodeExporter]

Additional comments:

1. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
28000&name=OnUncaughtException5_cef1_trunk_r844.patch&token=Zqbfru1vqGRhyJJeDOwl
7gkrsy8%3A1349718032803#134

+ if (settings.uncaught_exception_stack_size > 0) {
+ v8::V8::AddMessageListener(&CefV8MessageHandler);
+ v8::V8::SetCaptureStackTraceForUncaughtExceptions(true,
+ settings.uncaught_exception_stack_size,
+ v8::StackTrace::kDetailed);
+ }

This code should be called from CefProcessUIThread::Init() instead of 
CefContext::Initialize(). Otherwise, it won't be executed on the correct thread 
when multi_threaded_message_loop is true.

2. 
http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
28000&name=OnUncaughtException5_cef1_trunk_r844.patch&token=Zqbfru1vqGRhyJJeDOwl
7gkrsy8%3A1349718032803#481

+          "window.setTimeout(function(){ test(); }, 2000);\n"

http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=73600
28000&name=OnUncaughtException5_cef1_trunk_r844.patch&token=Zqbfru1vqGRhyJJeDOwl
7gkrsy8%3A1349718032803#513

+ CefPostDelayedTask(TID_UI, NewCefRunnableMethod(GetBrowser().get(),
+ &CefBrowser::CloseDevTools), 1000);

We can't safely assume that DevTools will load within a particular period of 
time. Instead, we should return a stub CefClient implementation from 
OnBeforePopup that signals the test to continue when OnLoadEnd or similar is 
called.

Original comment by magreenb...@gmail.com on 8 Oct 2012 at 7:25

[GoogleCodeExporter]

Attaching new patch with your comments addressed.

- moved v8 exception initialization code to CefProcessUIThread::Init().
- DevTools window is closed from within OnLoadEnd callback.
- Removed window.setTimeout, using frame->ExecuteJavaScript() from OnLoadEnd 
callback to call javascript's test().
- Renamed functions in v8 unittest so they are consistent: 
RunOnUncaughtExceptionTest(), RunOnUncaughtExceptionDevToolsTest().

Btw. is it intentional for the test to PASS OK when DestroyTest() is not 
called? This is happening when you close window manually while running the test.

Original comment by czarek.t...@gmail.com on 10 Oct 2012 at 8:37

Attachments:

• OnUncaughtException6_cef1_trunk_r851.patch

[GoogleCodeExporter]

Btw. this might be a bug, when I call frame->ExecuteJavascript() with second 
param "const CefString& scriptUrl" being set to some value, then in stack trace 
the scriptUrl is empty.

Original comment by czarek.t...@gmail.com on 10 Oct 2012 at 8:52

[GoogleCodeExporter]

Thanks for following through with this. Committed as CEF1 trunk revision 857, 
CEF1 1271 branch revision 858 and CEF1 1180 branch revision 859 with the 
following changes:

1. Add additional test code to verify that OnUncaughtException and DestroyTest 
are called.

2. Various minor style and documentation changes.

Would you like to develop a patch for CEF3? :-)

@comment#31: Could be a bug in WebKit.

Original comment by magreenb...@gmail.com on 12 Oct 2012 at 4:54

• Added labels: CEF1, CEF3

[GoogleCodeExporter]

Okay, I'll work on CEF 3 patch. As it was written in the thread, the 
OnUncaughtException() callback will go to CefRenderProcessHandler in CEF 3.

Original comment by czarek.t...@gmail.com on 12 Oct 2012 at 5:37

[GoogleCodeExporter]

How do I run unittests in CEF 3? When I run cef_unittests.exe I get errors:

    [1019/192553:ERROR:renderer_main.cc(215)] Running without renderer sandbox
    [1019/192556:ERROR:ipc_channel_win.cc(392)] pipe error: 232

I get a lot of these messages, also 3 tests fail:

    [  FAILED  ] SchemeHandlerTest.CustomStandardXHRDifferentOriginWithWhitelist
    [  FAILED  ] NavigationTest.FrameNameIdent
    [  FAILED  ] ProcessMessageTest.SendRecvJavaScript

Original comment by czarek.t...@gmail.com on 19 Oct 2012 at 5:32

[GoogleCodeExporter]

> [1019/192553:ERROR:renderer_main.cc(215)] Running without renderer sandbox

This is safe to ignore.

> [1019/192556:ERROR:ipc_channel_win.cc(392)] pipe error: 232

This is safe to ignore unless it's specifically causing a test failure.

> [  FAILED  ] SchemeHandlerTest.CustomStandardXHRDifferentOriginWithWhitelist
> [  FAILED  ] ProcessMessageTest.SendRecvJavaScript

These tests are known to be flaky.

> [  FAILED  ] NavigationTest.FrameNameIdent

What failure are you getting with this test?

Original comment by magreenb...@gmail.com on 19 Oct 2012 at 5:39

[GoogleCodeExporter]

NavigationTest.FrameNameIdent failure:

    [ RUN      ] NavigationTest.FrameNameIdent
    [1019/200230:ERROR:renderer_main.cc(215)] Running without renderer sandbox
    tests\unittests\navigation_unittest.cc(301): error: Value of: names[1].ToString().c_str()
      Actual: "nav2"
      Expected: frame2_name_.c_str()
    Which is: ""
    tests\unittests\navigation_unittest.cc(302): error: Value of: names[2].ToString().c_str()
      Actual: "<!--framePath //nav2/<!--frame0-->-->"
      Expected: frame3_name_.c_str()
    Which is: ""
    tests\unittests\navigation_unittest.cc(351): error: Value of: handler->got_frame2_name_
      Actual: false
    Expected: true
    [  FAILED  ] NavigationTest.FrameNameIdent (100 ms)

ProcessMessageTest.SendRecvJavaScript and NavigationTest.FrameNameIdent seems 
to be failing randomly, sometimes they pass OK, sometimes they FAIL. When 
ProcessMessageTest.SendRecvJavaScript fails I get an empty window with "test 
javascript" message, I need to close it so that tests continue.

OS: Win7 64 bit

Original comment by czarek.t...@gmail.com on 19 Oct 2012 at 6:08

[GoogleCodeExporter]

@comment#36: OK, then NavigationTest.FrameNameIdent is a flaky test as well. 
Ignore the flaky tests for now, that's a separate issue :-).

Original comment by magreenb...@gmail.com on 19 Oct 2012 at 6:33

[GoogleCodeExporter]

I am having troubles keeping track of DevTools windows, it is required to 
re-enable capturing of stack trace when last of devtools windows is closed. In 
CEF 1 there were functions ShowDevToolsClient()/CloseDevToolsClient() so it was 
easy. In CEF 3 I am trying to use notifications 
NOTIFICATION_DEVTOOLS_AGENT_ATTACHED/NOTIFICATION_DEVTOOLS_AGENT_DETACHED, but 
it's problematic. First issue is that NOTIFICATION_DEVTOOLS_AGENT_ATTACHED gets 
called multiple times for the same DevTools window, first call is in the 
context of the main browser which is fine, but in a moment it's called again 
withing the context of the DevTools window itself, which makes no sense. 
NOTIFICATION_DEVTOOLS_AGENT_DETACHED is called only once. This makes it hard to 
count devtools windows. Another problem is that 
NOTIFICATION_DEVTOOLS_AGENT_DETACHED is called before 
ScriptController::setCaptureCallStackForUncaughtExceptions(), so can't 
overwrite capturing, I tried to do it from within DestroyBrowser() but again no 
luck.

I am not sure how this should be done. Should we implement 
ShowDevToolsClient/CloseDevToolsClient as it is in CEF 1?

Original comment by czarek.t...@gmail.com on 21 Oct 2012 at 7:47

[GoogleCodeExporter]

In CefContext, on which thread should I call 
v8::V8::SetCaptureStackTraceForUncaughtExceptions(), TID_UI or TID_RENDERER?

Original comment by czarek.t...@gmail.com on 22 Oct 2012 at 7:43

[GoogleCodeExporter]

@comment#38-39: The V8 function needs to be executed in the render process, 
probably in CefRenderProcessObserver::WebKitInitialized. The 
uncaught-exception-stack-size value should be passed from the browser process 
to the render process via a command-line flag and 
CefContentBrowserClient::AppendExtraCommandLineSwitches (see for example how we 
handle kLocale).

The logic for identifying when a DevTools window is created and destroyed, and 
resetting the stack trace size accordingly, can also live in the render 
process. That might be easier/clearer than trying to use 
NOTIFICATION_DEVTOOLS_* in the browser process.

Original comment by magreenb...@gmail.com on 22 Oct 2012 at 3:35

[GoogleCodeExporter]

How do we track DevTools window creation? By checking frame's url in 
CefContentRendererClient::DidCreateScriptContext()? The url should be in 
format: 
"http://localhost:12345/devtools/devtools.html?ws=localhost:12345/devtools/page/
4239dba8747f71f4e721829ceb0e2bfa"? (or chrome-devtools). If so, should we 
create a global function for this url checking? And a unittest for it?

There is a problem with detecting time of DevTools window deletion, we need to 
reset stack trace after 
ScriptController::setCaptureCallStackForUncaughtExceptions() has been called, 
and it is called after the WebView has been destroyed. 
CefContentRendererClient::OnBrowserDestroyed() is executed before webview is 
destroyed, the same for CefBrowserImpl::FrameDetached/OnDestruct.

I looked over all files in libcef/renderer/ finding a way to do this, but no 
luck.

Original comment by czarek.t...@gmail.com on 22 Oct 2012 at 6:40

[GoogleCodeExporter]

@comment#41: With CEF3 and remote debugging the DevTools client isn't 
necessarily a CEF window (it might be an external browser accessing the 
DevTools URL). This means that we need to track the number of DevTools 
connections in a given render process instead of the number of browser windows 
hosting the DevTools client.

We therefore need to intercept but not handle the DevToolsAgentMsg_Attach and 
DevToolsAgentMsg_Detach messages (defined in 
content/common/devtools_messages.h) and keep the count in 
CefContentRendererClient. This can probably be done using an 
IPC::ChannelProxy::MessageFilter like the (previously deleted) 
CefRenderMessageFilter:

http://code.google.com/p/chromiumembedded/source/browse/trunk/cef3/libcef/render
er/render_message_filter.cc?r=601
http://code.google.com/p/chromiumembedded/source/browse/trunk/cef3/libcef/render
er/render_message_filter.h?r=601

The filter is then added in CefContentRendererClient::RenderThreadStarted() as 
shown here:

http://code.google.com/p/chromiumembedded/source/browse/trunk/cef3/libcef/render
er/content_renderer_client.cc?r=601#80

Original comment by magreenb...@gmail.com on 22 Oct 2012 at 9:35

[GoogleCodeExporter]

I've implemented CefRenderMessageFilter, but the order of calls is still not 
satisfactory:

  CefRenderMessageFilter::OnDevToolsAgentAttach()
  ScriptController::setCaptureCallStackForUncaughtExceptions(1)
  CefRenderMessageFilter::OnDevToolsAgentDetach()
  CefRenderMessageFilter::OnDevToolsAgentDetach_RENDERER_THREAD()
  ScriptController::setCaptureCallStackForUncaughtExceptions(0)

We need to be able to do stuff after ScriptController disabled capturing of 
stack trace.

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 1:03

[GoogleCodeExporter]

After editing content/renderer/devtools_agent.cc:

  void DevToolsAgent::OnDetach() {
    WebDevToolsAgent* web_agent = GetWebAgent();
    if (web_agent) {
      web_agent->detach();
      is_attached_ = false;
    }
    wprintf(L"content/renderer/DevToolsAgent::OnDetach()\n");
  }

The order of calls is fine:

  CefRenderMessageFilter::OnDevToolsAgentAttach()
  ScriptController::setCaptureCallStackForUncaughtExceptions(1)
  CefRenderMessageFilter::OnDevToolsAgentDetach()
  CefRenderMessageFilter::OnDevToolsAgentDetach_RENDERER()
  ScriptController::setCaptureCallStackForUncaughtExceptions(0)
  content/renderer/DevToolsAgent::OnDetach()

How do we overwrite the default implementation of DevToolsAgent::OnDetach()?

Or maybe is there a way to get called after the message has been handled?

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 3:18

[GoogleCodeExporter]

@comment#43-44: Unfortunately DevToolsAgent is not designed to be extended. 
Modifying the Chromium code is one option, but I think it may be sufficient to 
post a task that calls the V8 function after Detach has been handled.

Original comment by magreenb...@gmail.com on 23 Oct 2012 at 3:25

[GoogleCodeExporter]

I am posting the task on renderer thread, it is this line:

  CefRenderMessageFilter::OnDevToolsAgentDetach_RENDERER()

But as you see it executes before scriptcontroller disables capturing stack.

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 3:29

[GoogleCodeExporter]

Or maybe it is possible to call 
WebViewImpl->page()->inspectorController()->disconnectFrontend()?

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 3:35

[GoogleCodeExporter]

@comment#46: This is probably because your task is being posted to the render 
thread before the IPC message is proxied to the render thread for handling by 
OnDetach. You can potentially work around this problem by first posting to the 
IO thread (task will execute after the IPC message has been proxied) and then 
posting to the render thread (task will execute after OnDetach).

Original comment by magreenb...@gmail.com on 23 Oct 2012 at 3:36

[GoogleCodeExporter]

You mean the IO thread of the renderer process? Or the browser process (if so 
which)? In libcef/renderer/thread_util.h I don't see any IO thread macro.

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 3:53

[GoogleCodeExporter]

It is definitely IO thread of the browser, but how do we know to which browser 
post this task? And how to do that from the renderer process?

Original comment by czarek.t...@gmail.com on 23 Oct 2012 at 4:01