Closed
Bug 154751
Opened 22 years ago
Closed 21 years ago
ensure nsStyleContext::GetStyleData never returns null
Categories
(Core :: CSS Parsing and Computation, defect, P1)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla1.2beta
People
(Reporter: dbaron, Assigned: dbaron)
References
Details
(Whiteboard: [patch][whitebox])
Attachments
(1 file, 5 obsolete files)
61.01 KB,
patch
|
Details | Diff | Splinter Review |
I'd like to make certain that nsStyleContext::GetStyleData never returns null. I think it's better to do this than to scatter null checks around the tree, for at least two reasons: * code size * better error handling My patch will return a struct with the default values for the properties in an out-of-memory condition.
Assignee | ||
Comment 1•22 years ago
|
||
This doesn't quite compile yet, because that code can't really live in the style set -- it needs to be in rule node and it needs access to a pres context.
Assignee | ||
Updated•22 years ago
|
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1beta
Assignee | ||
Updated•22 years ago
|
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Assignee | ||
Comment 2•22 years ago
|
||
This compiles, but I haven't tested it yet. (My build isn't done, and I'm going home.) I added a dependency to the bug for putting nsStyleUtil back into content. However, this version of the patch (unlike what I had in my tree a few minutes ago) makes nsStyleStruct.cpp depend on nsStyleUtil.cpp. Then again, we could split nsStyleStruct.cpp in half and only put some of it in content/shared (leaving the constructors somewhere else).
Attachment #89524 -
Attachment is obsolete: true
Assignee | ||
Comment 3•22 years ago
|
||
Well, the patch seems to work... at least the browser starts up and things work. :-)
Whiteboard: [patch]
Assignee | ||
Comment 4•22 years ago
|
||
This is updated to the trunk and cleaned up a bit more.
Assignee | ||
Updated•22 years ago
|
Attachment #90918 -
Attachment is obsolete: true
Assignee | ||
Comment 5•22 years ago
|
||
merged with more recent changes to trunk
Attachment #95764 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Target Milestone: mozilla1.2alpha → mozilla1.2beta
Assignee | ||
Comment 6•22 years ago
|
||
Comment on attachment 98196 [details] [diff] [review] updated patch This still has a problem -- I need to figure out how to find a pres context so that I can call mDefaultStyleData.Destroy(0, presContext). (There might be some other things I've fixed in my tree, too.)
Attachment #98196 -
Flags: needs-work+
Assignee | ||
Updated•22 years ago
|
Priority: -- → P1
Comment 7•22 years ago
|
||
David - it would be nice if you could attach a testcase to this bug. It would be vital when QA will need to verify the bug and see the impact of this patch. Thx!
Whiteboard: [patch] → [patch][dev notes]
Assignee | ||
Comment 8•22 years ago
|
||
No. It's an issue about how we handle out-of-memory conditions.
Comment 9•22 years ago
|
||
ok... in this case, i'll add '[dev notes]' to the status whiteboard. (a way for QA to track bugs which do not require minimized testcase and highlight that this is going to be a whitebox level change) Thanks David !
Updated•22 years ago
|
Whiteboard: [patch][dev notes] → [patch][whitebox]
Assignee | ||
Comment 10•22 years ago
|
||
Also, fwiw, something in this patch breaks text zoom on certain things (especially form controls).
Assignee | ||
Comment 11•22 years ago
|
||
This fixes the text zoom bug and the leak, as well as cleaning up text zoom a little more. (I'd still like to move more stuff into nsStyleFont constructors, but that should be a separate patch.)
Attachment #98196 -
Attachment is obsolete: true
Assignee | ||
Updated•22 years ago
|
Attachment #113334 -
Flags: superreview?(bzbarsky)
Attachment #113334 -
Flags: review?(bzbarsky)
Comment 12•21 years ago
|
||
I'm going to try to get to this review within the next week....
Comment 13•21 years ago
|
||
Comment on attachment 113334 [details] [diff] [review] patch I assume all that font zoom stuff snuck in here illicitly? Other than that, looks pretty good, though I'm not quite done thinking through some parts of it....
Assignee | ||
Comment 14•21 years ago
|
||
No, the font zoom stuff was intentional. You get cleanup mixed in when I attach patches from my main tree.
Assignee | ||
Comment 15•21 years ago
|
||
No, actually, without the font zoom stuff, I couldn't have done the ctor_args addition to nsStyleStructList.h, which would have forced me to write things out in BuildDefaultStyleData. So there's even a reason it's mixed in.
Comment 16•21 years ago
|
||
OK, I see.
Comment 17•21 years ago
|
||
Request changing SEVERITY to CRITICAL because this blocks 195073 which is SEVERITY CRITICAL. Also: Target Milestone: 1.4a
Assignee | ||
Comment 18•21 years ago
|
||
That doesn't make sense, since bug 171830 is a real fix for that bug, whereas for this patch it's just a side effect (and perhaps harmful since it could hide other bugs, except that bug 171830 and the other similar changes will fix those other bugs).
Assignee | ||
Comment 19•21 years ago
|
||
Merged to trunk (after bryner's nsIStyleContext removal, bug 117316, and bug 171830).
Attachment #113334 -
Attachment is obsolete: true
Comment 20•21 years ago
|
||
Comment on attachment 113334 [details] [diff] [review] patch >Index: content/base/src/nsRuleNode.cpp >- PRBool zoom = PR_FALSE; > const nsFont* defaultFont; > if (!font) { >- mPresContext->GetDefaultFont(kPresContext_DefaultVariableFont_ID, &defaultFont); >- font = new (mPresContext) nsStyleFont(*defaultFont); >- zoom = PR_TRUE; >+ font = new (mPresContext) nsStyleFont(mPresContext); Could we move defaultFont down to where it's actually used? (~60 lines below) >Index: content/base/src/nsStyleSet.cpp >+PRBool StyleSetImpl::BuildDefaultStyleData(nsIPresContext* aPresContext) >+{ What happens if this gets called twice without shutting down in between? Eg EnsureRuleWalker fails after we BuildDefaultStyleDate when trying to nsRuleNode::CreateRootNode (out of memory, eg). Then the next time we EnsureRuleWalker we call BuildDefaultStyleData again and leak, no? We should probably Destroy() the mDefaultStyleData in EnsureRuleWalker if one of those other two things fail... Other than those two nits, looks great!
Attachment #113334 -
Flags: superreview?(bzbarsky)
Attachment #113334 -
Flags: superreview+
Attachment #113334 -
Flags: review?(bzbarsky)
Attachment #113334 -
Flags: review+
Assignee | ||
Comment 21•21 years ago
|
||
Fix checked in to trunk, 2003-03-13 07:29 PST. Filed bug 197205 on removing null-checks from callers.
You need to log in
before you can comment on or make changes to this bug.
Description
•