Closed
Bug 852476
Opened 11 years ago
Closed 11 years ago
Incorrect MOZ_MAKE_MEM_UNDEFINED annotations in nsPresShell.cpp
Categories
(Core :: Layout, defect)
Tracking
()
RESOLVED
FIXED
mozilla22
People
(Reporter: jseward, Assigned: decoder)
References
Details
Attachments
(1 file)
1.84 KB,
patch
|
MatsPalmgren_bugz
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•11 years ago
|
||
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)
Assignee | ||
Comment 2•11 years ago
|
||
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.
Reporter | ||
Comment 3•11 years ago
|
||
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; }
Assignee | ||
Comment 4•11 years ago
|
||
Yes, that looks right to me.
Assignee | ||
Comment 5•11 years ago
|
||
Comment 6•11 years ago
|
||
Comment on attachment 728972 [details] [diff] [review] Fix r=mats
Attachment #728972 -
Flags: review?(matspal) → review+
Assignee | ||
Comment 7•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd563ebad556
Comment 8•11 years ago
|
||
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.
Description
•