Closed
Bug 984226
Opened 10 years ago
Closed 10 years ago
OverflowChangedTracker should distinguish between two different types of overflow updates
Categories
(Core :: Layout, defect)
Core
Layout
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)
27.05 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•10 years ago
|
||
(And apologies for the incoherent writing, but it's too noisy in this work week room to hold an entire sentence in my head.)
Reporter | ||
Comment 2•10 years ago
|
||
(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).
Reporter | ||
Comment 3•10 years ago
|
||
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.
Assignee | ||
Comment 4•10 years ago
|
||
(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?
Reporter | ||
Comment 5•10 years ago
|
||
(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.
Assignee | ||
Comment 6•10 years ago
|
||
This also partially corrects Bug 926155
Attachment #8393971 -
Flags: review?(dbaron)
Assignee | ||
Comment 7•10 years ago
|
||
I have submitted the patch to the try server: https://tbpl.mozilla.org/?tree=Try&rev=547fa503c608
Reporter | ||
Comment 8•10 years ago
|
||
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-
Assignee | ||
Comment 9•10 years ago
|
||
>
> 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.
Reporter | ||
Comment 10•10 years ago
|
||
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.)
Assignee | ||
Comment 11•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Attachment #8393971 -
Attachment is obsolete: true
Assignee | ||
Comment 12•10 years ago
|
||
I have submitted the V2 patch to the try server: https://tbpl.mozilla.org/?tree=Try&rev=a9efc2f834b6
Reporter | ||
Comment 13•10 years ago
|
||
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+
Reporter | ||
Comment 14•10 years ago
|
||
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.
Assignee | ||
Comment 15•10 years ago
|
||
> 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.
Assignee | ||
Comment 16•10 years ago
|
||
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?
Reporter | ||
Comment 17•10 years ago
|
||
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.)
Reporter | ||
Comment 18•10 years ago
|
||
(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!)
Assignee | ||
Comment 19•10 years ago
|
||
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.".
Reporter | ||
Comment 20•10 years ago
|
||
Yes, that seems fine.
Assignee | ||
Comment 21•10 years ago
|
||
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.
Assignee | ||
Comment 22•10 years ago
|
||
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.
Assignee | ||
Comment 23•10 years ago
|
||
Work-in-progress fix
Attachment #8396765 -
Attachment is obsolete: true
Assignee | ||
Comment 24•10 years ago
|
||
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
Assignee | ||
Comment 25•10 years ago
|
||
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)
Assignee | ||
Comment 26•10 years ago
|
||
V4 Fix pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=9a3d2ad170d7
Reporter | ||
Comment 27•10 years ago
|
||
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+
Reporter | ||
Updated•10 years ago
|
Assignee: nobody → kgilbert
Assignee | ||
Comment 28•10 years ago
|
||
SVG related changes have been removed from the patch, to be added in a separate bug / patch.
Attachment #8399852 -
Attachment is obsolete: true
Assignee | ||
Comment 29•10 years ago
|
||
Updated comment based on review in Comment 27.
Attachment #8401099 -
Attachment is obsolete: true
Assignee | ||
Comment 30•10 years ago
|
||
> 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
Assignee | ||
Comment 31•10 years ago
|
||
> 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
Reporter | ||
Comment 32•10 years ago
|
||
Is this ready to land? Do you want to add checkin-needed?
Flags: needinfo?(kgilbert)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(kgilbert)
Keywords: checkin-needed
Assignee | ||
Comment 33•10 years ago
|
||
(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.
Comment 35•10 years ago
|
||
Backed out for crashtest asserts. https://hg.mozilla.org/integration/mozilla-inbound/rev/44732d1a525a https://tbpl.mozilla.org/php/getParsedLog.php?id=37460376&tree=Mozilla-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=37459751&tree=Mozilla-Inbound
Comment 36•10 years ago
|
||
And reftest failures across all platforms. https://tbpl.mozilla.org/php/getParsedLog.php?id=37461452&tree=Mozilla-Inbound
Reporter | ||
Comment 37•10 years ago
|
||
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)
Assignee | ||
Comment 38•10 years ago
|
||
(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)
Reporter | ||
Comment 39•10 years ago
|
||
(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).
Assignee | ||
Comment 40•10 years ago
|
||
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.
Assignee | ||
Comment 41•10 years ago
|
||
Updated patch to correct reftest and crashtest failures as described in Comment 28, Comment 29, and Comment 30.
Attachment #8401104 -
Attachment is obsolete: true
Assignee | ||
Comment 42•10 years ago
|
||
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.
Assignee | ||
Comment 43•10 years ago
|
||
(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.
Assignee | ||
Comment 44•10 years ago
|
||
(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
Assignee | ||
Comment 45•10 years ago
|
||
(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.
Assignee | ||
Comment 46•10 years ago
|
||
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.
Assignee | ||
Comment 47•10 years ago
|
||
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.
Reporter | ||
Comment 48•10 years ago
|
||
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)
Assignee | ||
Comment 49•10 years ago
|
||
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)
Assignee | ||
Comment 50•10 years ago
|
||
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)
Assignee | ||
Comment 51•10 years ago
|
||
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
Reporter | ||
Comment 52•10 years ago
|
||
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+
Assignee | ||
Comment 53•10 years ago
|
||
Updated with feedback in Comment 52
Attachment #8405578 -
Attachment is obsolete: true
Assignee | ||
Comment 54•10 years ago
|
||
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.
Assignee | ||
Comment 55•10 years ago
|
||
> 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.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 56•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/0d8ac5b08dd5
Keywords: checkin-needed
Comment 57•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/0d8ac5b08dd5
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
You need to log in
before you can comment on or make changes to this bug.
Description
•