[ASDisplayNode] cancelAsyncDisplay must clear dirty contents of layer

[garrettmoon]

From @molon on November 26, 2015 13:20

Hi,
There's a ASTextNode in a ASLazyLoadCellNode.
I try to set attributedString to it at the end of table view.
Then I insert new rows.

This is just the happen scene upon.

debug below:
some test I added:

break at the function

the output, we can see it breaks before the last isCancelledBlock

We can see that the willMoveToWindow:(window is nil) is called because of inserting rows.
it cause cancelAsyncDisplay to be called. This led to dirty contents of layer.

Then the last isCancelledBlock will return YES. The last draw attributedString will be ignored. So it layer.content is stale(dirty) now.

__enterHierarchy will be called when willMoveToWindow(window is not nil).
setNeedsDisplay is called in __enterHierarchy when layer.content == nil.
But the layer.content==nil can only ensure the layer will be redraw when nil, not stale(dirty).
And It's really dirty sometimes.
eg:
updateVisibleNodeIndexPaths will lead to recursivelyClearContents with some indexPaths. it's ok.
But [_layoutController indexPathsForScrolling:_scrollDirection viewportSize:viewportSize rangeType:rangeType] will return indexPaths which not visible sometimes, thus these rows will be remove from window by configureContentView:forCellNode: but havn't clear content.
So the rows will not be redisplay. I think it's a BUG.

In summary, the cancelAsyncDisplay only cancel the next drawing of layer, but! but! not clear the dirty contents of layer. So it will be not redraw because it's contents not nil ,when the layer display again,

I think views is not necessary to call cancelAsyncDisplay after removing from a window unless we can throw the dirty contents away.
It will cause some unforeseen circumstances.

:)

Copied from original issue: facebookarchive/AsyncDisplayKit#889

[garrettmoon]

From @appleguy on November 28, 2015 2:51

@molon thank you for the detailed description! I understand most of what you've discovered, but in my first read-through, don't yet understand as much as you do. I may be able to fully understand when I can look into the code alongside reading your description, but since I'm on a plane that is about to push back from the gate, I'll start with a question for you!

Question: are you saying that in this situation, the display is cancelled /but no replacement display is scheduled, and it should be scheduled/?

Do you believe that it should be cancelled and then re-displayed, or do you believe that it is always safe to avoid cancelling in this situation entirely and no re-display is needed?

I'm really glad you dug into the implementation here, as these days I am one of the only folks looking at this particular code path :).

[garrettmoon]

From @molon on November 28, 2015 5:1

@appleguy thanks for answer.

Question 1:
Yes. if it's layer.content is nil, it would be re-display. But sometimes it's layer.content is stale not nil.
a stale layer.content must follows scheduled display.

Question 2:
I believe that (avoid cancelling when node __exitHierarchy) is good choice.
layer.content should be always fresh or nil when no scheduled display.

I'm glad to learn so much from the library. :)

[garrettmoon]

@appleguy do you think this is still an issue?

[garrettmoon]

From @appleguy on November 28, 2016 1:40

Pulling this into 2.0.1 so it can be evaluated in more detail and then triaged to the right milestone if it can't be fixed immediately.

[garrettmoon]

From @appleguy on November 28, 2016 1:40

@Adlai-Holler has looked at the async display system a good bit (transactions), so he could coordinate with @nguyenhuy to decide which one of them has cycles post-2.0 to investigate.