[webkit-reviews] review denied: [Bug 239103] [WPE] Add support for generating breakpad dumps : [Attachment 457522] Updated patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Lauro Moura <lmoura at igalia.com>'s
request for review:
Bug 239103: [WPE] Add support for generating breakpad dumps
https://bugs.webkit.org/show_bug.cgi?id=239103

Attachment 457522: Updated patch

https://bugs.webkit.org/attachment.cgi?id=457522&action=review




--- Comment #8 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 457522
  --> https://bugs.webkit.org/attachment.cgi?id=457522
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=457522&action=review

> Source/WebKit/Shared/unix/BreakpadExceptionHandler.cpp:35
> +}

You could write this function as an inline static lambda below.

> Source/WebKit/Shared/unix/BreakpadExceptionHandler.cpp:39
> +    static google_breakpad::ExceptionHandler* exceptionHandler = nullptr;

You'll want to use WTF::MainThreadLazyNeverDestroyed<T> here, which will be
safer and should make it unneeded to apply the UNUSED_PARAM() macro. In this
case the initialization of the static should go into a std::once() invocation.

> Source/WebKit/Shared/unix/BreakpadExceptionHandler.cpp:58
> +#ifdef SIGPIPE

Why does SIGPIPE need to be ignored? A comment here explaining why would
go a long way.

> Source/WebKit/Shared/unix/BreakpadExceptionHandler.cpp:63
> +}

Instead of passing “breakpadCallback” from above, you can replace it with a
static
lambda. If you combine it with the NeverDestroyed stuff suggested above, you
can
call the static “construct()” template method:

  
exceptionHandler.construct(google_breakpad::MinidumpDescriptor(breakpadMinidump
Dir.utf8().data()), nullptr,
       [](const google_breakpad::MinidumpDescriptor&, void*, bool succeeded) ->
bool {
	   return succeeded;
       }, nullptr, true, -1);

> Source/WebKit/Shared/unix/BreakpadExceptionHandler.h:27
> +WK_EXPORT void installExceptionHandler();

The name of the function does not convey what's the goal of the handler. A
better name
would be WebKit::installBreakpadExceptionHandler().

> Source/WebKit/WebProcess/wpe/WebProcessMainWPE.cpp:81
> +#endif

How about processes other than NetworkProcess and WebProcess? For example some
folks are already
working on enabling GPUProcess (as per bug #238593) and we should cover crashes
from there as well.

To avoid repeating code, this should go into
AuxiliaryProcess::platformInitialize(), or maybe
even in AuxiliaryProcess::initialize() -- see
Source/WebKit/Shared/unix/AuxiliaryProcessMain.cpp
Breakpad works in most Unix-y systems (even Darwin) there is nothing about it
specific to the
WPE or GTK ports.