Closed
Bug 1160436
Opened 9 years ago
Closed 9 years ago
Fix PLDHashTable::operator=
Categories
(Core :: XPCOM, defect)
Core
XPCOM
Tracking
()
RESOLVED
FIXED
mozilla40
Tracking | Status | |
---|---|---|
firefox40 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
Details
Attachments
(1 file, 1 obsolete file)
7.06 KB,
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
As per bug 1149888 comment 10, PLDHashTable::operator= has some problems.
Assignee | ||
Comment 1•9 years ago
|
||
This fixes the following problems with PLDHashTable::operator=: - It doesn't handle self-assigments. - It leaks the memory used by the assigned-to table. - It doesn't leave the assigned-from table in a safely destructable state. It also adds some test code for operator=.
Attachment #8600232 -
Flags: review?(nfroyd)
Assignee | ||
Comment 2•9 years ago
|
||
Here's Valgrind output for the new test code when the old operator= function is used with it. The first two complaints are due to problem 3 from comment 1, and the leak is due to problem 2. > ==6116== Invalid read of size 4 > ==6116== at 0x4151CF: Finish (pldhash.cpp:317) > ==6116== by 0x4151CF: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337) > ==6116== by 0x40479F: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:148) > ==6116== by 0x4047DC: main (TestPLDHash.cpp:224) > ==6116== Address 0x139feb10 is 0 bytes inside a block of size 128 free'd > ==6116== at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==6116== by 0x4151FC: Finish (pldhash.cpp:330) > ==6116== by 0x4151FC: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337) > ==6116== by 0x404797: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:147) > 6116== by 0x4047DC: main (TestPLDHash.cpp:224) > ==6116== > ==6116== Invalid free() / delete / delete[] / realloc() > ==6116== at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==6116== by 0x4151FC: Finish (pldhash.cpp:330) > ==6116== by 0x4151FC: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337) > ==6116== by 0x40479F: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:148) > ==6116== by 0x4047DC: main (TestPLDHash.cpp:224) > ==6116== Address 0x139feb10 is 0 bytes inside a block of size 128 free'd > ==6116== at 0x4C2BE10: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==6116== by 0x4151FC: Finish (pldhash.cpp:330) > ==6116== by 0x4151FC: PL_DHashTableFinish(PLDHashTable*) (pldhash.cpp:337) > ==6116== by 0x404797: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:147) > ==6116== by 0x4047DC: main (TestPLDHash.cpp:224) > ==6116== > test_pldhash_assignment_operator : SUCCESS > ==6116== > ==6116== HEAP SUMMARY: > ==6116== in use at exit: 59,928 bytes in 448 blocks > ==6116== total heap usage: 999 allocs, 552 frees, 97,057 bytes allocated > ==6116== > ==6116== 128 bytes in 1 blocks are definitely lost in loss record 396 of 441 > ==6116== at 0x4C2ABA0: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so) > ==6116== by 0x415C19: Add (pldhash.cpp:574) > ==6116== by 0x415C19: Add (pldhash.cpp:645) > ==6116== by 0x415C19: PL_DHashTableAdd(PLDHashTable*, void const*) (pldhash.cpp:710) > ==6116== by 0x4046EC: TestPLDHash::test_pldhash_move_constructor() (TestPLDHash.cpp:139) > ==6116== by 0x4047DC: main (TestPLDHash.cpp:224) The new code triggers no Valgrind errors.
Assignee | ||
Comment 3•9 years ago
|
||
Whoa, try server is unhappy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa7564e15f8 I'll investigate on Monday.
Comment 4•9 years ago
|
||
Comment on attachment 8600232 [details] [diff] [review] Fix PLDHashTable::operator= Review of attachment 8600232 [details] [diff] [review]: ----------------------------------------------------------------- Since try is so red, I'm going to cancel this review; I don't see what the immediate issue is, but... ::: xpcom/glue/pldhash.cpp @@ +271,5 @@ > + using mozilla::Move; > + > + if (this != &aOther) { > + // Destruct |this|. > + Finish(); It looks like something is most unhappy with this Finish() call.
Attachment #8600232 -
Flags: review?(nfroyd)
Assignee | ||
Comment 5•9 years ago
|
||
I added an initializer list to the move constructor, which was necessary so that the call to the assignment operator wasn't looking at uninitialized values. And I expanded the test code to cover this case. I also made Finish() ok to call even if Init() hasn't been called -- this will be needed when I add a destructor, anyway.
Attachment #8600735 -
Flags: review?(nfroyd)
Assignee | ||
Updated•9 years ago
|
Attachment #8600232 -
Attachment is obsolete: true
Assignee | ||
Comment 6•9 years ago
|
||
Try server is much happier now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe3772537d91
Assignee | ||
Updated•9 years ago
|
Blocks: modernize-pldhash
Comment 7•9 years ago
|
||
Comment on attachment 8600735 [details] [diff] [review] Fix PLDHashTable::operator= Review of attachment 8600735 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for fixing this up. ::: xpcom/glue/pldhash.cpp @@ +267,5 @@ > } > > +PLDHashTable& PLDHashTable::operator=(PLDHashTable&& aOther) > +{ > + using mozilla::Move; Nit: this can go away, since we're already |using namespace mozilla;| in this file. @@ +269,5 @@ > +PLDHashTable& PLDHashTable::operator=(PLDHashTable&& aOther) > +{ > + using mozilla::Move; > + > + if (this != &aOther) { Do we really need to handle self-assignment here? I know it's Good C++ Practice and all, but there's a ton of code in the tree that doesn't handle this. Also, I think we should write this as: if (this == &aOther) { return *this; } and then proceed; handling exceptional cases first and all that.
Attachment #8600735 -
Flags: review?(nfroyd) → review+
Assignee | ||
Comment 8•9 years ago
|
||
> Do we really need to handle self-assignment here? I know it's Good C++ > Practice and all, but there's a ton of code in the tree that doesn't handle > this. I'll favour correctness over consistency with the existing code! :) > Also, I think we should write this as: > > if (this == &aOther) { > return *this; > } > > and then proceed; handling exceptional cases first and all that. Good idea.
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/168f09fc229f
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in
before you can comment on or make changes to this bug.
Description
•