trees set attributes during reflow (nsTreeBodyFrame::UpdateScrollbars)

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
12 years ago
11 years ago

People

(Reporter: dbaron, Assigned: smaug)

Tracking

(Blocks 2 bugs, {fixed1.8.0.13, fixed1.8.1.5})

Trunk
x86
Linux
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.5 +
wanted1.8.1.x +
blocking1.8.0.13 +
wanted1.8.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:critical?] second testcase private until 393321 fixed)

Attachments

(4 attachments, 1 obsolete attachment)

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
Assignee

Comment 1

12 years ago
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?
Assignee

Comment 2

12 years ago
Posted file testcase
scrollbar isn't anonymous for events in this case :(
Assignee

Comment 3

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

Updated

12 years ago
Depends on: 381153
Assignee

Updated

12 years ago
Assignee: nobody → Olli.Pettay
Assignee

Updated

12 years ago
Blocks: 123719
Assignee

Comment 4

12 years ago
Posted 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)
Assignee

Comment 5

12 years ago
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)
Assignee

Comment 6

12 years ago
Unfortunately couldn't do this without one nsWeakFrame.
Attachment #266739 - Flags: review?(roc)
Assignee

Updated

12 years ago
Status: NEW → ASSIGNED
Assignee

Updated

12 years ago
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.
Flags: blocking1.8.1.5+
Flags: blocking1.8.0.13+
Whiteboard: [sg:critical?]
Assignee

Comment 8

12 years ago
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+
Assignee

Comment 10

12 years ago
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.
Assignee

Updated

12 years ago
Depends on: 382444
Assignee

Comment 11

12 years ago
Assignee

Updated

12 years ago
Attachment #270153 - Attachment description: testcase → testcase, may crash on load
Assignee

Comment 12

12 years ago
Posted 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?
Assignee

Updated

12 years ago
Attachment #270155 - Attachment is patch: true
Attachment #270155 - Attachment mime type: text/x-patch → text/plain
Assignee

Comment 13

12 years ago
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?
Assignee

Comment 14

12 years ago
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

Updated

11 years ago
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.