Closed Bug 64116 Opened 19 years ago Closed 16 years ago

nsStyleSet.cpp poorly written

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.5beta

People

(Reporter: buster, Assigned: bryner)

Details

(Keywords: helpwanted)

Attachments

(6 files, 1 obsolete file)

1. mFrameConstructor is used all over the place without null checks.  I would add:
  NS_ASSERTION(mFrameConstructor, "unexpected null frame constructor");
  if (!mFrameConstructor) { return NS_ERROR_NULL_POINTER; }

2. no pointer arguments are checked for null.  I would at least add assertions.
mine
Assignee: clayton → buster
code clean-up bug, not a big priority for me right now.  Any takers?
Keywords: helpwanted
Target Milestone: --- → Future
Priority: -- → P4
Build reassigning Buster's bugs to Marc.
Assignee: buster → attinasi
I think I partly disagree with the suggestions in this bug, but there is
certainly some cleanup to be done.
Assignee: attinasi → misc
Component: Layout → Layout: Misc Code
QA Contact: petersen → nobody
How about giving this to bz?  No rush, of course.
Assignee: misc → bzbarsky
Component: Layout: Misc Code → Style System
Thanks. ;)

So the changes I've made so far are:

1)  Move from nsISupportsArray to nsCOMArray for type-safety, readability, etc
2)  Move from having lots of copies of the same code to having some macros
3)  Change some function signatures.

I seem to recall you mentioning something about looping over the arrays instead
of using Enumerate(); what was the reasoning there?

Oh, and if you think of things to do to this code feel free to dump them in the
bug, of course.
Priority: P4 → P2
Target Milestone: Future → mozilla1.5beta
EnsureRuleWalker should probably return an error/success indicator.
I have some EnsureRuleWalker-related changes in my tree already (see bug
154751), although I'm just checking mRuleWalker for null afterwards, and I'll
probably land those changes soon.
OS: Windows NT → All
Priority: P2 → P1
Hardware: PC → All
So, I have a patch I've been working on that eliminates nsIStyleSet as an XPCOM
interface, plus some other changes to make things faster/smaller.  The patch is
somewhat large but the changes are pretty straightforward:

- Consolidated style sheet add/remove API to not duplicate code for each type of
style sheet. Switched to an array of |nsCOMArray|s for holding the style sheets.

- Added batching so that updating the rule processor array can be deferred until
after all style sheets are added/removed.

- Stop null-checking the pres shell's style set everywhere.  Init() will fail if
it could not create the style set object; after that it can be assumed that the
style set is non-null.

- Moved ownership of the frame constructor from the style set to the pres shell,
and made an inline getter for the frame constructor on nsIPresShell.  As with
the style set, the frame constructor is guaranteed to be non-null after
PresShell::Init returns success, for the lifetime of the presshell.

- Removed nsStyleSet methods that simply forwarded to the frame constructor;
changed callers to call the frame constructor methods directly.

- Removed XPCOM component manager creation of both the frame constructor and
style set objects.

- Moved layout debugging hooks off of nsStyleSet.  nsIStyleSet::List (which
listed the document style sheets) becomes nsIDocument::ListStyleSheets, and
nsIStyleSet::ListContexts (which printed the style context tree starting at a
given frame) moves onto nsIPresShell.

- Converted nsIStyleRuleSupplier and its implementation in XBL to use a COMArray
enumerator, to stay compatible with the storage in nsStyleSet.

- Cleaned up and condensed some code in editor that was dealing with style sets.
 It now uses the nsIDocument API exclusively for adding and removing document
style sheets (currently it's inconsistent - it uses the document API for adding
a sheet, but the style set API for removing a sheet).

- Moved the override stylesheet hooks onto nsIPresShell.  It's kind of
unfortunate that we need this, but I'd like to defer any changes in that until
later.

- Moved the agent stylesheet hooks that the chrome registry uses onto nsIPresShell.

- Moved PresShell::CloneStyleSet inside #ifdef DEBUG, it's only needed there.
Attached patch big patch (obsolete) — Splinter Review
The aforementioned patch
Comment on attachment 138318 [details] [diff] [review]
big patch

dbaron, boris, either of you want to take a look at this?
Attachment #138318 - Flags: superreview?(bz-vacation)
Attachment #138318 - Flags: review?(dbaron)
Here's a review of roughly the first third of the patch (it's a little late to
untangle the nsStyleSet.cpp diffs):

I don't like the |StyleFrameConstruction| name for the accessor.  How
about |FrameConstructor|?  I'd rather that interface name went away and
we didn't have to think about two names for the same thing.
(You can just diff, un-patch, search-replace (with "()"), and re-patch.)
 
 
Index: content/base/public/nsIDocument.h
 
+#ifdef DEBUG
+#include <stdio.h>   // for FILE definition
+#endif
 
Don't |#ifdef| an |#include|.
 
 
Index: content/base/src/nsDocument.cpp
 
+    ((nsIPresShell *)mPresShells.ElementAt(indx))->StyleSet()->
 
NS_STATIC_CAST(nsIPresShell*, mPresShells.ElementAt(indx))->StyleSet()->
 
(and NS_STATIC_CAST as well just below)
 
 
in DocumentViewerImpl::CreateStyleSet(nsIDocument* aDocument, please use
a temporary variable rather than (*aStyleSet) all over the place.  Also,
it would probably be good to return already_AddRefed<nsStyleSet> instead
of using an nsStyleSet**.
 
 
In the nsPrintEngine.cpp change, it would be good to actually handle
out-of-memory.
 
 
nsStyleSet.cpp:
 
+#ifdef DEBUG
+#include "nsIFrame.h"
+#endif
 
Don't |#ifdef| includes.
 
+#ifdef MOZ_PERF_METRICS
+NS_IMPL_ISUPPORTS1(nsStyleSet, nsITimeRecorder)
 #endif
 
This isn't going to work so well -- you need to use
NS_IMPL_QUERYINTERFACE1 and stub out dummy AddRef and Release methods.
 
I'm up to "StyleSetImpl::RecycleArray".
> This isn't going to work so well -- you need to use
> NS_IMPL_QUERYINTERFACE1 and stub out dummy AddRef and Release methods.

I'm not sure what you mean.  The object is refcounted, so it needs to have
working addref/release methods.  Though now that I think about it, I'm not sure
it needs to be refcounted at all.  Is that what you're saying?
> in DocumentViewerImpl::CreateStyleSet(nsIDocument* aDocument, please use
> a temporary variable rather than (*aStyleSet) all over the place.  Also,
> it would probably be good to return already_AddRefed<nsStyleSet> instead
> of using an nsStyleSet**.

Well, I need to be able to return NS_ERROR_OUT_OF_MEMORY from that function...
are you suggesting making the nsresult an out-parameter instead? (or can a
|already_AddRefed| be an out-parameter?)
I think I'm going to just make the object non-refcounted.

As for the whole nsITimeRecorder business, another option is to simply get rid
of nsITimeRecorder.  nsStyleSet is the only implementation, and the only caller
is the pres shell.  Since it looks unlikely that anyone will be making further
use of this interface, my vote is to ditch it and save the trouble of a fake
nsISupports implementation.
Oh, I was thinking it wasn't refcounted since there was no Addref and Release
implementation #ifndef MOZ_PERF_METRICS -- or at least I didn't see it.  (Was it
somewhere else in the file?)
OK, nsIStyleSet** is fine with me.
Just FYI, in my tree I went ahead and removed refcounting from nsStyleSet, and
also removed the style resolution timing stuff (dbaron and I don't think anyone
is using it).  I'll include these changes in the next patch I post, which will
be after I've got all of dbaron's review comments to incorporate.
One more problem I found and fixed in my tree -- in PresShell::Destroy(), it
needs to set mStyleSet to null after deleting it so that Destroy() is not called
a second time from the destructor.
Here's comments on all of the rest of the patch except for the 3 things that I
noted at the end (I'll try to get to those later today -- I just skipped them
since they seemed to require close inspection and I wanted to feel like I was
mostly done):

nsStyleSet.cpp, continued:
 
  In |EndUpdate|, assigning to dirty and batching on separate lines
  would probably be clearer, and what you currently do seems to be
  asking for weird compiler bugs with bitfields to appear.
 
  In |EnableQuirkStyleSheet|, the following code does different things to
  the refcount in the two halves:
  +#ifdef DEBUG
  +      CallQueryInterface(sheet, &cssSheet);
  +      NS_ASSERTION(cssSheet, "Agent sheet must be a CSSStyleSheet");
  +#else
  +      cssSheet = NS_STATIC_CAST(nsICSSStyleSheet*, sheet);
  +#endif
  It would be better to just do:
  NS_ASSERTION(nsCOMPtr<nsICSSStyleSheet>(do_QueryInterface(sheet)) == cssSheet,
              "Agent sheet must be a CSSStyleSheet");
  after assigning to |cssSheet| with the non-DEBUG line above.
 
nsStyleSet.h:
  in definition of |dirty|, comment that there's one dirty bit per |sheetType|
 
 
The |ListStyleSheets| method you added to nsDocument should probably be
on nsPresShell instead.  That would be the equivalent of what it used to
do (rather than getting the document from the pres shell and then iterating
through all pres shells).  It also seems like something that shouldn't
really be on nsDocument.  (Requires changes to viewer and layout-debug.)
 
nsIPresShell.h:
  Don't |#ifdef| includes.
 
  I already mentioned s/StyleFrameConstruction()/FrameConstructor()/, I
  think.
 
nsPresShell.cpp:
  Should BeginUpdate and EndUpdate with UPDATE_STYLE call
  StyleSet()->[Begin|End]Update() ?  I think they should.  You might
  want to double-check with bz, though...
 
How about making nsCSSFrameConstructor::CreateContinuingFrame take only
a pres context argument and not a pres shell argument so you don't have
to use an extra variable every time you call it?
 
Need to go back to nsHTMLEditor.cpp and the stuff in nsPresShell that it
  calls.
Need to go back to removal of NotifyStyleSheetStateChanged.
Need to go back to nsChromeRegistry.cpp (both!) and the two new
  functions in nsPresShell.
Comment on attachment 138318 [details] [diff] [review]
big patch

PresShell::AddOverrideStyleSheet and PresShell::RemoveOverrideStyleSheet both
need to call ReconstructStyleData (see what the old code in nsHTMLEditor did).
And while you're there, you should move the |mStylesHaveChanged = PR_FALSE|
from EndUpdate into ReconstructStyleData itself.

Also, you probably shouldn't remove the styleSheet->SetOwningDocument call
(and comment) in nsHTMLEditor.cpp.

A list of things you should test:
 * alternate stylesheet switching
 * change font size prefs (should take effect immediately)
 * change color prefs (should take effect immediately)
 * Using CSSOM to add and remove style sheets.
 * Adding and removing style / link elements from the document.
 * changing between views in editor
 * adding and removing stylesheets in editor
> * Using CSSOM to add and remove style sheets.

Never mind.  The CSSOM doesn't support that.
Attached patch revised patchSplinter Review
addressed all of dbaron's comments.  fixed a bug where enabling/disabling style
sheets didn't cause the appropriate updates to happen (because the rule
processors' RuleCascadeData is no longer valid).  changed rule node gc to not
gc the root node so we don't have to worry about re-creating mRuleWalker.
Attachment #138318 - Attachment is obsolete: true
Attachment #138558 - Flags: superreview?(bz-vacation)
Attachment #138558 - Flags: review?(dbaron)
Attached file nsStyleSet.h
this should have been in the above patch
Comment on attachment 138558 [details] [diff] [review]
revised patch

r+sr=dbaron as long as you:
 * cvs add nsStyleSet.h :-)
 * cvs remove nsIStyleSet.h (that also isn't in this patch)
 * make the license on nsStyleSet.h match the license on nsIStyleSet.h /
nsStyleSet.cpp rather than being a new license (although feel free to add
yourself to the contributors list), since it's mainly moved code.
Attachment #138558 - Flags: superreview?(bz-vacation)
Attachment #138558 - Flags: superreview+
Attachment #138558 - Flags: review?(dbaron)
Attachment #138558 - Flags: review+
Attachment #138318 - Flags: superreview?(bz-vacation)
Attachment #138318 - Flags: review?(dbaron)
-> me
Assignee: bz-vacation → bryner
I will try to read this tonight or worst-case tomorrow morning and answer at
least the question in comment 22
Checked in.  I went ahead and added the BeginUpdate changes dbaron talked about,
but I'll make any changes that boris requests for that.
Status: NEW → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
What's the ordering on the style sheet arrays?  nsStyleSet.h should document
this.... nsIStyleSet.h used to. The comment on override sheets makes it sound
like that array is in the opposite order from the other arrays or something? 
But GatherRuleProcessors works the same way for all sheet types... and
AddDocStyleSheet assumes most-significant-first ordering too.

Does nsIDocument need the stdio.h?  I don't see that interface using FILE anywhere.

Why are we calling the style set's BeginUpdate/EndUpdate directly from the
document?  I would have thought the presshell would have been a more appropriate
place to do that, since all the presshells will get notified anyway?  For that
matter, why did similar code for StyleSheetApplicableStateChanged() move from
the presshell to the document?

The document viewer code could be made leak-safer by using nsAutoPtr<nsStyleSet>
in both places where we have a "owning" ref to the style set and using forget()
as needed (at the return point from the function, and after we give the set to
the document).  As things stand, it looks like if the presshell's Init() fails
we will leak.  Also, it would be good to document in nsStyleSet.h that the
presshell owns this beast and that no one else does.

I don't see anyone destroying nsPrintObject's mStyleSet. Did you mean to make
that member an nsAutoPtr?

Add a comment in nsRuleNode::Sweep explaining _why_ the root should not destroy
itself?

If BuildDefaultStyleData fails, don't you need to Destroy the mDefaultStyleData?
 We may have allocated the reset data and failed on the inherited data.  In any
case, the error there should probably be NS_ERROR_OUT_OF_MEMORY.

In nsStyleSet::ReplaceSheets, why not just call AppendObjects() instead?

In nsStyleSet::AddDocStyleSheet, why not 
  |NS_PRECONDITION(aSheet && aDocument, "null arg");| ?

BeginUpdate/EndUpdate have one issue as done in that patch -- updates can nest.
 It may be better (more efficient) to have a counter for mBatching that
BeginUpdate increments, EndUpdate decrements and takes action if it has reached
zero.  The other places can still just check !mBatching to get the info they
need.  Arguably, presshell should use a similar counter for its own style update
batching...  As things stand, a nested update will cause us to stop batching too
early.

In nsStyleSet::EndUpdate, I think parens around "1 << i" would improve readability.

In nsStyleSet::NotifyStyleContextDestroyed we want to assert that !deleted, no?
 As in, the assert will fire every single time with the current code?

Now that you've fixed my XXX comment in nsHTMLEditor::AddOverrideStyleSheet,
mind adjusting the comment in nsDocument::SetStyleSheetApplicableState accordingly?

In nsIPresShell, I'd think [OWNS] would be more accurate than [STRONG].

In nsIStyleFrameConstruction.h, the comment for nsFindFrameHint shouldn't
mention style set.

+  result = nsComponentManager::CreateInstance(kFrameSelectionCID, nsnull,
+                                              NS_GET_IID(nsIFrameSelection),
+                                              getter_AddRefs(mSelection));

As long as you're touching this code:

  mSelection = do_CreateInstance(kFrameSelectionCID, &result);

Why do we no longer need the hack to cause rule processor reinit in
PresShell::SetPreferenceStyleRules?  Because the style set doesn't care about
changes on the rule level (that's what I seem to recall, at least).

Arguably, AddOverrideSheet should prepend, not append -- that way override
sheets that are added later will override those added earlier.

PresShell::VerifyIncrementalReflow leaks the cloned set, no?  I know it's debug
code, but still... nsAutoPtr, please?

I'm sorry it took me so long to get to this.  Holiday travel is not conducive to
reviews... :(
> Why do we no longer need the hack to cause rule processor reinit in
> PresShell::SetPreferenceStyleRules?  Because the style set doesn't care about
> changes on the rule level (that's what I seem to recall, at least).

Hm.  I tested changing the color prefs and it took effect immediately.  However,
the style set does need to know about style sheet enabling/disabling because it
causes the RuleCascadeData on the rule processor to become invalid.  Does
inserting/removing rules here cause the same situation?
> Now that you've fixed my XXX comment in nsHTMLEditor::AddOverrideStyleSheet,
> mind adjusting the comment in nsDocument::SetStyleSheetApplicableState
> accordingly?

Hm, the comment still seems accurate to me.

> Arguably, AddOverrideSheet should prepend, not append -- that way override
> sheets that are added later will override those added earlier.

Perhaps, but I don't want to make this change right now because of the editor
stuff that could break (and editor is the only user of this).

fix bz's review comments as described above and per irc conversation
Attachment #138606 - Flags: review?(bz-vacation)
> because it causes the RuleCascadeData on the rule processor to become invalid

Adding a rule certainly changes the RuleCascadeData... See
CSSRuleProcessor::ClearRuleCascades, which is called by rule insertions.  But
the RuleCascadeData seems to be internal state of the rule processor that the
style set doesn't really look at, to me... So removing that code like you did
_should_ be safe.  I think.  David?  Any idea?

> Hm, the comment still seems accurate to me.

Er.. editor no longer calls SetStyleSheetApplicableState directly like it used
to -- you removed that code in your patch.  The more I think about this, the
more I think that calling SetStyleSheetApplicableState() on the document for a
non-document sheet is really bogus.  Would it make sense to move the override
sheets into the documents sheets array a la inline style at some point?

> because of the editor stuff that could break 

Fair enough.   Makes sense.
Comment on attachment 138606 [details] [diff] [review]
fix bz's comments

>Index: content/base/src/nsDocumentViewer.cpp

>+  nsresult rv = CreateStyleSet(mDocument, getter_Transfers(styleSet));

I'd use an nsAutoPtr in the CreateStyleSet impl too, in case someone decides to
add early error returns there....

>Index: content/base/src/nsStyleSet.cpp

>+    NS_ASSERTION(!deleted, "Root not must not be gc'd");

"root node"

r=bzbarsky with those nits.
Attachment #138606 - Flags: review?(bz-vacation) → review+
> fixed a bug where enabling/disabling style
> sheets didn't cause the appropriate updates to happen (because the rule
> processors' RuleCascadeData is no longer valid).

Could you remind me how you fixed this?  Perhaps a better fix would be to call
ClearRuleCascades() from CSSStyleSheetImpl::SetEnabled.

I'm again convinced (after being convinced and unconvinced) that the removal of
ClearRuleProcessors is safe, thanks to the code in CSSStyleSheetImpl::DidDirty,
except for the case of enabled state changing (see previous paragraph).
I fixed that by causing the style set to empty its rule processor array and
recreate the rule processors when a stylesheet is enabled or disabled.

I think you're right that calling ClearRuleCascades from SetEnabled would do the
trick and we could remove the clearing of the rule processor array, and in fact,
remove the notification of the style set that the change has happened.
This patch reverts back to using a raw pointer for the style set object, but
properly cleaning up in error cases.  It reduces the code size in
nsDocumentViewer.o by 1.1 KB on my machine.
Comment on attachment 138795 [details] [diff] [review]
remove nsAutoPtr usage to reduce bloat

bz, what do you think?
Attachment #138795 - Flags: review?(bz-vacation)
Comment on attachment 138795 [details] [diff] [review]
remove nsAutoPtr usage to reduce bloat

r=me I guess, though I'm still confused about the huge size savings... do
nsCOMPtr and nsRefPtr have similar size issues??
Attachment #138795 - Flags: review?(bz-vacation) → review+
Perhaps.  One thing we found in looking at this code is that there will be at
least 2 extra function calls:  getter_Transfers ends up calling |assign(0)|
which calls |delete oldPtr;|.  We will also |delete mRawPtr| on destruction -
mRawPtr will be null since this is after .forget().

From some simple tests, it doesn't look like the compiler is able to optimize
away these calls to delete(NULL).  If we were to change the logic in assign() to:

if (oldPtr)
  delete oldPtr;

then it looks like it can optimize away the code if it can see that oldPtr will
be null.  Of course, that's at the expense of extra code in the cases where it
can't tell that it will be null.  Unfortunately there's no way to indicate to
the compiler what we really want, which is "If you can tell that oldPtr is null
at compile time, then don't call delete, otherwise just generate a call to
delete".  Of course, it seems like that would be smart behavior anyway and not
something we should have to ask for.

(note to self: check whether __builtin_constant_p() does what I just described
for gcc)
> (note to self: check whether __builtin_constant_p() does what I just described
> for gcc)

Doesn't seem to, though I feel like it's a bug in gcc.  If it can determine that
mRawPtr is 0 such that it can optimize away an |if| block, then it seems like it
should return true for __builtin_constant_p().
Actually, the number of added function calls will be higher than 2.  gcc will
inline the destructor for nsAutoPtr at each of the early return points in the
function.  This should be improved when the tree-ssa branch lands for gcc 3.5.

I checked in the bloat-reduction patch.
Per http://gcc.gnu.org/bugzilla/show_bug.cgi?id=13643 , we'll be able to make
gcc generate something more intelligent here hopefully in 3.5.
You need to log in before you can comment on or make changes to this bug.