Closed
Bug 1074415
Opened 10 years ago
Closed 10 years ago
Build error in nsNPAPIPluginInstance when enabling PR_LOGGING in non-debug builds
Categories
(Core Graveyard :: Plug-ins, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla35
People
(Reporter: erahm, Assigned: erahm)
References
Details
Attachments
(1 file, 2 obsolete files)
2.07 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
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 | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
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 4•10 years ago
|
||
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+
Assignee | ||
Comment 5•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/fd8e58a613da
Comment 6•10 years ago
|
||
Backed out for Linux Werror bustage. https://hg.mozilla.org/integration/mozilla-inbound/rev/e0baa8287713 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2724707&repo=mozilla-inbound
Comment 7•10 years ago
|
||
(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.)
Assignee | ||
Comment 8•10 years ago
|
||
(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
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8497036 -
Attachment is obsolete: true
Comment 10•10 years ago
|
||
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.
Assignee | ||
Comment 11•10 years ago
|
||
Okay I just did what Daniel recommended, we can leave the horribleness that is nsPluginLogging.h for another day.
Attachment #8499221 -
Flags: review?(dholbert)
Assignee | ||
Updated•10 years ago
|
Attachment #8499190 -
Attachment is obsolete: true
Attachment #8499190 -
Flags: review?(jschoenick)
Comment 12•10 years ago
|
||
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+
Assignee | ||
Comment 13•10 years ago
|
||
try: https://tbpl.mozilla.org/?tree=Try&rev=7462b0fb74ec remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/8feab238d757
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/8feab238d757
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Updated•9 years ago
|
Flags: qe-verify-
Updated•2 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•