154751 - ensure nsStyleContext::GetStyleData never returns null

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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.

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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).

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Well, the patch seems to work... at least the browser starts up and things work. :-)

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: updated patch

This is updated to the trunk and cleaned up a bit more.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: updated patch

merged with more recent changes to trunk

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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.)

Comment 7

[Madhur Bhatia]

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!

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No.  It's an issue about how we handle out-of-memory conditions.

Comment 9

[Madhur Bhatia]

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 !

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Also, fwiw, something in this patch breaks text zoom on certain things
(especially form controls).

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: 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.)

Comment 12

[Boris Zbarsky [:bzbarsky]]

I'm going to try to get to this review within the next week....

Comment 13

[Boris Zbarsky [:bzbarsky]]

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....

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, the font zoom stuff was intentional.  You get cleanup mixed in when I attach
patches from my main tree.

Comment 15

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

[Boris Zbarsky [:bzbarsky]]

OK, I see.

Comment 17

[Ronald Tilby]

Request changing SEVERITY to CRITICAL because this blocks 195073 which is
SEVERITY CRITICAL.  Also: Target Milestone: 1.4a

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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).

Comment 19

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Merged to trunk (after bryner's nsIStyleContext removal, bug 117316, and bug
171830).

Comment 20

[Boris Zbarsky [:bzbarsky]]

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!

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Fix checked in to trunk, 2003-03-13 07:29 PST.

Filed bug 197205 on removing null-checks from callers.