[webkit-reviews] review denied: [Bug 207756] [WPE] API for pause/resume rendering : [Attachment 390913] Patch

[bugzilla-daemon at webkit.org]

Adrian Perez <aperez at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 207756: [WPE] API for pause/resume rendering
https://bugs.webkit.org/show_bug.cgi?id=207756

Attachment 390913: Patch

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




--- Comment #15 from Adrian Perez <aperez at igalia.com> ---
Comment on attachment 390913
  --> https://bugs.webkit.org/attachment.cgi?id=390913
Patch

First of all, I really don't like using signals for this. Signals are
troublesome for many reasons (many of them outlined in the comments
below) and furthermore it is not the business of a library to decide
how a program will behave depending on how the library was built.

Now, on the positive side: I do truly believe that something along the
lines of this patch can be useful in many scenarios, and I would love
to see the functionality landed in WPE (and also the GTK port!) in some
form. Adding API to allow an application to suspend pages (= webviews)
is something that would allow for example to let browsers (like GNOME
Web, or Cog) to completely or partially suspend/throttle pages which
are inactive (for example: non visible browser tabs).

My preference regarding would be to see this patch in WebKit *without*
the signal handling parts. If any application needs to do it using
signals for whatever reason, it's the application where the code
belongs, and if the application cannot be modified, people are still
free to use a patched build of WebKit—but I am convinced that signal
handling must be avoided at all costs in upstream WebKit.

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

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:310
> +    unsigned signalCheckSourceID { 0 };

It does not seem like a great idea to have one source per web view,
when signals are global things. I think it makes more sense to have
a singleton source, and register/unregister web views from a list
of web views to (un)suspend inside webkitWebViewConstructed() and
webkitWebViewDispose().

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:747
> +    signal(sig, wpeUIProcessSignalHandler);

Why is this needed? Signal handlers are never reset automatically,
one has to explicitly call signal(signum, SIG_DFL) to go back to the
default signal handler.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:752
> +	   s_sendResumeRenderingPending = true;

else
	UNREACHABLE();

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:831
> +#if HAVE_SIGNAL_H && PLATFORM(WPE) && defined(RENDERING_PAUSE_SIGNAL) &&
defined(RENDERING_RESUME_SIGNAL)

This could be as well be in the GTK port, I think we should not have
PLATFORM(WPE) guards here.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:832
> +    void (*pauseHandler)(int) = signal(RENDERING_PAUSE_SIGNAL,
wpeUIProcessSignalHandler);

It's very unfortunate g_unix_signal_source_new() only allows a small
subset of all possible signal numbers—that is quite silly. At least
if we are going to do signal handling manually, please use sigaction()
because it's safer to than plain ol' signal()... with sigaction() you
must set the SA_RESTART flag, so system calls which might have been
interrupted when the signal handler is called will be restarted—this
is unavailable with signal()—and also with sigaction() the signal being
delivered is temporarily added to the thread's signal mask to avoid
re-deliveries of the signal while the signal is being processed.

Even better would be to use signalfd() on Linux, and then handle that
file descriptor with a GSource created using g_unix_fd_source_new()
in the event loop—this completely bypasses the problem that there are
not that many signal-safe functions. JFTR, the equivalent for *BSD
would be to use kqueue() and an event type with EVFILT_SIGNAL.

I think at least changing the code to use sigaction() is a must.

(Yes, using Unix signals is a perilous affair. TL;DR: never do it
if you can avoid it.)

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:834
> +	   signal(RENDERING_PAUSE_SIGNAL, pauseHandler);

This code seems unnecessarily complicated to understand. Why do you even need
to do this? It looks like the idea is to try to use wpeUIProcessSignalHandler
but if there was some signal handler already registered, then configure it
back.

This seems like a terrible idea because if, let's say, a program accidentally
registers a handler for the resume signal, then it's possible to send a signal
to pause rendering, but it won't be possible to ever resume it!

If you want to keep existing signal handlers working, what you have to do
is keep a reference to them around, and invoke them from your handler in
turn. If you use a global handler as suggested, it's much easier to keep
a single sighandler_t value around for the existing handler, then do:

  static const sighandler_t s_existingSuspendSignalHandler = SIG_DFL;

  static void wpeUIProcessSignalHandler(int sig) {
      if (sig == RENDERING_PAUSE_SIGNAL) {
	  s_sendPauseRenderingPending = true;
	  if (s_existingSuspendSignalHandler != SIG_DFL)
	      s_existingSuspendSignalHandler(sig);
      } else if (sig == RENDERING_RESUME_SIGNAL) {
	  // Ditto for resuming.
      } else {
	  UNREACHABLE();
      }
  }

Then, when registering the signal:

  s_existingSuspendSignalHandler = signal(REDNERING_SUSPEND_SIGNAL,
wpeUIProcessSignalHandler);

This way you never need to play whack-a-mole with the installed signal
handlers, and previously existing handlers will be called without being
in a situation where one of the handlers is the WebKit-provided one and
another is the user-provided one.

Personally, I would go one step further, and use sigaction() passing
NULL as second parameter to obtain the current signal handler information
without changing it, and it is other than SIG_DFL for either signal, I
would just exit forcibly with an error: whoever thinks is a good idea to
use signals for this must let WebKit handle the signals.

> Source/WebKit/UIProcess/API/glib/WebKitWebView.cpp:998
> +    }

You can change these for lines to just:

    g_clear_handle_id(&webView->priv->signalCheckSourceID, g_source_remove);

> Source/WebKit/UIProcess/API/wpe/WebKitWebViewWPE.cpp:244
> +    }

Why is WebPage::setIsSuspended() not being used here? I would imagine that
one wants the whole page suspending, and not just media playback. If only
media playback suspension is desired in some cases, then wouldn't it be
better to use a tristate value here (Unpaused, Paused, MediaPaused)?
Then the property would be an “enum” instead of a bool/gboolean value.

> Source/cmake/OptionsWPE.cmake:215
> +    if (CMAKE_MATCH_1 LESS 32)

This check can fail for valid signals in the [SIGRTMIN, SIGRTMAX]
interval, which usually (but not always) has values >32. I would
remove this check and instead only check that the values are:

 - Greater than zero (because zero is not a valid signal number).
 - Different than SIGKILL (because it cannot be handled).
 - Different than SIGBUS or SIGSEGV (because those are used for
   signaling invalid access to memory, we would rather have the
   default bahavior of coredump+abort for those).
 - Different from SIGABRT (because we want abort() calls to work).
 - Different from SIGFPE and SIGILL (similar reasoning as above).
 - Different from SIGTRAP (we want debuggers to always work).
 - Different from SIGALMR (which is used by JSC's profiler code).
 - Different from SIGCLD (used to watch for exited child processes).