ensure nsStyleContext::GetStyleData never returns null

RESOLVED FIXED in mozilla1.2beta

Status

()

P1
normal
RESOLVED FIXED
17 years ago
16 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

Trunk
mozilla1.2beta
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch][whitebox])

Attachments

(1 attachment, 5 obsolete attachments)

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.
Created attachment 89524 [details] [diff] [review]
partial patch

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.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla1.1beta
Target Milestone: mozilla1.1beta → mozilla1.2alpha
Created attachment 90918 [details] [diff] [review]
patch, v. 2.0

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
Well, the patch seems to work... at least the browser starts up and things work. :-)
Whiteboard: [patch]
Created attachment 95764 [details] [diff] [review]
updated patch

This is updated to the trunk and cleaned up a bit more.
Attachment #90918 - Attachment is obsolete: true
Created attachment 98196 [details] [diff] [review]
updated patch

merged with more recent changes to trunk
Attachment #95764 - Attachment is obsolete: true
Target Milestone: mozilla1.2alpha → mozilla1.2beta
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+
Priority: -- → P1

Comment 7

16 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]
No.  It's an issue about how we handle out-of-memory conditions.

Comment 9

16 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

16 years ago
Whiteboard: [patch][dev notes] → [patch][whitebox]
Also, fwiw, something in this patch breaks text zoom on certain things
(especially form controls).
Created attachment 113334 [details] [diff] [review]
patch

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
Attachment #113334 - Flags: superreview?(bzbarsky)
Attachment #113334 - Flags: review?(bzbarsky)
I'm going to try to get to this review within the next week....
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....
No, the font zoom stuff was intentional.  You get cleanup mixed in when I attach
patches from my main tree.
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 17

16 years ago
Request changing SEVERITY to CRITICAL because this blocks 195073 which is
SEVERITY CRITICAL.  Also: Target Milestone: 1.4a
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).
Created attachment 116452 [details] [diff] [review]
patch

Merged to trunk (after bryner's nsIStyleContext removal, bug 117316, and bug
171830).
Attachment #113334 - Attachment is obsolete: true
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+
Fix checked in to trunk, 2003-03-13 07:29 PST.

Filed bug 197205 on removing null-checks from callers.
Blocks: 197205
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.