[Webkit-unassigned] [Bug 190469] [MSE][GStreamer] Missing opusparse gstreamer element crashes WebKitWebProcess

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=190469

--- Comment #9 from Adrian Perez <aperez at igalia.com> ---
(In reply to Michael Catanzaro from comment #8)
> (In reply to Alicia Boya García from comment #7)
> > In the current master this is a critical rather than a release assert, but
> > still...
> 
> That's even worse, since criticals are programming errors, so if you don't
> want it to fail gracefully, I would change it back to a release assert
> (which can be used to indicate a packaging issue rather than a programming
> error).

Now that I understand what the element is used for (thanks Alicia for
commenting on that), I would go with:

  if (G_UNLIKELY(!element)) {
      WTFLogAlways("Needed opusparse GStreamer element is not available.");
      abort();
  } 

It looks like a much better to have a descriptive message (which could be
even be more detailed than my example above) instead of an assertion which
may leave people puzzled wondering why something is triggering the assertion.

> > I'd rather fail fast on every case so that packagers actually notice the
> > problem and install the missing dependencies.
> 
> I guess it's OK, but then we need to announce the new dependency. We should
> also fail the build if it's missing at build time: not strictly necessary,
> since it's a runtime dependency, but otherwise packagers are not going to
> notice. I would had assumed the desired behavior would have been to just not
> play media if required codecs are missing, but whatever.

Yes, let's list the dependency and announce it properly.

With regard to checking at build time, it sounds like a can of works: one
cannot ask GStreamer to check whether an element is installed (it needs
running some code, which is a no-no for cross-compilation), and randomly
checking a few places on disk will result in false negatives. This is one
more reason why I believe it is important to provide a meaningful error
message designed to be consumed by humans before aborting the WebProcess
(and not just an assertion failure message, which most likely only us
developers will get a sense of!)

-- 
You are receiving this mail because:
You are the assignee for the bug.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.webkit.org/pipermail/webkit-unassigned/attachments/20181013/92efac08/attachment.html>