Closed Bug 1287155 Opened 4 years ago Closed 4 years ago

--disable-debug --enable-logrefcnt bustage in AtomicRefCountedWithFinalize.h


(Core :: Graphics, defect)

Not set



Tracking Status
firefox49 --- unaffected
firefox50 --- fixed


(Reporter: kairo, Assigned: kats)




(1 file, 1 obsolete file)

Recently I have been running into the following bustage in my Firefox builds (ignore the comm-cental in the path, it's just an artifact of where my source is placed):

/usr/bin/ccache /usr/bin/g++ -std=gnu++11 -o brkeng.o -c  -I/mnt/mozilla/build/firefox/dist/system_wrappers -include /mnt/mozilla/hg/comm-central/mozilla/config/gcc_hidden.h -DNDEBUG=1 -DTRIMMED=1 -DU_COMMON_IMPLEMENTATION -DLOCALE_SNAME=92 -DUCONFIG_NO_BREAK_ITERATION -DUCONFIG_NO_TRANSLITERATION -DUCONFIG_NO_REGULAR_EXPRESSIONS -DUCONFIG_NO_LEGACY_CONVERSION -DU_USING_ICU_NAMESPACE=0 -DU_NO_DEFAULT_INCLUDE_UTF_HEADERS=1 -DU_CHARSET_IS_UTF8 -I/mnt/mozilla/hg/comm-central/mozilla/config/external/icu/common -I/mnt/mozilla/build/firefox/config/external/icu/common -I/mnt/mozilla/hg/comm-central/mozilla/intl/icu/source/common -I/mnt/mozilla/hg/comm-central/mozilla/intl/icu/source/i18n -I/mnt/mozilla/build/firefox/dist/include  -I/mnt/mozilla/build/firefox/dist/include/nspr -I/mnt/mozilla/build/firefox/dist/include/nss       -fPIC  -DMOZILLA_CLIENT -include /mnt/mozilla/build/firefox/mozilla-config.h -MD -MP -MF .deps/brkeng.o.pp  -Wall -Wc++11-compat -Wempty-body -Wignored-qualifiers -Woverloaded-virtual -Wpointer-arith -Wsign-compare -Wtype-limits -Wunreachable-code -Wwrite-strings -Wc++14-compat -Wno-invalid-offsetof -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -Wno-error=array-bounds -Wno-unused-value -fno-exceptions -fno-strict-aliasing -fno-rtti -fno-exceptions -fno-math-errno -pthread -pipe  -g -freorder-blocks -Os -fomit-frame-pointer -frtti  /mnt/mozilla/hg/comm-central/mozilla/intl/icu/source/common/brkeng.cpp
In file included from ../../dist/include/mozilla/layers/ISurfaceAllocator.h:19:0,
                 from ../../dist/include/mozilla/layers/CompositorBridgeParent.h:31,
                 from ../../dist/include/mozilla/dom/TabParent.h:20,
                 from /mnt/mozilla/hg/comm-central/mozilla/dom/presentation/PresentationSessionInfo.cpp:9,
                 from Unified_cpp_dom_presentation0.cpp:101:
../../dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h: In member function 'void mozilla::AtomicRefCountedWithFinalize<T>::AddRef()':
../../dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h:114:38: error: 'mName' was not declared in this scope
       NS_LOG_ADDREF(this, count, mName, sizeof(*this));
../../dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h: In member function 'void mozilla::AtomicRefCountedWithFinalize<T>::Release()':
../../dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h:130:43: error: 'mName' was not declared in this scope
       NS_LOG_RELEASE(this, currCount, mName);
gmake[5]: *** [/mnt/mozilla/hg/comm-central/mozilla/config/ Unified_cpp_dom_presentation0.o] Error 1
gmake[5]: Leaving directory '/mnt/mozilla/build/firefox/dom/presentation'

Now I have been building with --disable-debug and I see that seems to only define mName |#ifdef DEBUG| while on line 114 and 130, it's used unconditionally. I guess that's the error.

All this was recently introduced in of bug 1280297, and that IIRC also fits approximately the time when my builds started failing (I don't necessarily build daily, esp. nowadays).

Kats, I guess you should take a look at that.
Flags: needinfo?(bugmail)
Sure, I can take a look. for the record, NS_LOG_ADDREF is defined to nothing unless NS_BUILD_REFCNT_LOGGING is defined, and that in turn is only defined if DEBUG or FORCE_BUILD_REFCNT_LOGGING are defined [1]. So if DEBUG is not defined for you, then I'm guessing you have FORCE_BUILD_REFCNT_LOGGING somewhere in your config, which is probably why nobody else has run into it yet.

Still, it should be an easy fix.

Assignee: nobody → bugmail
Component: Layout → Graphics
Flags: needinfo?(bugmail)
Hmm, I added --enable-logrefcnt to my config at some point, I guess that together with --disable-debug causes the issue. Maybe I should remove one or both of those options.
Attached patch Patch (obsolete) — Splinter Review
Here's a patch that should fix it. If you still have the configuration, mind trying it?
Attachment #8771489 - Flags: review?(kairo)
Comment on attachment 8771489 [details] [diff] [review]

Review of attachment 8771489 [details] [diff] [review]:

Sorry, I still fail with this now:

../dist/include/mozilla/layers/AtomicRefCountedWithFinalize.h:116:19: error: cannot convert 'mozilla::DebugOnly<int>' to 'nsrefcnt {aka long unsigned int}' for argument '2' to 'void NS_LogAddRef(void*, nsrefcnt, const char*, uint32_t)'
       NS_LOG_ADDREF(this, count, mName, sizeof(*this));
Attachment #8771489 - Flags: review?(kairo) → review-
Comment on attachment 8771489 [details] [diff] [review]

Ah, sorry I missed that. I did a local build using your configuration and fixed both errors. I'll put up a MozReview with the new patch which I verified locally.
Attachment #8771489 - Attachment is obsolete: true
Comment on attachment 8772000 [details]
Bug 1287155 - Fix build bustage when --disable-debug is combined with --enable-logrefcnt.

r=me, your call on whether you think changing it to not touch the refcount twice for logging purposes is worth it.

::: gfx/layers/AtomicRefCountedWithFinalize.h:115
(Diff revision 1)
> -      DebugOnly<int> count = ++mRefCount;
> -      NS_LOG_ADDREF(this, count, mName, sizeof(*this));
> +      mRefCount++;
> +      NS_LOG_ADDREF(this, mRefCount, mName, sizeof(*this));

This change makes me sad; could we do it:

  int count = ++mRefCount;
  NS_LOG_ADDREF(..., count, ...);

or does that seem too ugly?
Attachment #8772000 - Flags: review?(nfroyd) → review+
Summary: --disable-debug bustage in AtomicRefCountedWithFinalize.h → --disable-debug --enable-logrefcnt bustage in AtomicRefCountedWithFinalize.h
(In reply to Nathan Froyd [:froydnj] from comment #7)
> This change makes me sad; could we do it:
> ```
>   int count = ++mRefCount;
>   NS_LOG_ADDREF(..., count, ...);
> #endif
> ```
> or does that seem too ugly?

That would also be incorrect if NS_BUILD_REFCNT_LOGGING is not defined, because mRefCount wouldn't get incremented. We'd need an #else which makes it even uglier. Personally I don't think this class is in such hot code that touching mRefCount a second time for debug/log-refcnt builds only is a big deal.

This compiles correctly for me, thanks!
Pushed by
Fix build bustage when --disable-debug is combined with --enable-logrefcnt. r=froydnj
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.