Bug 177360

Summary: [GCrypt] Only report libgcrypt errors when logging is enabled
Product: WebKit Reporter: Zan Dobersek <zan>
Component: New BugsAssignee: Zan Dobersek <zan>
Severity: Normal CC: buildbot, calvaris, jiewen_tan, mcatanzaro, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Description Flags
Patch none

Description Zan Dobersek 2017-09-22 05:54:39 PDT
[GCrypt] Only report libgcrypt errors when logging is enabled
Comment 1 Zan Dobersek 2017-09-22 05:59:03 PDT
Created attachment 321535 [details]
Comment 2 Michael Catanzaro 2017-09-22 06:29:11 PDT
Comment on attachment 321535 [details]

I would expect errors to always be printed to stdout regardless of whether logging is enabled or not. What sort of error spam are you seeing that makes it preferable to silence all errors from libgcrypt?

I don't think using LOG_DISABLED to guard a call to WTFLogAlways makes sense, since WTFLogAlways is what you use to log something even when logs are disabled. You could use ENABLE(DEVELOPER_MODE), but even that doesn't make sense since there's no fundamental reason why errors should be silenced in release mode but not developer mode.

If you really need to silence these errors, I think the right way would be to define a new log channel in Logging.h for WebCrypto. The problem is that's complicated by the fact that we have this weird transitional split between WebCore/platform and PAL right now, where PAL can't use WebCore/platform. I think PAL is causing more problems than it solves so long as we have it left in this transitional state. I guess PAL needs its own Logging.h.

But logging is really intended for debug: errors should print always unless you have a *really* good reason not to.
Comment 3 Michael Catanzaro 2017-09-22 06:30:08 PDT
In general, we rely on stderr output to debug problems all the time, and it would be really unfortunate if error messages that we're used to seeing in development builds are not present in release builds.
Comment 4 Zan Dobersek 2017-09-24 03:30:32 PDT
Currently this prints out libgcrypt errors whenever WebCrypto API is misused by the Web application. I don't see how it's helpful to the end-user who has no interest in debugging to see all this error output.
Comment 5 Zan Dobersek 2017-09-24 03:34:15 PDT
I mean, there's a bunch of logging used in GStreamer code that is only usable when compiling with !LOG_DISABLED. This isn't any different.
Comment 6 Michael Catanzaro 2017-09-24 08:24:12 PDT
I don't know about GStreamer.

I think we should *never* print any errors to stderr when the WebCrypto API is misused by the web application. But we *should* always send console messages to the web inspector.
Comment 7 Zan Dobersek 2017-09-25 04:45:21 PDT
(In reply to Michael Catanzaro from comment #6)
> I think we should *never* print any errors to stderr when the WebCrypto API
> is misused by the web application.

This patch fixes that, while still enabling such logging for debugging purposes when !LOG_DISABLED is true.

> But we *should* always send console messages to the web inspector.

This isn't mandated anywhere. Neither do I know of any other API that does that in similar use cases. Regardless, providing libgcrypt-level details of these errors makes no sense to me.
Comment 8 Michael Catanzaro 2017-09-25 07:21:41 PDT
I still think this is not the best approach, but I'll defer to you. You can ask Calvaris to reset the r+.
Comment 9 Zan Dobersek 2017-09-25 08:29:08 PDT
Created attachment 321688 [details]

Adds a FIXME that instructs using a WTF log channel once available in PAL.
Comment 10 Zan Dobersek 2017-09-26 01:38:19 PDT
Comment on attachment 321688 [details]

Clearing flags on attachment: 321688

Committed r222497: <http://trac.webkit.org/changeset/222497>
Comment 11 Zan Dobersek 2017-09-26 01:38:23 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Radar WebKit Bug Importer 2017-09-27 12:17:39 PDT