herzbube / littlego

Little Go. An iOS application that lets you play the game of Go on the iPhone or iPad.
https://littlego.herzbube.ch/
Apache License 2.0
139 stars 54 forks source link

GoMove dealloc removes references in other GoMove objects that are != self #369

Closed herzbube closed 2 years ago

herzbube commented 2 years ago

The dealloc method of GoMove is responsible for removing references to self in the previous/next GoMove objects. The current implementation does this, however, without checking if the previous/next GoMove objects actually point to self. The implementation silently assumes that this must be the case, but as it now turns out this assumption is only true if the timing of deallocation is correct.

Looking at this schema illustrates the issue:

The user plays two moves.

  +---next------+
  |             v
move1         move2a
  ^             |
  +---previous--+

The user discards move2a, but the GoMove object is not immediately deallocated.
The user plays move2b that replaces move2a.
move2a still has the previous reference to move1.

  +---next------+
  |             v
move1         move2b     move2a
  ^             |          |
  +---previous--+          |
  ^                        |
  +---previous-------------+

The system finally deallocates move2a.
The current implementation of dealloc destroys the next reference in move1.

move1         move2b
  ^             |
  +---previous--+

Generally speaking, deallocation of a GoMove object can be delayed by two things:

The issue was found during unit testing, but it cannot be ruled out that this might also exist in production. And if not now, then in the future when some unrelated implementation detail changes.