1160436 - Fix PLDHashTable::operator=

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Nicholas Nethercote [inactive]]

As per bug 1149888 comment 10, PLDHashTable::operator= has some problems.

Comment 1

[Nicholas Nethercote [inactive]]

Attachment: Fix PLDHashTable::operator=

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=.

Comment 2

[Nicholas Nethercote [inactive]]

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.

Comment 3

[Nicholas Nethercote [inactive]]

Whoa, try server is unhappy: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8fa7564e15f8

I'll investigate on Monday.

Comment 4

[Nathan Froyd [:froydnj]]

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.

Comment 5

[Nicholas Nethercote [inactive]]

Attachment: Fix PLDHashTable::operator=

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.

Comment 6

[Nicholas Nethercote [inactive]]

Try server is much happier now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fe3772537d91

Comment 7

[Nathan Froyd [:froydnj]]

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.

Comment 8

[Nicholas Nethercote [inactive]]

> 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 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/168f09fc229f

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/168f09fc229f