Closed Bug 984226 Opened 6 years ago Closed 6 years ago

OverflowChangedTracker should distinguish between two different types of overflow updates

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla31

People

(Reporter: dbaron, Assigned: kip)

References

(Depends on 1 open bug, Blocks 3 open bugs)

Details

Attachments

(1 file, 8 obsolete files)

From when we introduced the UpdateOverflow hint in bug 524925 and across when it got moved in into the OverflowChangedTracker in bug 815666 in https://hg.mozilla.org/mozilla-central/rev/767accc64cf8 and https://hg.mozilla.org/mozilla-central/rev/0319dd845d53 , when we got an nsChangeHint_UpdateOverflow hint for a frame, we would update that frame's post-transform overflow area and then call nsIFrame::UpdateOverflow only on ancestors.

There are different ways we want to initiate overflow updates.  In some cases, such as changes of CSS transform, which are the most important case performance-wise, we want this behavior.  But there are other cases, such as the ones mats tried to do in bug 719177, we want to change a frame's own overflow area in other ways, and want to call nsIFrame::UpdateOverflow on that frame.

There are really three potential ways we might want to initiate UpdateOverflow on a frame:

(a) As mats wanted in bug 719177, we need to initiate the overflow updating process by calling UpdateOverflow on a frame that's had some change

(b) As we do for transforms, we want to initiate the process by starting with remapping the pre-transform overflow areas to post-transform overflow areas on the frame, and then start with UpdateOverflow on an ancestor.

(c) Possibly -- though I'm not sure -- in cases like position:sticky moving a child, we might want to start directly with nsIFrame::UpdateOverflow call on the parent.  (Is there actually a separate case here?  What if a sticky frame has a CSS transform?  Might that require updating the post-transform overflow anyway?  And does this case even work remotely sensibly at all???)  If this case actually exists, it probably doesn't need separate API since it can use the API for case (a) just called on the parent.  I'm also not sure if this case exists.

We've also consistently had very odd handling of a frame we initiate an UpdateOverflow on when the frame has no pre-transform overflow areas.  In particular, we assume that if a frame has no pre-transform overflow areas, we should call UpdateOverflow on it (case (a) above) whereas if a frame does have pre-transform overflow areas we should treat it as case (b) above.  This implicitly does something that should be explicit in the callers.  It's dangerous because if a caller requires behavior (a) this will give behavior (a) unless a frame has a transform -- so it might have something that appears to work, but breaks when used on a frame with transform.  (Bug 944291 fixed a variant of this -- the problem with coalescing between ancestors and descendants -- but didn't fix the case of explicit users.)  We should never do this implicitly -- callers should say which behavior they want.

So fixing this essentially means doing two things:

 (1) exposing to callers the CHILDREN_CHANGED vs STYLE_CHANGED distinction added in bug 944291 https://hg.mozilla.org/mozilla-central/rev/5e992c8ea14f

 (2) fixing the dangerous case above by avoiding the implicit fallback to behavior (a)

Once this is done we can proceed with bug 719177.
(And apologies for the incoherent writing, but it's too noisy in this work week room to hold an entire sentence in my head.)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8) from comment #0)
> We've also consistently had very odd handling of a frame we initiate an
> UpdateOverflow on when the frame has no pre-transform overflow areas.  In
> particular, we assume that if a frame has no pre-transform overflow areas,
> we should call UpdateOverflow on it (case (a) above) whereas if a frame does
> have pre-transform overflow areas we should treat it as case (b) above. 
> This implicitly does something that should be explicit in the callers.  It's
> dangerous because if a caller requires behavior (a) this will give behavior
> (a) unless a frame has a transform -- so it might have something that
> appears to work, but breaks when used on a frame with transform.  (Bug
> 944291 fixed a variant of this -- the problem with coalescing between
> ancestors and descendants -- but didn't fix the case of explicit users.)  We
> should never do this implicitly -- callers should say which behavior they
> want.

This problem was a regression from bug 720987 (https://hg.mozilla.org/mozilla-central/rev/731208933852 , partially backed out in https://hg.mozilla.org/mozilla-central/rev/fe7a433de8ff).
So I think this should also mean re-fixing bug 720987 correctly (and also bug 725664) by setting the InitialOverflowProperty more often, and using that in the OverflowChangedTracker instead of the PreTransformOverflowAreas.
(In reply to David Baron [:dbaron] (needinfo? me) (UTC+8, but slow response this week) from comment #3)
> So I think this should also mean re-fixing bug 720987 correctly (and also
> bug 725664) by setting the InitialOverflowProperty more often, and using
> that in the OverflowChangedTracker instead of the PreTransformOverflowAreas.

By "setting the InitialOverflowProperty more often" do you mean to change it's current behavior where it is "only set on frames that Preserve3D(), and when at least one of the overflow areas differs from the frame bound rect." and to always store it when nsIFrame::FinishAndStoreOverflow is called?
(In reply to :kip (Kearwood Gilbert) from comment #4)
> By "setting the InitialOverflowProperty more often" do you mean to change
> it's current behavior where it is "only set on frames that Preserve3D(), and
> when at least one of the overflow areas differs from the frame bound rect."
> and to always store it when nsIFrame::FinishAndStoreOverflow is called?

Yes on changing it from its current behavior -- but only by adding "|| hasTransform" to the condition, not by making it unconditional.
Attached patch V1 fix for Bug 984226 (obsolete) — Splinter Review
This also partially corrects Bug 926155
Attachment #8393971 - Flags: review?(dbaron)
I have submitted the patch to the try server: https://tbpl.mozilla.org/?tree=Try&rev=547fa503c608
Comment on attachment 8393971 [details] [diff] [review]
V1 fix for Bug 984226

Could you make the boolean parameter to AddFrame be an enum instead,
since booleans are unreadable at the caller?

(Using a boolean internally is ok, although using the enum internally
is ok too.)

>+   * Set aTransformOnly to True if the frame was explicitly added and hence
>+   * may have had a style change that changes its geometry relative to parent,
>+   * without reflowing. In this case we must update overflow on the frame's
>+   * parent even if this frame's overflow did not change.
>+   *
>+   * Set aTransformOnly to False if the overflow areas of children have changed
>+   * and we need to call UpdateOverflow on the frame.

I wouldn't include the "In this case we must update overflow on the
frame's parent even if this frame's overflow did not change."  That's
not part of the API contract; it's just something we don't bother
checking for (since UpdateOverflow returns whether the overflow changed,
whereas for this codepath we'd need to do so manually).  It might in
fact be worth checking that and avoiding the UpdateOverflow call on the
parent when the child's overflow didn't change.


Both callers in RestyleTracker should pass true (or its enum
replacement) for aTransformOnly.


Saner wrapping of the line you're changing in
nsIFrame::FinishAndStoreOverflow would be nice as well.


Marking review- because I'd like to have a look at the revised patch,
though otherwise this looks good.
Attachment #8393971 - Flags: review?(dbaron) → review-
> 
> Both callers in RestyleTracker should pass true (or its enum
> replacement) for aTransformOnly.
> 
Passing true to both callers results in the firing of the assertion in OverflowChangedTracker:

NS_ASSERTION(initial, "initial-overflow-areas required.");

I am investigating further to determine if the correct hints are being applied or if perhaps the assertion is firing incorrectly.
It looks like there have been some additional users of nsChangeHint_UpdateOverflow introduced.  So it probably needs to be split into nsChangeHint_UpdateOverflow and nsChangeHint_UpdatePostTransformOverflow, where transform changes only set nsChangeHint_UpdatePostTransformOverflow.

(I'm not sure when nsChangeHint_ChildrenOnlyTransform is used.)
Attached patch V2 fix for Bug 984226 (obsolete) — Splinter Review
Updated with feedback in Comment 8 and Comment 10.

I have been conservative in passing nsChangeHint_UpdateOverflow rather than nsChangeHint_UpdatePostTransformOverflow to avoid hitting the added assertion that validates that the InitialOverflowProperty is present.  Perhaps it will be possible to utilize this optimization more frequently with a bit of work and testing in places such as SVGTransformableElement::GetAttributeChangeHint
Attachment #8396765 - Flags: review?(dbaron)
Attachment #8393971 - Attachment is obsolete: true
I have submitted the V2 patch to the try server:

https://tbpl.mozilla.org/?tree=Try&rev=a9efc2f834b6
Comment on attachment 8396765 [details] [diff] [review]
V2 fix for Bug 984226

I think the commit message is probably more verbose than it needs to be.


>-        hint = NS_SubtractHint(hint,
>-                 NS_CombineHint(nsChangeHint_UpdateOverflow,
>-                                nsChangeHint_ChildrenOnlyTransform));
>+        hint = NS_SubtractHint(hint, nsChangeHint_UpdateOverflow);
>+        hint = NS_SubtractHint(hint, nsChangeHint_ChildrenOnlyTransform);
>+        hint = NS_SubtractHint(hint, nsChangeHint_UpdatePostTransformOverflow);

Maybe better to add to the existing structure, which guarantees
constant-folding?  Though hopefully optimizers can deal with this.


>       NS_ASSERTION(!(hint & nsChangeHint_ChildrenOnlyTransform) ||
>-                   (hint & nsChangeHint_UpdateOverflow),
>-                   "nsChangeHint_UpdateOverflow should be passed too");
>+                   (hint & nsChangeHint_UpdateOverflow) ||
>+                   (hint & nsChangeHint_UpdatePostTransformOverflow),
>+                   "nsChangeHint_UpdateOverflow or "
>+                   "nsChangeHint_UpdatePostTransformOverflow "
>+                   "should be passed too");

This assertion should be left as-is, I think.  I don't think it makes
sense to use UpdatePostTransformOverflow with ChildrenOnlyTransform.

>+      if (!didReflowThisFrame &&
>+         (hint & (nsChangeHint_UpdateOverflow |
>+                  nsChangeHint_UpdatePostTransformOverflow))) {

Indent the bottom two lines one more space.


The inner use of changeKind that processes ChildrenOnlyTransform should
probably just pass CHILDREN_CHANGED rather than using changeKind.  (We
could optimize it to use TRANSFORM_CHANGED in most cases (possibly even
all except when we add a ChildrenOnly transform), but I don't think it's
worth the bother.)


>+    /**
>+     * The frame was explicitly added and hence may have had a style change that
>+     * changes its geometry relative to parent, without reflowing.
>+     */
>+    TRANSFORM_CHANGED

Clarify this comment to make it clear that it was explicitly added as
a result of nsChangeHint_UpdatePostTransformOverflow?

>+    Entry(nsIFrame* aFrame, uint32_t aDepth, ChangeKind aChangeKind = CHILDREN_CHANGED)

Could you drop the default value of aChangeKind and make callers
pass it explicitly.  That's clearer.


>+    /**
>+     * True if the frame was explicitly added and hence may have had a style
>+     * change that changes its geometry relative to parent, without reflowing.
>+     * In this case we must update overflow on the frame's parent even if
>+     * this frame's overflow did not change.
>+     *
>+     * False if the overflow areas of children have changed so we need to call
>+     * UpdateOverflow on the frame.
>+     */
>+    ChangeKind mChangeKind;

Drop the comment; your comment defining ChangeKind covers this, and it's
not true/false anymore.

In nsChangeHint.h, could you add UpdatePostTransformOverflow next
to UpdateOverflow and bump the numbers of the higher ones, instead
of adding to the end?

StickyScrollContainer.cpp:
>+    if(f != aSubtreeRoot) {

space between "if" and "(".

nsFrame.cpp:

>+  if ((Preserves3D() || HasPerspective() || IsTransformed()) &&
>+                        (!aOverflowAreas.VisualOverflow().IsEqualEdges(bounds) ||
>                         !aOverflowAreas.ScrollableOverflow().IsEqualEdges(bounds))) {

Please fix the indent on the second and third lines here (the opening
parens should line up with the second opening paren on the first line).

nsStyleStruct.cpp:

The most important place to use UpdatePostTransformOverflow rather than UpdateOverflow is one that you missed:
>     if (!mSpecifiedTransform != !aOther.mSpecifiedTransform ||
>         (mSpecifiedTransform &&
>          *mSpecifiedTransform != *aOther.mSpecifiedTransform)) {
>       NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdateOverflow,
>                                          nsChangeHint_UpdateTransformLayer));
>     }

(But you can't use it on the one just before that, which handles changes
in HasTransformStyle().)


>     const nsChangeHint kUpdateOverflowAndRepaintHint =
>-      NS_CombineHint(nsChangeHint_UpdateOverflow, nsChangeHint_RepaintFrame);
>+      NS_CombineHint(nsChangeHint_UpdatePostTransformOverflow, nsChangeHint_RepaintFrame);

In order to do this, you'd need to condition adding the hint for
transform-origin changes on having a transform, and condition adding
the hint for perspective-origin changes on having a perspective.  I
think both are probably reasonable changes to make, though it might make
sense to do so in a separate bug and double-check with mattwoodrow.

(Also, I notice we probably don't handle perspective changes correctly,
since it requires updating the post-transform overflow area of the
*children*.  That's definitely a separate bug.)

nsStyleStruct.h:

nsStylePosition::MaxDifference shouldn't need to change since
you're not changing what nsStylePosition::CalcDifference does.

nsStyleDisplay::MaxDifference should be updated to add
UpdatePostTransformOverflow (as you do), but you shouldn't need to add
ChildrenOnlyTransform.

r=dbaron with those things fixed
Attachment #8396765 - Flags: review?(dbaron) → review+
I'm having trouble thinking of reftests that would be fixed by this patch -- it will definitely fix cases of visual overflow areas being too large in response to repeated changes, but given the interaction of the two broken bits (using pre-transform overflow rather than initial overflow, and taking the presence of a transform to mean that only the post-transform overflow needed updating, no matter what the origin of the change hint) I'm having trouble thinking of testable cases with bugs (i.e., incorrect scrollable overflow, or visual overflow area being too small).

Also, we should probably have a followup bug on adding nsChangeHint_UpdateParentOverflow, which we can use for the mOffset change handling, and which would post a CHILDREN_CHANGED overflow hint to the tracker for the *parent* frame.
> The most important place to use UpdatePostTransformOverflow rather than
> UpdateOverflow is one that you missed:
> >     if (!mSpecifiedTransform != !aOther.mSpecifiedTransform ||
> >         (mSpecifiedTransform &&
> >          *mSpecifiedTransform != *aOther.mSpecifiedTransform)) {
> >       NS_UpdateHint(hint, NS_CombineHint(nsChangeHint_UpdateOverflow,
> >                                          nsChangeHint_UpdateTransformLayer));
> >     }
> 
> (But you can't use it on the one just before that, which handles changes
> in HasTransformStyle().)

I have experimented with using nsChangeHint_UpdatePostTransformOverflow in this case, and it resulted in hitting the exception in RestyleTracker.h:

NS_ASSERTION(initial, "initial-overflow-areas required.");

In this test, I was using the page:

https://bug926155.bugzilla.mozilla.org/attachment.cgi?id=816315

The assertion hits shortly after the page has loaded.

Also of note is that when the assertion fires in this case, it is an nsBoxFrame containing a nsXULElement.  Perhaps the issue is with how often we set nsIFrame::InitialOverflowProperty?

I am continuing to investigate further.
As nsIFrame::FinishAndStoreOverflow only updates InitialOverflowProperty only if it is not equal to the frame bounds, functions such as nsIFrame::RecomputePerspectiveChildrenOverflow fall back to using the frame bounds when InitialOverflowProperty can not be found:

        nsOverflowAreas* overflow = 
          static_cast<nsOverflowAreas*>(child->Properties().Get(nsIFrame::InitialOverflowProperty()));
        nsRect bounds(nsPoint(0, 0), child->GetSize());
        if (overflow) {
          nsOverflowAreas overflowCopy = *overflow;
          child->FinishAndStoreOverflow(overflowCopy, bounds.Size());
        } else {
          nsOverflowAreas boundsOverflow;
          boundsOverflow.SetAllTo(bounds);
          child->FinishAndStoreOverflow(boundsOverflow, bounds.Size());
        }

Would it be appropriate to do something similar within OverflowChangedTracker::Flush?
Yes, it would.  Though it makes it harder to assert that we've done things correctly.

(Maybe, for the assertions, we can set a second frame property, #ifdef DEBUG, that we can then assert is present.)
(By a second frame property, I mean setting a property to probably a null value to indicate that we met the conditions for storing an InitialOverflowProperty() except for the condition of having non-default overflow to store.  And *removing* that property in FinishAndStoreOverflow if we don't meet the conditions!)
By updating OverflowChangedTracker::Flush as described in Comment 16, I may also be able to update SVGTransformableElement::GetAttributeChangeHint to return nsChangeHint_UpdatePostTransformOverflow as well in the branch with the comment, "// We just assume the old and new transforms are different.".
Yes, that seems fine.
Updating SVGTransformableElement::GetAttributeChangeHint to return nsChangeHint_UpdatePostTransformOverflow broke at least one reftest:

/layout/reftests/svg/smil/transform/scale-1.svg

This appears to be cased by nsSVGElement::DidAnimateTransformList() always passing nsIDOMMutationEvent::MODIFICATION to SVGTransformableElement::GetAttributeChangeHint, even when the animation is applying a transform for the first time.  The first call to FinishAndStoreOverflow must have the union of the children overflows passed as aOverflowAreas.  This is then stored by FinishAndStoreOverflow in the InitialOverflowProperty to be used in subsequent calls to FinishAndStoreOverflow that do not require reflowing the children.

I have updated nsSVGElement::DidAnimateTransformList() to differentiate between nsIDOMMutationEvent::MODIFICATION and nsIDOMMutationEvent::ADDITION in the same manner that Element::MaybeCheckSameAttrVal does, checking if the transform attribute value has already been set.

This has enabled the nsChangeHint_UpdatePostTransformOverflow for SVG without breaking the scale-1.svg reftest.

I am reviewing the other cases where I am setting nsChangeHint_UpdatePostTransformOverflow to ensure that similar issues are handled correctly.
The first nsChagneHint_UpdatePostTransformOverflow assigned in nsStyleDisplay::CalcDifference is hit by a reftest:

/layout/reftests/transform/stresstest-1.html

This reftest hits the fallback in OverflowChangedTracker::Flush as the first Flush has no InitialOverflowProperty set on the frame.  The reftest is passing with the frame bounds being passed in to the FinishAndStoreOverflow call.
Attached patch V3 Fix for Bug 984226 (obsolete) — Splinter Review
Work-in-progress fix
Attachment #8396765 - Attachment is obsolete: true
I have added a reftest to Bug 926155 that is corrected with the patch for this bug:

https://bugzilla.mozilla.org/attachment.cgi?id=8394585&action=edit
Attached patch V4 Fix for Bug 984226 (obsolete) — Splinter Review
I am flagging for review again after applying changes in :dbaron's feedback due to the additional changes made to SVGTransformableElement::GetAttributeChangeHint and nsSVGElement::DidAnimateTransformList described in Comment 21.

Please advise if these changes should be broken out to a separate patch / bug.
Attachment #8398204 - Attachment is obsolete: true
Attachment #8399852 - Flags: review?(dbaron)
Comment on attachment 8399852 [details] [diff] [review]
V4 Fix for Bug 984226

Could you wrap all but the first line of the commit message at less than
80 characters?  (The first line is special and should be all on one
line, but the rest should be wrapped.)



nsSVGElement.cpp:

space after the "if"

But, actually, I really don't think you want to use GetAttrInfo; instead
just call HasAttr() with the same parameters.

But are you sure the check works the way you expect?  Given the
method name being DidAnimateTransformList, I'd expect the attribute to
already be set (assuming that the animation operates through setting
attributes in the first place, which I'm not sure of).

I'm also just a little suspicious of it because it's doing something
differently from all the rest of the DidAnimate* functions.

Maybe it would be better to break out the SVG changes into a separate
patch, and ask for review from somebody who knows the SMIL animation
code better?




On the rest of the patch, though technically you didn't need to
ask for review again, one other comment:

In nsChangeHint.h, could you update the comment for
nsChangeHint_UpdateOverflow to change "frame's effect on its ancestors'
overflow area has changed" to "frame's overflow area has changed" and
remove the position/transform examples.

r=dbaron with that, if you move the nsSVGElement and
SVGTransformableElement changes to another bug.


FOLLOWUP:  We should remember to do a followup bug for changes to
position updating only the parent's overflow.
Attachment #8399852 - Flags: review?(dbaron) → review+
Assignee: nobody → kgilbert
Attached patch V5 Fix for Bug 984226, r=dbaron (obsolete) — Splinter Review
SVG related changes have been removed from the patch, to be added in a separate bug / patch.
Attachment #8399852 - Attachment is obsolete: true
Attached patch V6 Fix for Bug 984226, r=dbaron (obsolete) — Splinter Review
Updated comment based on review in Comment 27.
Attachment #8401099 - Attachment is obsolete: true
> Maybe it would be better to break out the SVG changes into a separate
> patch, and ask for review from somebody who knows the SMIL animation
> code better?

Bug 991545 has been added to break out the SVG changes
Blocks: 991545
Blocks: 992077
Blocks: 926155
> Also, we should probably have a followup bug on adding
> nsChangeHint_UpdateParentOverflow, which we can use for the mOffset change
> handling, and which would post a CHILDREN_CHANGED overflow hint to the
> tracker for the *parent* frame.

I have added a followup bug for adding nsChangeHint_UpdateParentOverflow, Bug 992077
Is this ready to land?  Do you want to add checkin-needed?
Flags: needinfo?(kgilbert)
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #32)
> Is this ready to land?  Do you want to add checkin-needed?

Ready to land; have added checkin-needed.
You should probably apply (in a separate bug; needinfo? me and I can vouch) for level 1 commit access so you have access to the try server, so you can test attempts to fix these failures before relanding.
Flags: needinfo?(kgilbert)
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #37)
> You should probably apply (in a separate bug; needinfo? me and I can vouch)
> for level 1 commit access so you have access to the try server, so you can
> test attempts to fix these failures before relanding.

Have try access already.  Have confirmed that crashtest asserts and reftest failures in Comment 35 and Comment 36 are reported in a try push.

It appears that many of these failures are due to logic that depended on the prior behavior of OverflowChangeTracker, which ignored the result of UpdateOverflow() for the frames passed in to OverflowChangeTracker::AddFrame.  The parents of these frames would cascade up when UpdateOverflow() returned true; however, the leaf-level frames passed in to AddFrame where always cascading up regardless of the return value.
Flags: needinfo?(kgilbert)
(In reply to :kip (Kearwood Gilbert) from comment #38)
> It appears that many of these failures are due to logic that depended on the
> prior behavior of OverflowChangeTracker, which ignored the result of
> UpdateOverflow() for the frames passed in to
> OverflowChangeTracker::AddFrame.  The parents of these frames would cascade
> up when UpdateOverflow() returned true; however, the leaf-level frames
> passed in to AddFrame where always cascading up regardless of the return
> value.

Ah, indeed.  I suppose it might make sense to temporarily undo that change, but try to do it again as part of bug 992077.  If you take that approach, please add a comment to that bug as a reminder (and explanation).
At least one of the crashtest failures was due to nsChangeHint_UpdatePostTransformOverflow being applied to the <html> element, which did not have the NS_FRAME_MAY_BE_TRANSFORMED state flag set.
Attached patch V7 Fix for Bug 984226 (obsolete) — Splinter Review
Updated patch to correct reftest and crashtest failures as described in Comment 28, Comment 29, and Comment 30.
Attachment #8401104 - Attachment is obsolete: true
I have submitted an updated patch to the try server to determine if the reftest and crashtest failures described in Comment 38, 39, and 40 are corrected without any knock-on effects.
(In reply to :kip (Kearwood Gilbert) from comment #41)
> Created attachment 8404321 [details] [diff] [review]
> V7 Fix for Bug 984226
> 
> Updated patch to correct reftest and crashtest failures as described in
> Comment 28, Comment 29, and Comment 30.

Correction: Failures are described in Comment 38, Comment 39, and Comment 30.
(In reply to :kip (Kearwood Gilbert) from comment #42)
> I have submitted an updated patch to the try server to determine if the
> reftest and crashtest failures described in Comment 38, 39, and 40 are
> corrected without any knock-on effects.

Try submitted:

https://tbpl.mozilla.org/?tree=Try&rev=0d6067f24141
(In reply to David Baron [:dbaron] (needinfo? me) (UTC-7) from comment #39)
> (In reply to :kip (Kearwood Gilbert) from comment #38)
> > It appears that many of these failures are due to logic that depended on the
> > prior behavior of OverflowChangeTracker, which ignored the result of
> > UpdateOverflow() for the frames passed in to
> > OverflowChangeTracker::AddFrame.  The parents of these frames would cascade
> > up when UpdateOverflow() returned true; however, the leaf-level frames
> > passed in to AddFrame where always cascading up regardless of the return
> > value.
> 
> Ah, indeed.  I suppose it might make sense to temporarily undo that change,
> but try to do it again as part of bug 992077.  If you take that approach,
> please add a comment to that bug as a reminder (and explanation).

In the updated patch, V7 Fix for Bug 984226, I have added OverflowChangedTracker::CHILDREN_AND_PARENT_CHANGED, which emulates the prior behavior of OverflowChangedTracker::Flush for frames passed to OverflowChangedTracker::AddFrame that did not have a PreTransformOverflowAreasProperty.

I have added Bug 992077, Comment 1 as an explanation and reminder to correct the dependent code and eliminate OverflowChangedTracker::CHILDREN_AND_PARENT_CHANGED.
One of the crashtests was expecting an assertion to ensure that the assertion was not removed until the underlying cause for Bug 973322 was fixed.  It appears that the "V7 Fix for Bug 984226" patch corrects the underlying cause for Bug 973322.

The workaround applied in Bug 973322 results in the MathML output being suppressed when sticky is enabled to avoid crashing the browser.  The the "V7 Fix for Bug 984226" patch in place, the MathML output is properly displayed, matching the production FX 29.0 build and the assertion no longer occurs.

When Bug 984226 lands, perhaps we can update the layout/mathml/crashtests/crashtests.list manifest to expect "0" asserts for 973322-1.xhtml.
One of the crashtests was configured to expect an assertion, ensuring that the assertion was not removed until the underlying cause for Bug 973322 was fixed.  It appears that the "V7 Fix for Bug 984226" patch corrects the underlying cause for Bug 973322.

The workaround applied in Bug 973322 avoids crashing the browser by suppressing MathML rendering when the reflow is interrupted by sticky positioning bugs.  The the "V7 Fix for Bug 984226" patch in place, the reflow is no longer interrupted, MathML output is properly displayed, and the assertion no longer occurs.

When Bug 984226 is ready to land, perhaps we can update the layout/mathml/crashtests/crashtests.list manifest to expect "0" asserts for 973322-1.xhtml.
Did you want me to review the revised patch?

(Also, I find it a little confusing to say "bug 984226" in this bug, rather than just "this bug", since it requires mental work to discover that it's a self-reference.)
Flags: needinfo?(kgilbert)
Comment on attachment 8404321 [details] [diff] [review]
V7 Fix for Bug 984226

This patch has been run through he try server with better results.  Thanks for reviewing it.
Attachment #8404321 - Flags: review?(dbaron)
Flags: needinfo?(kgilbert)
Attached patch V8 Fix for Bug 984226 (obsolete) — Splinter Review
The patch has been updated to eliminate the MathML crashtest failure by expecting "0" asserts rather than "1" assert.

Please see Comment 47 for more details.
Attachment #8404321 - Attachment is obsolete: true
Attachment #8404321 - Flags: review?(dbaron)
Attachment #8405578 - Flags: review?(dbaron)
Comment on attachment 8405578 [details] [diff] [review]
V8 Fix for Bug 984226

Submitted to try to confirm that the crashtest goes green with the manifest update:

https://tbpl.mozilla.org/?tree=Try&rev=058a790f25b4
Comment on attachment 8405578 [details] [diff] [review]
V8 Fix for Bug 984226

>+        // Frame can not be transformed, and thus can not use the
>+        // nsChangeHint_UpdatePostTransformOverflow optimization.

Well, it's not that it can't use the optimization, but really that if it
can't be transformed, then a change to transform has no effect so you
don't need to do anything.  (Since what you're doing here is doing less
work, not more.)  So I think the comment here should be adjusted to
reflect that.


>+    } else if (aChangeKind == CHILDREN_AND_PARENT_CHANGED) {
>+      // Update existing entry.  CHILDREN_AND_PARENT_CHANGED is stronger than
>+      // TRANSFORM_CHANGED and CHILDREN_CHANGED and will replace the value.
>+      entry->mChangeKind = CHILDREN_AND_PARENT_CHANGED;
>+    } else if (aChangeKind == CHILDREN_CHANGED &&
>+               entry->mChangeKind != CHILDREN_AND_PARENT_CHANGED) {
>+      // Update existing entry.  CHILDREN_CHANGED is stronger than
>+      // TRANSFORM_CHANGED and will replace the value.
>+      entry->mChangeKind = CHILDREN_CHANGED;
>     }

It would probably be simpler to put the entries in the enum in order
(either TRANSFORM_CHANGED / CHILDREN_CHANGED / CHILDREN_AND_PARENT_CHANGED
or the reverse order) and use std::max() or std::min() as appropriate.


FOLLOWUP: Also, we should have a followup on using the return value
from FinishAndStoreOverflow for the TRANSFORM_CHANGED hint as well.


r=dbaron with that; sorry for the delay
Attachment #8405578 - Flags: review?(dbaron) → review+
Updated with feedback in Comment 52
Attachment #8405578 - Attachment is obsolete: true
Comment on attachment 8407052 [details] [diff] [review]
V9 Fix for Bug 984226,r=dbaron

Updated patch submitted to try:

https://tbpl.mozilla.org/?tree=Try&rev=d0253985d4a2

Once this passes try, I will set checkin-needed on this bug.
Blocks: 996800
> FOLLOWUP: Also, we should have a followup on using the return value
> from FinishAndStoreOverflow for the TRANSFORM_CHANGED hint as well.
> 
I have created a followup bug to capture this, Bug 996800.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/0d8ac5b08dd5
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Depends on: 1001237
Depends on: 1005405
Depends on: 1019992
You need to log in before you can comment on or make changes to this bug.