Closed Bug 852476 Opened 11 years ago Closed 11 years ago

Incorrect MOZ_MAKE_MEM_UNDEFINED annotations in nsPresShell.cpp

Categories

(Core :: Layout, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla22

People

(Reporter: jseward, Assigned: decoder)

References

Details

Attachments

(1 file)

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.
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)
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.
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;
    }
Yes, that looks right to me.
Blocks: 838557
Attached patch FixSplinter Review
Assignee: nobody → choller
Status: NEW → ASSIGNED
Attachment #728972 - Flags: review?(matspal)
Comment on attachment 728972 [details] [diff] [review]
Fix

r=mats
Attachment #728972 - Flags: review?(matspal) → review+
https://hg.mozilla.org/mozilla-central/rev/cd563ebad556
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla22
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: