chromiumembedded / cef

Chromium Embedded Framework (CEF). A simple framework for embedding Chromium-based browsers in other applications.
https://bitbucket.org/chromiumembedded/cef/
Other
3.31k stars 463 forks source link

Implement global v8 exception handler #736

Closed magreenblatt closed 12 years ago

magreenblatt commented 12 years ago

Original report by Anonymous.


Original issue 736 created by czarek.tomczak on 2012-09-29T14:17:43.000Z:

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().

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 1. originally posted by czarek.tomczak on 2012-09-29T14:27:39.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 2. originally posted by czarek.tomczak on 2012-09-29T18:17:51.000Z:

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?

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 3. originally posted by czarek.tomczak on 2012-09-30T07:01:05.000Z:

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.
magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 4. originally posted by czarek.tomczak on 2012-09-30T10:36:59.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 5. originally posted by czarek.tomczak on 2012-09-30T12:57:59.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 6. originally posted by czarek.tomczak on 2012-10-01T08:55:29.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 7. originally posted by czarek.tomczak on 2012-10-01T14:11:02.000Z:

Patch is ready, attaching "OnScriptException_cef1_trunk.patch".

Changes:

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:

magreenblatt commented 12 years ago

Comment 8. originally posted by magreenblatt on 2012-10-01T14:58:02.000Z:

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=7360007000&name=OnScriptException\_cef1\_trunk\_r823.patch&token=PLi2u-gYycvsuSnmKivsr1GVoUY%3A1349100781974comment 19.6

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

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360007000&name=OnScriptException\_cef1\_trunk\_r823.patch&token=PLi2u-gYycvsuSnmKivsr1GVoUY%3A1349100781974comment 15.9

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

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360007000&name=OnScriptException\_cef1\_trunk\_r823.patch&token=PLi2u-gYycvsuSnmKivsr1GVoUY%3A1349100781974comment 29.2

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

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360007000&name=OnScriptException\_cef1\_trunk\_r823.patch&token=PLi2u-gYycvsuSnmKivsr1GVoUY%3A1349100781974comment 42.7

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.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360007000&name=OnScriptException\_cef1\_trunk\_r823.patch&token=PLi2u-gYycvsuSnmKivsr1GVoUY%3A1349100781974comment 28.7

+void CefV8MessageHandler(v8::Handle message, v8::Handle data)
+{

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

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360007000&name=OnScriptException\_cef1\_trunk\_r823.patch&token=PLi2u-gYycvsuSnmKivsr1GVoUY%3A1349100781974comment 37.3

Wrap lines to 80 characters.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 9. originally posted by czarek.tomczak on 2012-10-01T18:12:41.000Z:

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.

magreenblatt commented 12 years ago

Comment 10. originally posted by magreenblatt on 2012-10-01T18:32:04.000Z:

@ commentcomment 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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 11. originally posted by czarek.tomczak on 2012-10-01T18:56:57.000Z:

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?

magreenblatt commented 12 years ago

Comment 12. originally posted by magreenblatt on 2012-10-01T19:01:39.000Z:

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

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 13. originally posted by czarek.tomczak on 2012-10-02T11:14:32.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 14. originally posted by czarek.tomczak on 2012-10-02T14:33:31.000Z:

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
magreenblatt commented 12 years ago

Comment 15. originally posted by magreenblatt on 2012-10-02T14:37:26.000Z:

@ commentcomment 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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 16. originally posted by czarek.tomczak on 2012-10-02T15:43:11.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 17. originally posted by czarek.tomczak on 2012-10-02T19:24:23.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 18. originally posted by czarek.tomczak on 2012-10-03T06:58:18.000Z:

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

magreenblatt commented 12 years ago

Comment 19. originally posted by magreenblatt on 2012-10-03T20:45:26.000Z:

Some comments about your most recent patch:

http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360018000&name=OnScriptException3\_cef1\_trunk\_r834.patch&token=amnKXD43ahOgTb\_qx9h-aTjKpWA%3A1349295082718comment 25.4

+void CefV8EnableGlobalExceptionHandler() {

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?

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 20. originally posted by czarek.tomczak on 2012-10-04T07:41:24.000Z:

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?

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 21. originally posted by czarek.tomczak on 2012-10-04T08:03:48.000Z:

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_?

magreenblatt commented 12 years ago

Comment 22. originally posted by magreenblatt on 2012-10-04T19:54:28.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 23. originally posted by czarek.tomczak on 2012-10-04T23:02:14.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 24. originally posted by czarek.tomczak on 2012-10-06T19:07:53.000Z:

I've updated to revision 844, updated chromium to 160122 (bb). 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".

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 25. originally posted by czarek.tomczak on 2012-10-06T19:10:13.000Z:

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

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 26. originally posted by czarek.tomczak on 2012-10-06T19:16:25.000Z:

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 comment 24., it addresses issues you mentioned in comment comment 22..

magreenblatt commented 12 years ago

Comment 27. originally posted by magreenblatt on 2012-10-08T17:02:02.000Z:

Overall looks good. Some comments on your new patch:

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360024000&name=OnUncaughtException4\_cef1\_trunk\_r844.patch&token=wftVH9PXsouPthMgkilRpH-HA2A%3A1349713830883comment 21.0

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

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 28. originally posted by czarek.tomczak on 2012-10-08T17:39:02.000Z:

Done, attaching new patch.

magreenblatt commented 12 years ago

Comment 29. originally posted by magreenblatt on 2012-10-08T19:25:12.000Z:

Additional comments:

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360028000&name=OnUncaughtException5\_cef1\_trunk\_r844.patch&token=Zqbfru1vqGRhyJJeDOwl7gkrsy8%3A1349718032803comment 13.4

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.

  1. http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360028000&name=OnUncaughtException5\_cef1\_trunk\_r844.patch&token=Zqbfru1vqGRhyJJeDOwl7gkrsy8%3A1349718032803comment 48.1

http://code.google.com/p/chromiumembedded/issues/attachmentText?id=736&aid=7360028000&name=OnUncaughtException5\_cef1\_trunk\_r844.patch&token=Zqbfru1vqGRhyJJeDOwl7gkrsy8%3A1349718032803comment 51.3

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 30. originally posted by czarek.tomczak on 2012-10-10T08:37:15.000Z:

Attaching new patch with your comments addressed.

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 31. originally posted by czarek.tomczak on 2012-10-10T08:52:21.000Z:

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.

magreenblatt commented 12 years ago

Comment 32. originally posted by magreenblatt on 2012-10-12T16:54:05.000Z:

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? :-)

@ commentcomment 31.: Could be a bug in WebKit.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 33. originally posted by czarek.tomczak on 2012-10-12T17:37:38.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 34. originally posted by czarek.tomczak on 2012-10-19T17:32:42.000Z:

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
magreenblatt commented 12 years ago

Comment 35. originally posted by magreenblatt on 2012-10-19T17:39:28.000Z:

[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?

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 36. originally posted by czarek.tomczak on 2012-10-19T18:08:25.000Z:

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

magreenblatt commented 12 years ago

Comment 37. originally posted by magreenblatt on 2012-10-19T18:33:28.000Z:

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

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 38. originally posted by czarek.tomczak on 2012-10-21T19:47:10.000Z:

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?

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 39. originally posted by czarek.tomczak on 2012-10-22T07:43:13.000Z:

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

magreenblatt commented 12 years ago

Comment 40. originally posted by magreenblatt on 2012-10-22T15:35:48.000Z:

@ commentcomment 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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 41. originally posted by czarek.tomczak on 2012-10-22T18:40:37.000Z:

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.

magreenblatt commented 12 years ago

Comment 42. originally posted by magreenblatt on 2012-10-22T21:35:47.000Z:

@ commentcomment 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/renderer/render\_message\_filter.cc?r=601
http://code.google.com/p/chromiumembedded/source/browse/trunk/cef3/libcef/renderer/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/renderer/content\_renderer\_client.cc?r=601comment 80.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 43. originally posted by czarek.tomczak on 2012-10-23T13:03:19.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 44. originally posted by czarek.tomczak on 2012-10-23T15:18:50.000Z:

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?

magreenblatt commented 12 years ago

Comment 45. originally posted by magreenblatt on 2012-10-23T15:25:19.000Z:

@ commentcomment 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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 46. originally posted by czarek.tomczak on 2012-10-23T15:29:08.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 47. originally posted by czarek.tomczak on 2012-10-23T15:35:39.000Z:

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

magreenblatt commented 12 years ago

Comment 48. originally posted by magreenblatt on 2012-10-23T15:36:53.000Z:

@ commentcomment 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).

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 49. originally posted by czarek.tomczak on 2012-10-23T15:53:32.000Z:

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.

magreenblatt commented 12 years ago

Original comment by Anonymous.


Comment 50. originally posted by czarek.tomczak on 2012-10-23T16:01:22.000Z:

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?