40.54 KB, application/vnd.mozilla.xul+xml
1.69 KB, patch
|Details | Diff | Splinter Review|
5.24 KB, application/vnd.mozilla.xul+xml
1.79 KB, patch
|Details | Diff | Splinter Review|
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.
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?
scrollbar isn't anonymous for events in this case :(
Hmm, the testcases causes actually different stack, but anyhow, shows that mutation events are propagated.
Because of Bug 346957 and other tree fixes, EnsureView shouldn't be needed anymore, I think. It was added originally in Bug 123719.
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.
Unfortunately couldn't do this without one nsWeakFrame.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
The testcase in the bug causes a sg:critical crash (deleted nsTreeBodyFrame) on the 1.8 branch, though stack trace differs from comment 0.
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.
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?
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.
Attachment #270153 - Attachment description: testcase → testcase, may crash on load
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 on attachment 270155 [details] [diff] [review] for 1.8 Clearing a? because the patch is included in the patch for Bug 382444.
This is verified in Thunderbird 188.8.131.52 on PC (build 2007080918). The second test case crashes on the Mac in 184.108.40.206 though. I've opened bug 393321 to track this after talking to Dveditz.
And verified that it still crashes in Thunderbird 220.127.116.11.
Whiteboard: [sg:critical?] → [sg:critical?] second testcase private until 393321 fixed
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
You need to log in before you can comment on or make changes to this bug.