Closed Bug 1415637 Opened 2 years ago Closed 2 years ago

Possibly incorrect MOZ_MAKE_MEM_UNDEFINED in nsTHashtable copy constructor

Categories

(Core :: XPCOM, enhancement)

enhancement
Not set

Tracking

()

RESOLVED FIXED
mozilla58
Tracking Status
firefox58 --- fixed

People

(Reporter: jseward, Assigned: jseward)

Details

Attachments

(2 files)

Starting Fx on Valgrind, letting it load theregister.co.uk and letting it
fall idle produces the somewhat strange uninit-value-use warnings shown in
the attachment.  The origin of them is a MOZ_MAKE_MEM_UNDEFINED annotation in

template<class EntryType>
nsTHashtable<EntryType>::nsTHashtable(nsTHashtable<EntryType>&& aOther).
Attached file Valgrind complaints
Julian and I have talked about this twice now.  The relevant complaint from Valgrind is:

==26104== Thread 16 Socket Thread:
==26104== Conditional jump or move depends on uninitialised value(s)
==26104==    at 0x10A41EEA: PLDHashTable::~PLDHashTable() (MC-TALL/xpcom/ds/PLDHashTable.cpp:315)
==26104==    by 0x10A49168: nsTHashtable<nsBaseHashtableET<nsStringHashKey, nsCOMPtr<nsIVariant> > >::~nsTHashtable() (MC-TALL/xpcom/ds/nsTHashtable.h:391)
==26104==    by 0x10A4629D: ~nsBaseHashtable (MC-TALL/xpcom/ds/nsBaseHashtable.h:52)
==26104==    by 0x10A4629D: ~nsInterfaceHashtable (MC-TALL/xpcom/ds/nsInterfaceHashtable.h:23)
==26104==    by 0x10A4629D: ~nsHashPropertyBagBase (MC-TALL/xpcom/ds/nsHashPropertyBag.h:17)
==26104==    by 0x10A4629D: nsHashPropertyBag::~nsHashPropertyBag() (MC-TALL/xpcom/ds/nsHashPropertyBag.cpp:284)
==26104==    by 0x10DEA859: mozilla::net::HttpBaseChannel::~HttpBaseChannel() (MC-TALL/netwerk/protocol/http/HttpBaseChannel.cpp:233)
==26104==    by 0x10E1C217: mozilla::net::HttpChannelChild::~HttpChannelChild() (MC-TALL/netwerk/protocol/http/HttpChannelChild.cpp:211)
==26104==    by 0x10E1C302: mozilla::net::HttpChannelChild::~HttpChannelChild() (MC-TALL/netwerk/protocol/http/HttpChannelChild.cpp:216)
==26104==    by 0x10E1C38B: mozilla::net::HttpChannelChild::Release() (MC-TALL/netwerk/protocol/http/HttpChannelChild.cpp:266)
==26104==    by 0x10DE9899: Release (MC-TALL/gcc-Og-nondebug-stylo-systemalloc/dist/include/mozilla/RefPtr.h:41)
==26104==    by 0x10DE9899: Release (MC-TALL/gcc-Og-nondebug-stylo-systemalloc/dist/include/mozilla/RefPtr.h:398)
==26104==    by 0x10DE9899: ~RefPtr (MC-TALL/gcc-Og-nondebug-stylo-systemalloc/dist/include/mozilla/RefPtr.h:79)
==26104==    by 0x10DE9899: mozilla::net::HttpBackgroundChannelChild::ActorDestroy(mozilla::ipc::IProtocol::ActorDestroyReason) (MC-TALL/netwerk/protocol/http/HttpBackgroundChannelChild.cpp:410)
==26104==    by 0x110744B1: mozilla::net::PHttpBackgroundChannelChild::DestroySubtree(mozilla::ipc::IProtocol::ActorDestroyReason) (MC-TALL/gcc-Og-nondebug-stylo-systemalloc/ipc/ipdl/PHttpBackgroundChannelChild.cpp:417)
==26104==    by 0x1107565F: mozilla::net::PHttpBackgroundChannelChild::OnMessageReceived(IPC::Message const&) (MC-TALL/gcc-Og-nondebug-stylo-systemalloc/ipc/ipdl/PHttpBackgroundChannelChild.cpp:370)
==26104==    by 0x111A9A6B: mozilla::ipc::PBackgroundChild::OnMessageReceived(IPC::Message const&) (MC-TALL/gcc-Og-nondebug-stylo-systemalloc/ipc/ipdl/PBackgroundChild.cpp:1815)
==26104==    by 0x10F6CC3E: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (MC-TALL/ipc/glue/MessageChannel.cpp:2119)

In ~PLDHashTable, we're checking whether we've allocated mEntryStore yet, to determine if anything needs to be done.

The relevant code in nsTHashtable looks like:

template<class EntryType>
nsTHashtable<EntryType>::nsTHashtable(nsTHashtable<EntryType>&& aOther)
  : mTable(mozilla::Move(aOther.mTable))
{
  // aOther shouldn't touch mTable after this, because we've stolen the table's
  // pointers but not overwitten them.
  MOZ_MAKE_MEM_UNDEFINED(&aOther.mTable, sizeof(aOther.mTable));
}

My position is that this is a bogus annotation.  The comment is wrong on multiple fronts:

1. aOther.mTable is still touchable memory, because the destructor has to examine the internals of aOther.mTable to determine if anything needs to be done.  Whoever was holding aOther shouldn't be touching aOther, but that's not something we can enforce with Valgrind; it'd have to be done with a use-after-move static analysis or similar.

2. We have overwritten aOther.mTable's pointers (see PLDHashTable(PLDHashTable&&))--we had to overwrite them with nulls so two different hashtable objects wouldn't be pointing at the same memory.

WDYT, njn?
...and bugzilla mid-airs resolve incorrectly once again.  Please see comment 2.
Flags: needinfo?(n.nethercote)
Comment 2 makes sense to me. I do wonder why we haven't hit this before, though.
Flags: needinfo?(n.nethercote)
(In reply to Nicholas Nethercote [:njn] from comment #5)
> I do wonder why we haven't hit this before, though.

I don't know.  I can say that I only recall seeing this within the
past two months or so; not before that.
Attachment #8926521 - Flags: review?(nfroyd)
Attachment #8926521 - Flags: review?(nfroyd) → review+
Pushed by jseward@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c69cfd07577
Possibly incorrect MOZ_MAKE_MEM_UNDEFINED in nsTHashtable copy constructor.  r=froydnj.
https://hg.mozilla.org/mozilla-central/rev/4c69cfd07577
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
jseward: there's no need for the commit message to match the bug title. It's better to describe what the patch does, e.g. "Remove incorrect MOZ_MAKE_MEM_UNDEFINED in nsTHashtable's move constructor". Extra detail beyond the first line is also welcome, when appropriate.
Assignee: nobody → jseward
You need to log in before you can comment on or make changes to this bug.