Closed Bug 381120 Opened 13 years ago Closed 13 years ago

trees set attributes during reflow (nsTreeBodyFrame::UpdateScrollbars)

Categories

(Core :: XUL, defect)

x86
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: dbaron, Assigned: smaug)

References

(Blocks 2 open bugs)

Details

(Keywords: fixed1.8.0.13, fixed1.8.1.5, Whiteboard: [sg:critical?] second testcase private until 393321 fixed)

Attachments

(4 files, 1 obsolete file)

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.
Blocks: 335053, 335054
No longer blocks: 355053, 355054
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?
Attached file testcase
scrollbar isn't anonymous for events in this case :(
Hmm, the testcases causes actually different stack, but anyhow, shows that mutation events are propagated.
Depends on: 381153
Assignee: nobody → Olli.Pettay
Blocks: 123719
Attached patch Remove EnsureView call (obsolete) — Splinter Review
Because of Bug 346957 and other tree fixes, EnsureView shouldn't be
needed anymore, I think. It was added originally in Bug 123719.
Attachment #266736 - Flags: review?(neil)
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.
Attachment #266736 - Attachment is obsolete: true
Attachment #266736 - Flags: review?(neil)
Unfortunately couldn't do this without one nsWeakFrame.
Attachment #266739 - Flags: review?(roc)
Status: NEW → ASSIGNED
Status: ASSIGNED → RESOLVED
Closed: 13 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.
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Whiteboard: [sg:critical?]
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?
Flags: wanted1.8.1.x+
Flags: wanted1.8.0.x+
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.
Depends on: 382444
Attachment #270153 - Attachment description: testcase → testcase, may crash on load
Attached patch for 1.8Splinter Review
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.
Attachment #270155 - Flags: approval1.8.1.5?
Attachment #270155 - Flags: approval1.8.0.13?
Attachment #270155 - Attachment is patch: true
Attachment #270155 - Attachment mime type: text/x-patch → text/plain
Comment on attachment 270155 [details] [diff] [review]
for 1.8

Clearing a? because the patch is included in the patch for Bug 382444.
Attachment #270155 - Flags: approval1.8.1.5?
Attachment #270155 - Flags: approval1.8.0.13?
Checked in Bug 382444 to branches.
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.
And verified that it still crashes in Thunderbird 1.5.0.12.
Depends on: 393321
Attachment #270153 - Attachment is private: true
Group: security
Whiteboard: [sg:critical?] → [sg:critical?] second testcase private until 393321 fixed
Depends on: 430394
Component: XP Toolkit/Widgets: Trees → XUL
QA Contact: xptoolkit.trees → xptoolkit.widgets
Attachment #270153 - Attachment is private: false
You need to log in before you can comment on or make changes to this bug.