kojii@, thank you for filing an issue, but let me confirm the issue.

UpdateDistribution() is a pre-requirement of updating the layout tree.UpdateDistribution, but UpdatDistribution() itself doesn't modify the layout tree.

I guess the situation is slightly different. Updating a distribution is cheaper than updating LayoutTree.

Here's a sample stack. ...or is it a bug for UpdateDistribution() to call DetachLayoutTree()?

#1 0xb88123c in blink::LayoutObject::DestroyAndCleanupAnonymousWrappers()
#2 0xa710876 in blink::Node::DetachLayoutTree(blink::Node::AttachContext const&)
#3 0xa8ace03 in LazyReattachIfAttached
#4 0xa8ace03 in blink::InsertionPoint::SetDistributedNodes(blink::DistributedNodes&)
#5 0xa89a441 in blink::DistributionPool::DistributeTo(blink::InsertionPoint*, blink::ElementShadowV0*)
#6 0xa89c70d in blink::ElementShadowV0::Distribute()
#7 0xa70c192 in DistributeIfNeeded
#8 0xa70c192 in blink::Node::RecalcDistribution()
#9 0xa70c647 in blink::Node::RecalcDistribution()
#10 0xa70c647 in blink::Node::RecalcDistribution()
#11 0xa70c647 in blink::Node::RecalcDistribution()
#12 0xa70c647 in blink::Node::RecalcDistribution()
#13 0xa709396 in blink::Node::UpdateDistribution()

Yes, but I am still confused.

Calling UpdateDistribution() twice in a row doesn't have any effect. The 2nd call is almost no-op in most cases (if the node is connected).
DetachLayoutTree() should be done sooner or later in creating layout tree.

Could you give us an example of the call sites which should be bad? That would help us to understand the issue.

This is from  issue 726716 , more stack:

#13 0xa709396 in blink::Node::UpdateDistribution()
#14 0xc503114 in blink::AXObjectImpl::AccessibilityIsIgnored() third_party/WebKit/Source/modules/accessibility/AXObjectImpl.cpp:612:11
#15 0xc537783 in blink::AXObjectCacheImpl::GetOrCreate(blink::LayoutObject*) third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:401:48
#16 0xc53c663 in blink::AXObjectCacheImpl::TextChanged(blink::LayoutObject*) third_party/WebKit/Source/modules/accessibility/AXObjectCacheImpl.cpp:588:15
#17 0xb971a1e in blink::LayoutText::SetText(WTF::PassRefPtr<WTF::StringImpl>, bool) third_party/WebKit/Source/core/layout/LayoutText.cpp:1688:12
#18 0xb96db3c in blink::LayoutText::SetTextWithOffset(WTF::PassRefPtr<WTF::StringImpl>, unsigned int, unsigned int, bool) third_party/WebKit/Source/core/layout/LayoutText.cpp:1559:3
#19 0xa81bc63 in blink::Text::UpdateTextLayoutObject(unsigned int, unsigned int) third_party/WebKit/Source/core/dom/Text.cpp:468:23
#20 0xa4c77cc in blink::CharacterData::SetDataAndUpdate(WTF::String const&, unsigned int, unsigned int, unsigned int, blink::CharacterData::UpdateSource) third_party/WebKit/Source/core/dom/CharacterData.cpp:184:19
#21 0xa4c954f in blink::CharacterData::appendData(WTF::String const&)

So we detach LayoutText while it was called from the LayoutText.

Thanks. I think I am getting to understand the issue. It looks the issue happened as follows:

1. We got a reference to a LayoutText, (A)
2. Some DOM mutation happened after 1. That means (A) should not be used anymore because it no longer reflects the latest status of layout tree.
3. UpdateDistribution() is called, which discarded (A) surely. That is the correct behavior, I think.
4. We touched (A). That caused Heap-use-after-free.

I think calling UpdateDistribution() is not wrong here. Rather, it detects the bad usage of (A), which is no longer valid.

What should be fixed is that we touched (A) even though it is no longer valid, and we can't tell it easily.

Thank you Hayato for the analysis. UpdateDistribution() to change the layout tree being by design match to my understanding.

Then we're back to the original point that it should never be called from within layout code; the same criteria as UpdateStyleAndLayout() should apply. I understand it's much lighter than UpdateStyleAndLayout() so the perf isn't a concern here, but it looks like there's the same type of risk in UpdateDistribution().

dmazzoni@ is working on the found case in issue 699438.

+yosin, in case editing calls UpdateDistribution()?

editing calls UpdateDistribution() in
 - ToPositionInFlatTree()
 - ComparePositions(PositionInFlatTree)

These functions should be called after LayoutClean. But, some callers may not.

Thank you yosin@, that is great.

I might have missed something though. Can you explain a bit more?

When the tree is layout clean, distribution is also clean, correct? In other words, if distribution is dirty and you call UpdateDistribution(), layout is no longer clean. As you can see in #3 stack, UpdateDistribution() calls LazyReattachIfAttached().

So:
  // Here we're layout clean. Safe to work on LayoutObject.
  LayoutObject* something = some_node->GetLayoutObject();
  another_node->GetDocument().UpdateDistribution();
  // here the layout tree maybe dirty, and the |something| maybe destroyed.
  // You must call UpdateStyleAndLayout() to get layout clean again,
  another_node->GetDocument().UpdateStyleAndLayout();
  // and re-retrieve LayoutObjects.
  something = some_node->GetLayoutObject(); // maybe different from the first one

This is almost analogous to:
  LayoutObject* something = some_node->GetLayoutObject();
  another_node->GetDocument().UpdateStyleAndLayout();
  // here the |something| maybe destroyed.
  // Re-retrieve LayoutObjects.
  something = some_node->GetLayoutObject(); // maybe different from the first one

UpdateDistribution() is called in kInStyleRecalc before actual style resolution
started. When UpdateDistribution() changes style or layout, it sets dirty bit
and rollback DocumentLifecycle to kVisualUpdatePending.

After calling UpdateDistribution(), we should make sure LayoutClean before accessing LayoutObject.

In theory, every LayoutObject should be done LayoutClean.
Editing code should follow this rule. ;-)

Editing has two call sites:
- ToPositionInFlatTree()
- ComparePositionInFlatTree()