Closed Bug 171830 Opened 22 years ago Closed 22 years ago

[FIX]Don't use nsCSSPropList hints for inline style changes


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






(Reporter: bzbarsky, Assigned: bzbarsky)



(Keywords: testcase)


(1 file, 7 obsolete files)

I'll attach a patch tomorrow if my floppy drive cooperates and I find my
floppies.  The basic idea of the patch is as follows:

1)  Add AttributeWillChange to nsIDocumentObserver
2)  Cache the old style data in nsCSSFrameConstructor::AttributeWillChange
3)  Compare the old to the new in nsCSSFrameConstructor::AttributeChanged

The current setup passes the cached data out as an nsISupports, then gets it
passed back in.  I talked to jst, and he does not like that very much.. I'll be
attaching the patch with that setup as-is (because it pretty much works) and
I'll also attach the conversation he and I had.

I'll work on posting some testcases for testing as well.
Blocks: 158713
Depends on: 171808
Attached file Conversation with jst (obsolete) —
Whiteboard: [dev notes]
Attached patch Work-in-progress (obsolete) — Splinter Review
This is pretty much complete, imo, pending resolution of jst's concern.  There
are some testcases to test with (and more are welcome) at:
Keywords: testcase
Whiteboard: [dev notes]
Blocks: 171522
Blocks: 164840
Comment on attachment 101301 [details] [diff] [review]

Good news -- this seems to fix a few random bugs.  Bad news -- this regresses
bug 138120.  The testcase in that bug triggers the

	     "parent frame but no child frame or undisplayed entry");

assertion....  Which seems bad.
Comment on attachment 102414 [details] [diff] [review]
This fixes the assertion and the regression (apply in addition to, not instead of)

Why bother with asserts if they get ignored?  I've moved this patch to its
proper place -- bug 151883.
Attachment #102414 - Attachment is obsolete: true
Depends on: 151883
Depends on: 174178
Depends on: 140789
Comment on attachment 101301 [details] [diff] [review]

As suspected, this regresses bug 118415.  I'm still not sure whether the patch
for that _really_ fixed things in the old world or just papered over the
problem... in any case, I need to come up with a fix in the context of the new
Attachment #101301 - Flags: needs-work+
so i think i like jsts suggestion with a servergenerated key better since it
less work in the common case (that we're not passing data from WillChange to

One way to take care of the memorymanagement is to have the observer hold a hash
of nsISupports, that way you should never leak anything. You still might hold on
to an object much longer then you need though, if someone aborts, which
admittedly is a bad thing.

hmm.. though we could add an AttributeDidntChange notification as well, that is
called if someone aborts.
Hoping to get that regression addressed sometime this week.  We need to come to
a decision here on the exact way to pass the context around....
Priority: -- → P1
Target Milestone: --- → mozilla1.3alpha
Attached patch checkpoint (obsolete) — Splinter Review
This resolves all known regressions.  Remaining issues:

1)  How should the notifications work?
2)  What datatype should be used for storing the style data (nsCachedStyleData
    feels like a hack when all is said and done)... (I'd do a stub impl of
    nsIStyleContext, but we want to remove nsIStyleContext....)
3)  How to avoid code duplication in the CalcDifference functions?
4)  Disentangling from some other patches.

Point #4 means there's stuff in this patch that's pretty irrelevant to this
bug... I'm sorry about that; I'll try to disentangle as soon as I can... in the
meantime, some general feedback on the approach and such would be much
Attachment #101301 - Attachment is obsolete: true
Depends on: 177543
Yeah, there's got to be a way to eliminate the duplication of this
CalcStyleDifference function.

Maybe we could make them both small by using a loop to iterate through the
different style structs, instead of writing them all out explicitly?
Yeah... Another possibility is that we have an nsIStyleContextBase and
nsIStyleContext, with the latter inheriting from the former....
nsIStyleContextBase could be what CalcDifference() takes and instead of using
nsCachedStyleData to move this info around I could use an nsIStyleContextBase
(which would have the minimal number of methods needed to actually function in

Of course this represents even _more_ virtualization of nsIStyleContext, which
may not be a good idea.... :(
Can't you just make a 'fake' nsStyleContext with your nsCachedStyleData in it,
but  decoupled from rules and other style contexts?
I considered doing that too...  I'll give it a shot tonight or tomorrow and see
how it affects perf.  
Attached patch Use a temporary style context (obsolete) — Splinter Review
Roc, that works beautifully.  ;)  This patch has all the irrelevant stuff
stripped out and is basically reviewable.... (modulo the specific changes to

In case people are interested in the perf impact, here're the results on (3
or 5 trials for each test):

inlineStyleOnBody: 1224 1205 1224 1208 1225
inlineStyleOnBodyWithSelectorOnStyleAttr: 1440 1378 1379 1441 1379
uselessInlineStyleOnBody: 1601 1606 1626 1627 1626
movingEmptyDivs: 12963 12879 12969
movingFullDivs: 57013 56354 56344
notMovingFullDivs: 46770 46268 46241

inlineStyleOnBody: 879 892 875 893 881
inlineStyleOnBodyWithSelectorOnStyleAttr: 1573 1533 1530 1535 1535
uselessInlineStyleOnBody: 431 415 436 419 436
movingEmptyDivs: 14035 13958 13923
movingFullDivs: 51825 51367 51483
notMovingFullDivs: 8693 8344 8217

And testing with :

Creating 200 layers - time - 1174 1153 1195
Moving 200 layers 100 px to left - time - 8326 8627 8446
Cliping 200 layers - time - 310 310 311
Setting Opacity 10% - time - 8938* 8847* 8816*
Reverting z-index for 200 layers - time - 307 319 337
Moving with changing opacity and clip - time - 21116* 21031* 21022*
Inputing text into 200 layers - time - 512 509 509
Resizing 200 layers - time - 562 587 565

Creating 200 layers - time - 1142 1143 1137
Moving 200 layers 100 px to left - time - 8635 8583 8652
Cliping 200 layers - time - 321 318 320
Setting Opacity 10% - time - 24867** 22199** 22133**
Reverting z-index for 200 layers - time - 325 297 292
Moving with changing opacity and clip - time - 42236** 40992** 41705**
Inputing text into 200 layers - time - 525 517 495
Resizing 200 layers - time - 575 565 567

*No repaints or reflows happen between opacity=100% and opacity=10% due to
 continuous reframing
**Repaints and reflows happen as opacity/position/clip change
Attachment #104642 - Attachment is obsolete: true
I considered subclassing nsStyleContext for the snapshot (to override
PeekStyleData not to look in the rulenode) , but wasn't sure whether making
PeekStyleData virtual was ok.... if someone has strong feelings one way or the
other, let me know.
Blocks: 175070
Flags: blocking1.3b?
I'm not getting to this till January, so this is not making 1.3.
Flags: blocking1.3b?
Target Milestone: mozilla1.3alpha → mozilla1.4alpha
> The current setup passes the cached data out as an nsISupports, then gets it
> passed back in.  I talked to jst, and he does not like that very much..
+/* Strategy for dealing with inline style:
+ *
+ * 1) During AttributeWillChange, see if we already have an inline style
+ *    rule.  If we do, get the style context and cache the style data and the
+ *    pointer (weak ref; we just care about the value) to the style rule.
+ *    Pass this data out as aContext.
+ * 2) During AttributeChanged, see if we have a style rule and if so whether
+ *    it's the same as the rule we remembered in AttributeWillChange.  If the
+ *    rules are the same, the style context will not be changing!
+ * 3) If the inline style rules are different, just do a normal re-resolve
+ *    and get the hint from that.  This will create new style contexts as
+ *    needed.
+ */

+nsCSSFrameConstructor::AttributeWillChange(nsIPresContext* aPresContext,
+                                           nsIContent*     aContent,
+                                           PRInt32         aNameSpaceID,
+                                           nsIAtom*        aAttribute,
+                                           nsISupports**   aContext)
+  *aContext =
+    NS_STATIC_CAST(nsISupports*,
+                   new StyleAttrChangeContext(snapshot, rule, importantRule,
+                                              aPresContext));

Possible suggestion/simplication to avoid this issue. What about using the
frame manager instead of dragging the context around? That is, rather than
returning the context as you are doing (just to get it back later on), just
stash it as a property in the frame manager. Then, when AttributeChanged() is
called later on, you can fetch it back from there.

+  //XXXbz We need to decide what the contract is here.  Is it OK to call
+  //AttributeWillChange but not AttributeChanged (eg if an error occurs while
+  //changing the attribute)?  If so, we should say so explicitly, I think.

It will also avoid this issue. In the case of success (i.e., perfect match of
"willchange" and "changed" pairs), the property is removed. In case of failure
("willchange" without "changed"), there will be at most a dangling entry but it
will overriden if "willchange" is called again, or it will be destroyed when the
manager goes away, leaving no leak behind.
What about the case when style is being changed on a display:none element 
(which has a style context in the undisplayed map but has no frame)?  I'm not 
sure I can use the frame manager there (though I _could_ maybe use the 
undisplayed map itself....)
> I _could_ maybe use the undisplayed map itself

Seems plausible since the map refers to the old style context data -- until
completion of the maintainance [set->ClearStyleData()] that AttributeChanged() does.

This actually raises one question with your |GetStyleContextFor|... Isn't there
a loophole in your patch in that you are getting the same old context in the
situation of display=none? i.e., are you not testing the same old context
against itself?
That's the general problem with the inline style changes -- the fact that the 
style context pointer does _not_ change in the process.  That's why I save the 
old style data, then clear the style context's cache, then compare, forcing the 
style context to recompute style data in the process...

There is an issue I'm looking into with this whole approach (and this may apply 
to the current code too) where style data may get cleared and this may cause 
later changes to not trigger the right change hint...
Blocks: 140789
No longer depends on: 140789
Depends on: 188803
Attached patch Alternate (cleaner) approach (obsolete) — Splinter Review
I'm a _lot_ happier with this one... It still fixes all the problems the old
one fixed, but mostly _removes_ code.  Perf testing shows that it's within 1-2%
of current trunk on all the DHTML tests I've run (except the ones I rigged to
show off the benefits of this patch -- we're 10-60% faster than trunk on those
;) ).

This patch depends on bug 117316 being fixed so we don't grow the ruletree like

Once that happens, the perf here should be retested, but I suspect that it's
not going to change much.
Depends on: 191794
Blocks: 173373
Attachment #104971 - Attachment is obsolete: true
Attachment #113089 - Flags: superreview?(roc+moz)
Attachment #113089 - Flags: review?(dbaron)
Summary: Don't use nsCSSPropList hints for inline style changes → [FIX]Don't use nsCSSPropList hints for inline style changes
Comment on attachment 113089 [details] [diff] [review]
Alternate (cleaner) approach

r=dbaron (although maybe we shouldn't be using the literal 0x7fffffff as the
weight without a macro to hide it)
Attachment #113089 - Flags: review?(dbaron) → review+
Comment on attachment 113089 [details] [diff] [review]
Alternate (cleaner) approach

isn't there a PR_INT32_MAX macro for that?
Yeah, good point.  I'll change that to PR_INT32_MAX
Comment on attachment 113089 [details] [diff] [review]
Alternate (cleaner) approach

yeah baby!

I hope that we can hunt down and kill the other users of the hint from the
Attachment #113089 - Flags: superreview?(roc+moz) → superreview+
Blocks: 193275
Blocks: 74951
Attachment #101209 - Attachment is obsolete: true
Attachment #113089 - Attachment is obsolete: true
Blocks: 194615
No longer blocks: 194615
*** Bug 194928 has been marked as a duplicate of this bug. ***
Blocks: 195073
Request changing SEVERITY to CRITICAL because this blocks 195073 which is
SEVERITY = CRITICAL.  Also, comment 22 states this patch depends on 117316.
Let me know when you're ready to land.
Pretty much any time (as in the patch is ready to go).  The problem is finding a
time to watch the tree...  Can you do Tuesday morning?  Should we ask for a carpool?
I could watch the tree for you if needed (find me on IRC to make sure I'm
around), and I don't think there's a need for a carpool.
This should apply to a current trunk.....
Attachment #115276 - Attachment is obsolete: true
I already checked it in (based on the merging I did a week or so ago), actually.
Ah, cool.  Thank you!
Fix checked in to trunk, 2003-03-06 11:06/07 PST with additional unused variable
removal at 13:01 PST.
Closed: 22 years ago
Resolution: --- → FIXED
+  if (!decl) {
+    return result;
+  }
+  nsCOMPtr<nsICSSLoader> cssLoader;
+  nsCOMPtr<nsICSSParser> cssParser;
+  nsCOMPtr<nsIURI> baseURI;
+  result = GetCSSParsingEnvironment(mContent,
+                                    getter_AddRefs(baseURI),
+                                    getter_AddRefs(cssLoader),
+                                    getter_AddRefs(cssParser));
+  if (NS_FAILED(result)) {
+    return result;
+  }

Possible leak of |decl|. (2 places).

Also missing the check for success in the call of setDecl() and the ruleAbort
business in case of failure.
> Possible leak of |decl|. (2 places).

Actually, no.  |decl| is not addrefed by GetCSSDeclaration.

> and the ruleAbort business in case of failure

Nom the only case in which RuleAbort() is needed is if the decl was never set in
a rule (and the decl we get out of GetCSSDeclaration is guaranteed to be set in
a rule).  RuleAbort() is a fancy name for "delete" in the case of
CSSDeclarations... and a decl owned by a rule will be released by the rule when
the rule dies.

Now we _could_ end up with a double-delete of the decl if SetHTMLAttribute fails
in SetCSSDeclaration when called from GetCSSDeclaration... Bug 196271 filed. 
Also filed bug 196273 on general nsCSSDeclaration memory management.
Blocks: 175686
Hmm... the not clearing of display data seems to have fixed some {ib} bugs (eg
bug 175686).  It'd be good to retest the other {ib} bugs.
Blocks: 179935
*** Bug 156206 has been marked as a duplicate of this bug. ***
Blocks: 143202
Blocks: 190558
Blocks: 196603
Note that this caused regression bug 196603
*** Bug 81678 has been marked as a duplicate of this bug. ***
Blocks: 130800
You need to log in before you can comment on or make changes to this bug.