Bug 177360 – [GCrypt] Only report libgcrypt errors when logging is enabled

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

[Zan Dobersek]

[GCrypt] Only report libgcrypt errors when logging is enabled

[Zan Dobersek]

Created attachment 321535 [details]
Patch

[Michael Catanzaro]

Comment on attachment 321535 [details]
Patch

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.

[Michael Catanzaro]

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.

[Zan Dobersek]

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.

[Zan Dobersek]

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.

[Michael Catanzaro]

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.

[Zan Dobersek]

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

[Michael Catanzaro]

I still think this is not the best approach, but I'll defer to you. You can ask Calvaris to reset the r+.

[Zan Dobersek]

Created attachment 321688 [details]
Patch

Adds a FIXME that instructs using a WTF log channel once available in PAL.

[Zan Dobersek]

Comment on attachment 321688 [details]
Patch

Clearing flags on attachment: 321688

Committed r222497: <http://trac.webkit.org/changeset/222497>

[Zan Dobersek]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34692969>