876085 - Stop returning nsChangeHint_NeedDirtyReflow unnecessarily from CalcDifference for SVG property changes

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Jonathan Watt [:jwatt]]

In bug 875037 I discovered that it's not really possible to just return nsChangeHint_NeedReflow from a style struct's CalcDifference method, so I ended up returning nsChangeHint_NeedDirtyReflow with it. That will cause us to do excessive work though, so we should try and remove that.

The issue that I encountered is that if CalcDifference just returns the hint nsChangeHint_NeedReflow we can end up hitting:

  "ASSERTION: Shouldn't be trying to restyle non-elements directly"

under the stack:

  nsStyleChangeList::AppendChange
  CaptureChange
  nsFrameManager::ReResolveStyleContext
  nsFrameManager::ReResolveStyleContext
  nsFrameManager::ReResolveStyleContext
  nsFrameManager::ComputeStyleChangeFor
  nsCSSFrameConstructor::RestyleElement
  mozilla::css::RestyleTracker::ProcessOneRestyle
  mozilla::css::RestyleTracker::DoProcessRestyles
  mozilla::css::RestyleTracker::ProcessRestyles
  nsCSSFrameConstructor::ProcessPendingRestyles

where AppendChange is passed an nsTextFrame. (See bug 875037 comment 8 for its specifics.)

The issue seems to be that nsChangeHint_NeedReflow is not handled for descendants if the nsChangeHint_NeedDirtyReflow isn't also present:

http://mxr.mozilla.org/mozilla-central/source/layout/base/nsChangeHint.h?rev=7a2f7a45819a&mark=195-196#182

which is why we end up processing the nsChangeHint_NeedDirtyReflow hint for an nsTextFrame and triggering the assertion above.

Any thoughts on how we should handle this, dbaron?

Comment 1

[Jonathan Watt [:jwatt]]

The only other style struct change that seems to result in only nsChangeHint_NeedReflow being returned from CalcDifference is nsStylePosition::mJustifyContent. I wonder if we just don't have any tests that cause that to trigger the assertion.

Comment 2

[Jonathan Watt [:jwatt]]

Assuming I land tomorrow, this bug will be to remove the lines:

https://mxr.mozilla.org/mozilla-central/search?string=bug%20876085

Comment 3

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

So it's a really bad idea to return a hint that's non-inherited from the CalcDifference method for an inherited struct.  (I think there's a case or two where such a bug slipped in, and an existing bug on the resulting assertions... but we don't want more.)

Is this for an inherited struct?  If so...what do you intend it to mean to want NeedReflow but not NeedDirtyReflow?

(sorry, very quick answer, but pressed for time right now)

Comment 4

[Cameron McCormack (:heycam)]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #3)
> So it's a really bad idea to return a hint that's non-inherited from the
> CalcDifference method for an inherited struct.  (I think there's a case or
> two where such a bug slipped in, and an existing bug on the resulting
> assertions... but we don't want more.)

I do not have a good grasp on the different style change hints yet.  If CalcDifference should not return non-inherited hints for inherited style structs, can we assert this?  Maybe in the DO_STRUCT_DIFFERENCE macro used in nsStyleContext::CalcStyleDifference?

Comment 5

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

Yes, we probably should; there's an open bug on such an assertion (relatively new, and I think the same assertion as comment 0) that I haven't gotten to yet, I think.


Anyway, what was the thing that you thought didn't need NeedDirtyReflow?

Comment 6

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

(FWIW, this bug as described is WONTFIX; we should probably add some assertions to make the situation clearer, though.)

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 876085 - Remove unneeded dirty reflows. r=jwatt,longsonr

We've reworked change hints tons of times since this was filed, this
works now.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/16950be9e3a0
Remove unneeded dirty reflows. r=longsonr

Comment 9

[Sandor Molnar[:smolnar]]

https://hg.mozilla.org/mozilla-central/rev/16950be9e3a0