852476 - Incorrect MOZ_MAKE_MEM_UNDEFINED annotations in nsPresShell.cpp

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Julian Seward [:jseward]]

Builds with --disable-jemalloc (irrelevant, I think) and --enable-valgrind
and --enable-debug and --enable-optimize create a huge number of undefined
value errors when run on Valgrind.  It seems to be related to this
part of layout/base/nsPresArena.cpp:

       if (len > 0) {
         // LIFO behavior for best cache utilization
         result = list->mEntries.ElementAt(len - 1);
         list->mEntries.RemoveElementAt(len - 1);
335      MOZ_MAKE_MEM_UNDEFINED(result, list->mEntrySize);
   #if defined(DEBUG)
         {
           char* p = reinterpret_cast<char*>(result);
           char* limit = p + list->mEntrySize;
           for (; p < limit; p += sizeof(uintptr_t)) {
             uintptr_t val = *reinterpret_cast<uintptr_t*>(p);
342          NS_ABORT_IF_FALSE(val == ARENA_POISON,
                               nsPrintfCString("PresArena: poison overwritten; "
                                               "wanted %.16llx "
                                               "found %.16llx "
                                               "errors in bits %.16llx",
                                               uint64_t(ARENA_POISON),
                                               uint64_t(val),
                                               uint64_t(ARENA_POISON ^ val)
                                               ).get());
           }
         }
   #endif
         return result;
       }

It appears that the test at 342 causes V/Memcheck to complain (a lot)
about undefined values that have been created at 335.

Comment 1

[Julian Seward [:jseward]]

Conditional jump or move depends on uninitialised value(s)
   at 0x5B27C77: nsPresArena::AllocateByObjectID(nsPresArena::ObjectID, unsigned long) (layout/base/nsPresArena.cpp:342)
   by 0x5D18B0C: nsStyleContext::operator new(unsigned long, nsPresContext*) (layout/style/../base/nsIPresShell.h:246)
   by 0x5D1908C: NS_NewStyleContext(nsStyleContext*, nsIAtom*, nsCSSPseudoElements::Type, nsRuleNode*, bool) (layout/style/nsStyleContext.cpp:726)
   by 0x5D1CB2F: nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, nsCSSPseudoElements::Type, mozilla::dom::Element*, unsigned int) (layout/style/nsStyleSet.cpp:769)
   by 0x5D201A6: nsStyleSet::ResolveAnonymousBoxStyle(nsIAtom*, nsStyleContext*) (layout/style/nsStyleSet.cpp:1463)
   by 0x5AB86C8: nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent*) (layout/base/nsCSSFrameConstructor.cpp:2770)
   by 0x5AB9D51: nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*, nsIFrame**) (layout/base/nsCSSFrameConstructor.cpp:2325)
   by 0x5ABB2E5: nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (layout/base/nsCSSFrameConstructor.cpp:7046)
   by 0x5ABBC75: nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (layout/base/nsCSSFrameConstructor.cpp:6933)
   by 0x5B4411B: PresShell::Initialize(int, int) (layout/base/nsPresShell.cpp:1732)
   by 0x5E1157D: nsContentSink::StartLayout(bool) (content/base/src/nsContentSink.cpp:1174)
   by 0x64B9C9A: nsHtml5TreeOpExecutor::StartLayout() (parser/html/nsHtml5TreeOpExecutor.cpp:737)

 Uninitialised value was created by a client request
   at 0x5B27C49: nsPresArena::AllocateByObjectID(nsPresArena::ObjectID, unsigned long) (layout/base/nsPresArena.cpp:335)
   by 0x5D18B0C: nsStyleContext::operator new(unsigned long, nsPresContext*) (layout/style/../base/nsIPresShell.h:246)
   by 0x5D1908C: NS_NewStyleContext(nsStyleContext*, nsIAtom*, nsCSSPseudoElements::Type, nsRuleNode*, bool) (layout/style/nsStyleContext.cpp:726)
   by 0x5D1CB2F: nsStyleSet::GetContext(nsStyleContext*, nsRuleNode*, nsRuleNode*, nsIAtom*, nsCSSPseudoElements::Type, mozilla::dom::Element*, unsigned int) (layout/style/nsStyleSet.cpp:769)
   by 0x5D201A6: nsStyleSet::ResolveAnonymousBoxStyle(nsIAtom*, nsStyleContext*) (layout/style/nsStyleSet.cpp:1463)
   by 0x5AB86C8: nsCSSFrameConstructor::SetUpDocElementContainingBlock(nsIContent*) (layout/base/nsCSSFrameConstructor.cpp:2770)
   by 0x5AB9D51: nsCSSFrameConstructor::ConstructDocElementFrame(mozilla::dom::Element*, nsILayoutHistoryState*, nsIFrame**) (layout/base/nsCSSFrameConstructor.cpp:2325)
   by 0x5ABB2E5: nsCSSFrameConstructor::ContentRangeInserted(nsIContent*, nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (layout/base/nsCSSFrameConstructor.cpp:7046)
   by 0x5ABBC75: nsCSSFrameConstructor::ContentInserted(nsIContent*, nsIContent*, nsILayoutHistoryState*, bool) (layout/base/nsCSSFrameConstructor.cpp:6933)
   by 0x5B4411B: PresShell::Initialize(int, int) (layout/base/nsPresShell.cpp:1732)
   by 0x5E1157D: nsContentSink::StartLayout(bool) (content/base/src/nsContentSink.cpp:1174)
   by 0x64B9C9A: nsHtml5TreeOpExecutor::StartLayout() (parser/html/nsHtml5TreeOpExecutor.cpp:737)

Comment 2

[Christian Holler (:decoder)]

This looks like it's my fault. In the debug case we mark the memory as undefined first (which is right in general), but in the debug build, it will also be poisoned and therefore the code in #ifdef debug can check the content. Off the top of my head I would say we can add a MOZ_MAKE_MEM_DEFINED at the beginning of the debug block and a MOZ_MAKE_MEM_UNDEFINED at the end to ensure it's not used afterwards. I wouldn't want to treat the poison value as defined in general as it might hide certain bugs.

Comment 3

[Julian Seward [:jseward]]

This is what you mean, yes?

    void* result;
    if (len > 0) {
      // LIFO behavior for best cache utilization
      result = list->mEntries.ElementAt(len - 1);
      list->mEntries.RemoveElementAt(len - 1);
#if defined(DEBUG)
      {
        MOZ_MAKE_MEM_DEFINED(result, list->mEntrySize);
        char* p = reinterpret_cast<char*>(result);
        char* limit = p + list->mEntrySize;
        for (; p < limit; p += sizeof(uintptr_t)) {
          uintptr_t val = *reinterpret_cast<uintptr_t*>(p);
          NS_ABORT_IF_FALSE(val == ARENA_POISON,
                            nsPrintfCString("PresArena: poison overwritten; "
                                            "wanted %.16llx "
                                            "found %.16llx "
                                            "errors in bits %.16llx",
                                            uint64_t(ARENA_POISON),
                                            uint64_t(val),
                                            uint64_t(ARENA_POISON ^ val)
                                            ).get());
        }
      }
#endif
      MOZ_MAKE_MEM_UNDEFINED(result, list->mEntrySize);
      return result;
    }

Comment 4

[Christian Holler (:decoder)]

Yes, that looks right to me.

Comment 5

[Christian Holler (:decoder)]

Attachment: Fix

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 728972 [details] [diff] [review]
Fix

r=mats

Comment 7

[Christian Holler (:decoder)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cd563ebad556

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/cd563ebad556