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)

defect

Tracking

()

RESOLVED FIXED
mozilla1.2beta

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

(Whiteboard: [patch][whitebox])

Attachments

(1 file, 5 obsolete files)

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.
Attached patch partial patch (obsolete) — Splinter Review
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
Attached patch patch, v. 2.0 (obsolete) — Splinter Review
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]
Attached patch updated patch (obsolete) — Splinter Review
This is updated to the trunk and cleaned up a bit more.
Attached patch updated patch (obsolete) — Splinter Review
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+
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.
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 !
Whiteboard: [patch][dev notes] → [patch][whitebox]
Also, fwiw, something in this patch breaks text zoom on certain things
(especially form controls).
Attached patch patch (obsolete) — Splinter Review
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.
OK, I see.
Blocks: 195073
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).
Attached patch patchSplinter Review
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
Closed: 21 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: