827239 - "ASSERTION: Going to leak style data" with CSS animation, rem unit

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect)

Description

[Jesse Ruderman]

Attachment: testcase

###!!! ASSERTION: Going to leak style data: '!aHighestNode->mStyleData.mResetData-> mStyleStructs[eStyleStruct_Padding]', file layout/style/nsRuleNode.cpp, line 6245

Assertion failure: mPresArenaAllocCount == 0 (Some pres arena objects were not freed), at layout/base/nsPresShell.cpp:770

Comment 1

[Jesse Ruderman]

Attachment: stacks

Comment 2

[Boris Zbarsky [:bzbarsky]]

David, could this be fallout from the changes to cache rem in the ruletree?

Comment 3

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

Did you test that?  I'd have guessed bug 572200, but I'll look into it further.

Comment 4

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

Actually, I suspect you're right, and I suspect the assertion is firing on the way out of a case where we recur into style data computation on the same node.

Comment 5

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

Why does PresShell::FlushPendingNotifications call nsCSSFrameConstructor::CreateNeededFrames before it calls ProcessPendingRestyles?  (I suppose maybe because if we processed restyles first we might create the frames as part of that restyle processing?)  It seems like processing pending restyles first would both be more efficient (no restyle processing that applies to frames we just created) and would also solve this (in that we'd restyle the root before we created frames new frames for its descendant), at least if I'm understanding this correctly.

(I think what's happening is that the boom() function calls appendChild() (which appends a child to the root) and calls addSheet() (which posts a restyle for the root).  We process the appendChild *first*, which gets the style for the child ahead of the root, during which the rem code gets the style for the root, which actually has exactly the same set of rules as its child.  I think if we always processed style changes rootmost-first we wouldn't have this problem, and I tend to think we're actually pretty close to already obeying this invariant.)

Comment 6

[Boris Zbarsky [:bzbarsky]]

> Did you test that? 

Not yet, no.

> I suppose maybe because if we processed restyles first we might create the frames as part
> of that restyle processing?

I believe that was the reasoning at the time, yes....  Timothy may recall for sure.

Comment 7

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

I wonder if we could merge the frame creations into the restyle tree.

Comment 8

[Timothy Nikkel (:tnikkel)]

(In reply to Boris Zbarsky (:bz) from comment #6)
> > I suppose maybe because if we processed restyles first we might create the frames as part
> > of that restyle processing?
> 
> I believe that was the reasoning at the time, yes....  Timothy may recall
> for sure.

I think my reasoning at the time was to avoid any changes of behaviour. So since before lazy frame construction we always had frames created when restyle processing I put that call there so we'd also have frames constructed before restyle processing with lazy frame construction. There might have been more, but off the top of my head that's it.

Comment 9

[Timothy Nikkel (:tnikkel)]

(In reply to David Baron [:dbaron] from comment #7)
> I wonder if we could merge the frame creations into the restyle tree.

That sounds like a good idea. The two paths are quite similar. And restyle processing has advanced since when the lazy frame construction stuff was added.

Comment 10

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

Another reason to call it first is to avoid assertions like those in bug 997506.

Comment 11

[Jesse Ruderman]

WFM. 

dbaron, do you want to file any followups based on your comments above?