381120 - trees set attributes during reflow (nsTreeBodyFrame::UpdateScrollbars)

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The tree code sets attributes during reflow, which is bad:

nsIContent::SetAttr(int, nsIAtom*, nsAString_internal const&, int) (/builds/trunk/obj/firefox-debugopt/layout/xul/base/src/tree/src/../../../../../../dist/include/content/nsIContent.h:249)
nsTreeBodyFrame::UpdateScrollbars(nsTreeBodyFrame::ScrollParts const&) (/builds/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:830)
nsTreeBodyFrame::ScrollToRowInternal(nsTreeBodyFrame::ScrollParts const&, int) (/builds/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:3839)
nsTreeBodyFrame::ScrollToRow(int) (/builds/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:3818)
nsTreeBodyFrame::EnsureView() (/builds/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:405)
nsTreeBodyFrame::GetMinSize(nsBoxLayoutState&) (/builds/trunk/mozilla/layout/xul/base/src/tree/src/nsTreeBodyFrame.cpp:232)

Now that style reresolution is asynchronous, this isn't as bad as it used to be (or do we implement mutation events that can pick up attribute changes?).  But we still should be doing this asynchronously.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

We do implement DOMAttributeModified, but mutation events aren't 
propagated from native anonymous content. Usually scrollbars are 
native-anon, is that the case also with trees?

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase

scrollbar isn't anonymous for events in this case :(

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Hmm, the testcases causes actually different stack, but anyhow, shows that mutation events are propagated.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Remove EnsureView call

Because of Bug 346957 and other tree fixes, EnsureView shouldn't be
needed anymore, I think. It was added originally in Bug 123719.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 266736 [details] [diff] [review]
Remove EnsureView call

Hmm, no this is not good enough. EnsureView must be called somewhere, but just not during reflow.
Maybe in the reflowcallback.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Postpone EnsureView to ReflowFinished when called during Reflow

Unfortunately couldn't do this without one nsWeakFrame.

Comment 7

[Daniel Veditz [:dveditz]]

The testcase in the bug causes a sg:critical crash (deleted nsTreeBodyFrame) on the 1.8 branch, though stack trace differs from comment 0.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

I don't get the crash on 1.8. Bug 381153 should have fixed that.
Though, probably a bit different kind of testcase does crash because 
attachment 266739 [details] [diff] [review] isn't in the branch.

Comment 9

[Daniel Veditz [:dveditz]]

Are you saying we don't need this on the branch since we've fixed bug 381153, or that we do, we just need to adjust the testcase?

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Need to adjust the testcase. Setting an attribute while reflowing is always bad. I'll modify the patch a bit for 1.8 and try to write a new
testcase.

Comment 11

[Olli Pettay [:smaug][[e-mail]]]

Attachment: testcase, may crash on load

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8

The only change to the trunk patch is PresContext() -> GetPresContext().
This doesn't fix the crash, but moves it out of the reflow (which is 
needed). Bug 382444 will fix the crash properly.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 270155 [details] [diff] [review]
for 1.8

Clearing a? because the patch is included in the patch for Bug 382444.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Checked in Bug 382444 to branches.

Comment 15

[Al Billings [:abillings - ex-MoCo]]

This is verified in Thunderbird 1.5.0.13 on PC (build 2007080918). 

The second test case crashes on the Mac in 2.0.0.6 though. I've opened bug 393321 to track this after talking to Dveditz.

Comment 16

[Al Billings [:abillings - ex-MoCo]]

And verified that it still crashes in Thunderbird 1.5.0.12.