Bug 239103 – [WPE] Add support for generating breakpad dumps

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Lauro Moura]

[WPE] Add support for generating breakpad dumps

[Lauro Moura]

Created attachment 457310 [details]
Patch

[Lauro Moura]

Comment on attachment 457310 [details]
Patch

Needs a few tweaks

[Lauro Moura]

Created attachment 457522 [details]
Updated patch

Updated patch fixing a compilation issue and adding missing file.

[Philippe Normand]

Is breakpad shipped in the SDK?

[Lauro Moura]

(In reply to Philippe Normand from comment #4)
> Is breakpad shipped in the SDK?

Not yet. I tested it against a breakpad compiled outside the sdk and it worked fine.

Adding it to the SDK would be nice instead of compiling it manually. Maybe in a followup patch, to make it easier to backport this to older WPE versions?

[Adrian Perez]

(In reply to Philippe Normand from comment #4)
> Is breakpad shipped in the SDK?

The following was good enough to have breakpad built, here with
“bst-wrapper build sdk/breakpad.bst”, but I haven't tested any
further than that:

---
# Tools/buildstream/elements/sdk/breakpad.bst
kind: autotools

build-depends:
- freedesktop-sdk.bst:public-stacks/buildsystem-autotools.bst

sources:
- kind: git
  url: https://chromium.googlesource.com/breakpad/breakpad
  ref: d8a7c0548c7b30689df9a65e52b7ab9db37ab9e8
  track: chrome_99
- kind: git
  url: https://chromium.googlesource.com/linux-syscall-support
  ref: e1e7b0ad8ee99a875b272c8e33e308472e897660
  track: main
  directory: src/third_party/lss

public:
  bst:
    integration-commands:
    - |
      minidump-2-core -v

[Adrian Perez]

(In reply to Lauro Moura from comment #5)
> (In reply to Philippe Normand from comment #4)
> > Is breakpad shipped in the SDK?
> 
> Not yet. I tested it against a breakpad compiled outside the sdk and it
> worked fine.
> 
> Adding it to the SDK would be nice instead of compiling it manually. Maybe
> in a followup patch, to make it easier to backport this to older WPE
> versions?

Follow-up patch SGTM. Feel free to pick the example buildstream element
source I copied in my previous comment as basis for the patch :-)

[Adrian Perez]

Comment on attachment 457522 [details]
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(breakpadMinidumpDir.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.

[Philippe Normand]

Comment on attachment 457522 [details]
Updated patch

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

> Source/cmake/OptionsWPE.cmake:79
> +WEBKIT_OPTION_DEFINE(ENABLE_BREAKPAD "Whether or not enable breakpad minidump support." PUBLIC OFF)

Having it disabled on all ports will encourage build breaks and bitrotting... Can we enable this at least in WPE developer builds once the dependency is in the SDK?

[Lauro Moura]

(In reply to Adrian Perez from comment #8)
> Comment on attachment 457522 [details]
> Updated patch
> 

snip.

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

It's used to avoid breakpad's handler not being invoked by early exits like when journald emits SIGPIPE. From https://www.freedesktop.org/software/systemd/man/systemd-journald.service.html#Stream%20logging:

> In order to react gracefully in this (journald stopped) case it is recommended that programs logging to standard output/error ignore such errors. If the SIGPIPE UNIX signal handler is not blocked or turned off, such write attempts will also result in such process signals being generated, see signal(7). To mitigate this issue, systemd service manager explicitly turns off the SIGPIPE signal for all invoked processes by default (this may be changed for each unit individually via the IgnoreSIGPIPE= option, see systemd.exec(5) for details).

I'll add a comment in the updated patch.

[Adrian Perez]

(In reply to Lauro Moura from comment #10)
> (In reply to Adrian Perez from comment #8)
> > Comment on attachment 457522 [details]
> > Updated patch
> > 
> 
> snip.
> 
> > > 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.
> > 
> 
> It's used to avoid breakpad's handler not being invoked by early exits like
> when journald emits SIGPIPE. From
> https://www.freedesktop.org/software/systemd/man/systemd-journald.service.
> html#Stream%20logging:
> 
> > In order to react gracefully in this (journald stopped) case it is recommended that programs logging to standard output/error ignore such errors. If the SIGPIPE UNIX signal handler is not blocked or turned off, such write attempts will also result in such process signals being generated, see signal(7). To mitigate this issue, systemd service manager explicitly turns off the SIGPIPE signal for all invoked processes by default (this may be changed for each unit individually via the IgnoreSIGPIPE= option, see systemd.exec(5) for details).

Thanks for the explanation. This means we want to *always* ignore SIGPIPE
in subprocesses, not only when ENABLE(BREAKPAD) is in effect. A good place
to setup this could be Source/WebKit/Shared/unix/AuxiliaryProcessMain.cpp;
and while at it let's use sigaction() instead of signal() -- also if you
prefer to split the part that configures SIGPIPE to be ignored into a
separate patch, that would be fine =)

> I'll add a comment in the updated patch.

Thanks!

[Lauro Moura]

Pull request: https://github.com/WebKit/WebKit/pull/2893

[EWS]

Committed 253381@main (b4d401778976): <https://commits.webkit.org/253381@main>

Reviewed commits have been landed. Closing PR #2893 and removing active labels.

[Radar WebKit Bug Importer]

<rdar://problem/98575006>