531859 - Remove printf console output from PluginInstance*

Status: RESOLVED FIXED

(Core Graveyard :: Plug-ins, defect)

Description

[Jim Mathies [:jimm]]

Currently log output is printed to console:

#undef _MOZ_LOG
#define _MOZ_LOG(s) printf("[PluginProcessParent] %s\n", s)

This should be tied to low level nspr log output for plugins. Current console output can really drag debug builds down.

Comment 1

[Benjamin Smedberg]

I have a patch.

Comment 2

[Benjamin Smedberg]

Attachment: Fix logging by using NSPR logging instead of printf, rev. 1

When I was being lazy I just replace _MOZ_LOG(__FUNCTION__); with PLUGING_LOG_DEBUG_FUNCTION, but in some cases I was more careful and added more detailed parameter logging.

Comment 3

[Benjamin Smedberg]

Attachment: Fix logging by using NSPR logging instead of printf, rev. 1.1

Whoops, removing necessary code is bad.

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 418430 [details] [diff] [review]
Fix logging by using NSPR logging instead of printf, rev. 1.1

>diff --git a/dom/plugins/Makefile.in b/dom/plugins/Makefile.in
>--- a/dom/plugins/Makefile.in
>+++ b/dom/plugins/Makefile.in
>@@ -111,8 +111,10 @@ LOCAL_INCLUDES = \
>   -I$(topsrcdir)/modules/plugin/base/src/ \
>   $(NULL)
> 
> include $(topsrcdir)/config/config.mk
> include $(topsrcdir)/ipc/chromium/chromium-config.mk
> endif
> 
> include $(topsrcdir)/config/rules.mk
>+
>+DEFINES += -DFORCE_PR_LOG
>\ No newline at end of file

Might as well fix this while we're at it.

>diff --git a/dom/plugins/PluginInstanceChild.cpp b/dom/plugins/PluginInstanceChild.cpp
>--- a/dom/plugins/PluginInstanceChild.cpp
>+++ b/dom/plugins/PluginInstanceChild.cpp
> 
> NPError
> PluginInstanceChild::NPN_SetValue(NPPVariable aVar, void* aValue)
> {
>-    printf ("[PluginInstanceChild] NPN_SetValue(%s, %ld)\n",
>-            NPPVariableToString(aVar), reinterpret_cast<intptr_t>(aValue));
>     AssertPluginThread();
> 

Any reason we're not logging this anymore?

>diff --git a/dom/plugins/PluginMessageUtils.h b/dom/plugins/PluginMessageUtils.h
>--- a/dom/plugins/PluginMessageUtils.h
>+++ b/dom/plugins/PluginMessageUtils.h
>@@ -42,32 +42,39 @@
> #include "IPC/IPCMessageUtils.h"
> 
> #include "npapi.h"
> #include "npruntime.h"
> #include "npfunctions.h"
> #include "nsAutoPtr.h"
> #include "nsStringGlue.h"
> #include "nsThreadUtils.h"
>+#include "prlog.h"
> 
> namespace mozilla {
> 
> // XXX might want to move these to nscore.h or something, they can be
> // generally useful
> struct void_t { };
> struct null_t { };
> 
> namespace ipc {
> 
> typedef intptr_t NPRemoteIdentifier;
> 
> } /* namespace ipc */
> 
> namespace plugins {
> 
>+extern PRLogModuleInfo* gPluginLog;
>+
>+#define PLUGIN_LOG_DEBUG(args) PR_LOG(gPluginLog, PR_LOG_DEBUG, args)
>+#define PLUGIN_LOG_DEBUG_FUNCTION PR_LOG(gPluginLog, PR_LOG_DEBUG, ("%s", __FUNCTION__))
>+#define PLUGIN_LOG_DEBUG_METHOD PR_LOG(gPluginLog, PR_LOG_DEBUG, ("%s<%p>", __FUNCTION__, (void*) this))
>+

It really stinks that this logging doesn't have class/file name.  We could get log output like

  NPP_Foo
  NPP_Foo
  NPP_Foo
  NPP_Foo

Maybe we could log __FILE__ along with __FUNCTION__?  Another option could be a MOZ_NPAPI_THING that each .cpp defines for itself, e.g. |#define MOZ_NPAPI_THING "PluginInstanceChild"|, and PLUGIN_LOG_* magically uses.

Comment 5

[Benjamin Smedberg]

Attachment: Fix logging by using NSPR logging instead of printf, rev. 1.2

Ideally I think that IPDL should do forced PR logging and a lot of this logging is irrelevant, but until then this should do the trick.

Comment 6

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #5)
> Ideally I think that IPDL should do forced PR logging and a lot of this logging
> is irrelevant, but until then this should do the trick.

Yeah, some of this is redundant.  We only need to log (i) messages with params we want to print; (ii) calls to NP* functions that might not result in IPC, like NPN_GetValue().  IPDL can log the rest.

Comment 7

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 420572 [details] [diff] [review]
Fix logging by using NSPR logging instead of printf, rev. 1.2

Cool.  I didn't know about __PRETTY_FUNCTION__.

Comment 8

[Benjamin Smedberg]

http://hg.mozilla.org/mozilla-central/rev/1ef03c8f6ad1