[ASDisplayNode] Fix infinite layout loop

[nguyenhuy]

The problem was introduced by #428. It will occur if a node does either of the followings:

1. Keeps using a stale pending layout over a calculated layout
2. Doesn't update the next layout's version after calling _setNeedsLayoutFromAbove.

[ghost]

🚫 CI failed with log

[Adlai-Holler]

Sigh it seems that the fundamental problem here is that we call [self setNeedsLayout] inside of -_setNeedsLayoutFromAbove only so that the node participates in the next layout pass, and not to indicate that the node's layout data is invalid.

We should try to make this distinction clearer as we go.

I don't like the idea of overwriting the version of a given layout. It means we could show old layout data unwittingly. Since we're holding the lock (shouldn't we call _locked_setNeedsLayoutFromAbove esp since we call into the supernode?), this is probably not a practical issue though.

However, I think there's way to fix this without overwriting the version: Replace the [self setNeedsLayout] with [self.layer setNeedsLayout]. Does that seem reasonable?

[nguyenhuy]

@Adlai-Holler Yes, it indeed sounds like a cleaner approach. However, there are a couple of problems with it:

• Calling -setNeedsLayout on the layer is only true for the node that originates the call. Since _setNeedsLayoutFromAbove is recursive, all other higher up nodes need to invalidate their layouts, so [self setNeedsLayout] is needed. This can be solved by passing the original node as a parameter and compare against self.
• Another bigger problem is that, some classes override -setNeedsLayout to add custom logics. Metadata node for example needs to reset its animation flags as soon as the current layout is invalidated. Not calling [self setNeedsLayout] in this case is an API breaking change.

[appleguy]

Should we revert the layoutVersion change for now? It's definitely necessary (currently) to call setNeedsLayout while ascending the tree, so we should be careful to align any changes with the current expectations of the code.

[appleguy]

Does this effectively replace the line I commented on here? https://github.com/TextureGroup/Texture/pull/428/files#r126296449

If so, we should probably replace the line at that same location. I think it should not have been removed originally.

[nguyenhuy]

Yes, more or less. The main difference is that here only stale or inapplicable pending layouts are cleared, so it can happen that at a given time, both calculated and pending layouts point to a valid layout (and thus can be interchangeable). With the other original line, all pending layouts are cleared, including valid/applied ones.

[Adlai-Holler]

Why is this line necessary @nguyenhuy ? EDIT: The pending layout is still valid, it's just not valid at our current constrained size. Why is the layoutVersion + currentConstrainedSize not sufficient to tell us whether a given layout is valid?

[nguyenhuy]

Either of this line or the new layout version check above fixes part of the problem. The reason I included it is that at this point, the pending layout can be stale (here) and it's never going to be applied. And yet, it still holds onto orphaned subnodes that either will never be inserted, or were/soon-will-be removed by newer layouts (check out -[ASLayout retainSublayoutLayoutElements]).

Edit: the version correction below is still absolutely needed, or we need to rethink the whole _setNeedsLayoutFromAbove strategy.

[nguyenhuy]

It is true that in case the pending layout can't be applied because of a bounds mismatch, we can keep it around. I'm fine with adding another flag here to handle that edge.

However, as said above, in case _pendingDisplayNodeLayout->version < _layoutVersion, it's quite important to get rid of it.

[Adlai-Holler]

Sorry I must be missing something big. Why is it important to get rid of it? What's the benefit?

EDIT: Sure in terms of general hygiene we should clear out layouts to reclaim resources when we notice they're out-of-date, but why here?

[Adlai-Holler]

Oh there's a comment between those two that I missed. OK that makes sense, to release those nodes. We should have a common pattern that in one step checks if a given layout is valid, and discards it if not, and use that everywhere, but yes I'm on the same page about this line now.

[nguyenhuy]

👍

[Adlai-Holler]

Medium/long term we should do what Huy suggested, and only (re) call [self setNeedsLayout] for nodes above the one whose layout we're trying to generate. Put harshly, this diff is very hacky – it basically spoofs the freshness of data after calling out to the client. If the client were to change layout params between us generating the layout and overwriting the version, the layout may show version=4 but have been generated based on version=2 data, and we'll show stale data. Not likely a practical problem, but for safety we should strive to capture the version before generating a layout and never after.

In the near term, we can land this as-is so that folks are unblocked.

[nguyenhuy]

@Adlai-Holler Agreed!

I reckon the long-term solution would be to always layout from root.

[nguyenhuy]

I'm gonna merge this PR and run an internal test. If we encounter another problem, I think we should really consider to revert the original PR as @appleguy suggested. Thanks everyone!