Closed Bug 1074415 Opened 5 years ago Closed 5 years ago

Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds

Categories

(Core :: Plug-ins, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla35

People

(Reporter: erahm, Assigned: erahm)

References

Details

Attachments

(1 file, 2 obsolete files)

When DEBUG isn't defined, but PR_LOGGING is we end up trying to log a DebugOnly value.

> 0:23.55 /Users/ericrahm/dev/mozilla-central/dom/plugins/base/nsNPAPIPluginInstance.cpp:570:99: error: cannot convert 'DebugOnly<NPError>' to 'NPError' (aka 'short') without a conversion operator
> 0:23.55     window->clipRect.top, window->clipRect.bottom, window->clipRect.left, window->clipRect.right, (NPError)error));
> 0:23.55                                                                                                   ^~~~~~~~~~~~~~
> 0:23.55 /Users/ericrahm/dev/mozilla-central/dom/plugins/base/nsPluginLogging.h:79:40: note: expanded from macro 'NPP_PLUGIN_LOG'
> 0:23.55    PR_LOG(nsPluginLogging::gNPPLog, a, b); \
> 0:23.55                                        ^
> 0:23.56 /Users/ericrahm/dev/mozilla-central/obj-x86_64-apple-darwin13.4.0/dist/include/nspr/prlog.h:181:19: note: expanded from macro 'PR_LOG'
> 0:23.56       PR_LogPrint _args;         \
> 0:23.56                   ^
> 0:23.56 1 error generated.
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 8497036 [details] [diff] [review]
Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds

Don't use DebugOnly if the build has logging enabled.
Attachment #8497036 - Flags: review?(benjamin)
Blocks: 806819
Comment on attachment 8497036 [details] [diff] [review]
Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds

Review of attachment 8497036 [details] [diff] [review]:
-----------------------------------------------------------------

John, bsmedberg indicated you could take a look at this small review.
Attachment #8497036 - Flags: review?(benjamin) → review?(jschoenick)
Comment on attachment 8497036 [details] [diff] [review]
Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds

r=me
Attachment #8497036 - Flags: review?(jschoenick) → review+
(This probably needs to be conditional on PLUGIN_LOGGING, since the logging macro in question here only uses its arguments if PLUGIN_LOGGING is defined.)
(In reply to Daniel Holbert [:dholbert] from comment #7)
> (This probably needs to be conditional on PLUGIN_LOGGING, since the logging
> macro in question here only uses its arguments if PLUGIN_LOGGING is defined.)

That fails too [1]. The issue appears to be related to including prlog.h, then including nsPluginLogging.h. nsPluginLogging.h does bad things like trying to force NSPR logging in a header, defining an NSPR internal that then overrides it's own ifdefs. This bug has been around for about 12 years at least [2].

Anyhow the rundown of the bustage is:
#include "prlog.h"
  => defines it's include guards
  => FORCE_PR_LOG isn't set, so stubs out PR_LOG and friends
  => FORCE_PR_LOG isn't set, so PR_LOGGING doesn't get set
#include "nsPluginLogging.h"
  => defines FORCE_PR_LOG
  => defines PR_LOGGING (which it most certainly should not)
  => makes sure PLUGIN_LOGGING is always defined
  => #includes "prlog.h", this does nothing b/c it hits its include guards
  => sets up some macros that just make you do a bunch of PR_LogFlushes b/c PR_LOG is stubbed out

So then we get to my change, PR_LOGGING has been force defined, PLUGIN_LOGGING has been force defined, so I don't have a good test to see if the value is going to be used.

[1] https://tbpl.mozilla.org/?tree=Try&rev=7d2790d31a62
[2] https://github.com/mozilla/gecko-dev/commit/973ae91db598f40050070a0300ff21ce06f582e3
John, this changed just enough that you might want to review again. Really the only change is that nsPluginLogging.h no longer #defines PR_LOGGING.
Attachment #8499190 - Flags: review?(jschoenick)
Attachment #8497036 - Attachment is obsolete: true
Won't this still cause problems in builds with PR_LOGGING defined, but with PLUGIN_LOGGING *not* defined?

(It looks like PLUGIN_LOGGING is defined by default, so maybe that's not worth worrying about. Still, it looks like it's set up to let you enable/disable it as you like...)

It might be simpler to just drop the DebugOnly<> version of this variable, and pass it to mozilla::unused, with a comment saying something like:
 // 'error' is only used if this is a logging-enabled build.
 // That is somewhat complex to check, so we just use "unused"
 // to suppress any compiler warnings in build configurations
 // where the logging is a no-op.
Okay I just did what Daniel recommended, we can leave the horribleness that is nsPluginLogging.h for another day.
Attachment #8499221 - Flags: review?(dholbert)
Attachment #8499190 - Attachment is obsolete: true
Attachment #8499190 - Flags: review?(jschoenick)
Comment on attachment 8499221 [details] [diff] [review]
Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds

>Bug 1074415 - Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds. r=johns

(The commit message should have r=dholbert now, I guess)

>-    DebugOnly<NPError> error;
>+    NPError error;
[...]
>     window->clipRect.top, window->clipRect.bottom, window->clipRect.left, window->clipRect.right, (NPError)error));

Drop the (NPError) cast in the actual logging invocation -- I don't think we need it, now that that's the actual type.

r=me with that.
Attachment #8499221 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/8feab238d757
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.