Closed Bug 1287155 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Graphics, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox49 --- unaffected
firefox50 --- fixed

People

(Reporter: kairo, Assigned: kats)

References

Details

Attachments

(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/rules.mk:934: 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 https://dxr.mozilla.org/mozilla-central/source/gfx/layers/AtomicRefCountedWithFinalize.h#44 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 https://hg.mozilla.org/mozilla-central/rev/537e6ba21ea9 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.

[1] https://dxr.mozilla.org/mozilla-central/rev/88bebcaca249aeaca9197382e89d35b02be8292e/xpcom/base/nscore.h#214
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]
Patch

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]
Patch

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.

https://reviewboard.mozilla.org/r/64944/#review61964

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:

```
#ifdef NS_BUILD_REFCNT_LOGGING
  int count = ++mRefCount;
  NS_LOG_ADDREF(..., count, ...);
#endif
```

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:
> 
> ```
> #ifdef NS_BUILD_REFCNT_LOGGING
>   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.
https://reviewboard.mozilla.org/r/64942/#review62034

This compiles correctly for me, thanks!
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d0a139d1f27c
Fix build bustage when --disable-debug is combined with --enable-logrefcnt. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/d0a139d1f27c
Status: NEW → RESOLVED
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.