Jesse Ruderman Reporter
	Description • 11 years ago

With:
  user_pref("layout.css.sticky.enabled", true);

###!!! ASSERTION: Should be starting from the first continuation: 'nsLayoutUtils::IsFirstContinuationOrSpecialSibling(aFrame)', file layout/generic/StickyScrollContainer.cpp, line 283

###!!! ASSERTION: Can't sticky position individual continuations: 'nsLayoutUtils::IsFirstContinuationOrSpecialSibling(aFrame)', file layout/generic/StickyScrollContainer.cpp, line 133

(Bug 918994 had similar symptoms, but is gone now.)

	Corey Ford [:coyotebush]
	Comment 2 • 11 years ago

As of bug 828312 patches 9a/9b, if I understand correctly, RecomputePosition *should* always be passing a FirstContinuationOrSpecialSibling to StickyScrollContainer::PositionContinuations. So I suppose it's more properly a problem with that guarantee.

	:kip (Kearwood Gilbert) Assignee
	Comment 5 • 11 years ago

Comment on attachment 8409920 [details] [diff] [review]
V1 Fix for Bug 926728

Due to ib-splits or pseudo-elements, RestyleManager::ComputeStyleChangeFor will add multiple frames of a sticky element to the nsStyleChangeList, resulting in StickyScrollContainer::PositionContinuations being called on continuations or ib-split-siblings other than the first, triggering the assert.  In this case, the first continuation or ib-split-sibling has already been passed into StickyScrollContainer::PositionContinuations, positioning all continuations and ib-split-siblings of the sticky already; additional calls to StickyScrollContainer::PositionContinuations for the frame can be skipped.
A condition that suppressed extraneous StickyScrollContainer::PositionContinuation calls in RestyleManager::RecomputePosition was removed in Bug 828312 Patch 9b; however this check was only checking for prior continuations and was not aware of ib-split-siblings.
This bug can be corrected by restoring condition within RestyleManager::RecomputePosition, implemented with nsLayoutUtils::IsFirstContinuationOrIBSplitSibling.
The existing comment within RestyleManager::RecomputePosition indicates that this is the expected behavior:
// Update sticky positioning for an entire element at once when
// RecomputePosition is called with the first continuation in a chain.

	:kip (Kearwood Gilbert) Assignee
	Comment 6 • 11 years ago

Fixed logic inversion.

	:kip (Kearwood Gilbert) Assignee
	Comment 7 • 11 years ago

Comment on attachment 8409942 [details] [diff] [review]
V2 Fix for Bug 926728

Pushed to try:

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

	:kip (Kearwood Gilbert) Assignee
	Comment 8 • 11 years ago

Added crashtest to ensure that the "Should be starting from the first continuation" assertion is not fired when recalculating the position of a sticky with ib-split siblings.

	Corey Ford [:coyotebush]
	Comment 9 • 11 years ago

That explanation makes sense. I'll try to look at the code before the end of the week.

	Corey Ford [:coyotebush]
	Comment 10 • 11 years ago

Comment on attachment 8410398 [details] [diff] [review]
Crashtest for Bug 926728

Review of attachment 8410398 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/crashtests/crashtests.list
@@ +428,5 @@
>  load 931460-1.html
>  load 935765-1.html
>  load 942690.html
>  load 973390-1.html
> +load 926728.html

Inserting this before 931464.html would keep the file a little more sorted.

	Corey Ford [:coyotebush]
	Comment 11 • 11 years ago

(In reply to :kip (Kearwood Gilbert) from comment #5)
> Due to ib-splits or pseudo-elements, RestyleManager::ComputeStyleChangeFor
> will add multiple frames of a sticky element to the nsStyleChangeList

Weren't the patches in bug 828312 intended to change that? Or maybe they only addressed true continuations and not ib-splits? The position:relative code in RecomputePosition, for instance, also appears designed for the case of IsFirstContinuationOrIBSplitSibling(aFrame), so it seems like it would be preferable to avoid passing later continuations/ib-split-siblings into RecomputePosition at all.

	:kip (Kearwood Gilbert) Assignee
	Comment 12 • 11 years ago

Corrected order of entries in crashtests.list

	Corey Ford [:coyotebush]
	Comment 13 • 11 years ago

Comment on attachment 8412705 [details] [diff] [review]
V2 Crashtest for Bug 926728,r=coreyf

> Bug 926728 - Added crashtest

No need for the extra request, but I noticed that this patch subject should be more descriptive ("crashtest for recalculating..." as you have in the body, perhaps?) and have r=corey.

	:kip (Kearwood Gilbert) Assignee
	Comment 14 • 11 years ago

I have stepped through RestyleManager::ComputeStyleChangeFor and saw that the logic there suppresses multiple continuations with the same style; however, it will always process the first continuation of all ib-split-siblings.

if GetPrevContinuationWithSameStyle can potentially return false for a continuation other than the first (which the logic in RestyleManager::ComputeStyleChangeFor appears to expect), then it is possible that multiple continuations within a single ib-split-sibling or non-ib-split continuation could be passed through.

	:kip (Kearwood Gilbert) Assignee
	Comment 15 • 11 years ago

Updated description in patch

	Corey Ford [:coyotebush]
	Comment 16 • 11 years ago

Having browser RestyleManager.cpp some more, I see that multiple continuations and ib-split siblings of the same element can certainly be added to the change list. That makes sense when they have different styles (say, when ::first-line is involved).

But assuming (as the sticky code already does, actually) that continuations/ib-split siblings can't have different *positioning-related* styles, I wonder whether it would make any sense to skip setting nsChangeHint_RecomputePosition for anything but the first continuation?

	Corey Ford [:coyotebush]
	Comment 17 • 11 years ago

> That makes sense when they have different styles (say, when ::first-line is involved).

which line of thinking leads me to a testcase that triggers the assertions without involving ib-splits. (But the current patch still fixes this one too.)

	Corey Ford [:coyotebush]
	Comment 18 • 11 years ago

Comment on attachment 8409942 [details] [diff] [review]
V2 Fix for Bug 926728

In any case, I still suspect it would be nicer to place this check at some earlier stage. (At the very least, it should cover the relpos logic too, but as I noted it might better belong in the change hint handling.)

	:kip (Kearwood Gilbert) Assignee
	Comment 19 • 11 years ago

Removed redundant nested "if" block

	Corey Ford [:coyotebush]
	Comment 20 • 11 years ago

Comment on attachment 8417504 [details] [diff] [review]
V3 Fix for Bug 926728,r=corey

So since it sounds like change-hint refactoring could be a bigger project, this is a fine solution for now.

The other thing I was just thinking about is that you could consider adding a code comment to keep track of the fact that we probably shouldn't even be calling RecomputePosition in the situation where this test is false. A followup bug suffices too.

r=me regardless.

	David Baron :dbaron:
	Comment 21 • 11 years ago

I actually think we should fix this one differently, by just updating the caller to get the first continuation or IB sibling in this case (i.e., patching the same code, but to get the first instead of condition on being first).  I think this is a rare case, and it's a lot easier to guarantee that we're going to do the right thing both now and in the future if we just do the work twice in this edge case (of having a sticky inside a situation where styles change across continuations).

	Corey Ford [:coyotebush]
	Comment 22 • 11 years ago

I suppose that would work fine too. Although it seems unlikely to me that we would ever *not* call RecomputePosition on the first.

	:kip (Kearwood Gilbert) Assignee
	Comment 23 • 11 years ago

Updated based on Comment 21

Rather than skipping the call to StickyScrollContainer::PositionContinuation for continuations other than the first, the first continuation of the first ib-split-sibling for the passed frame is used every time within RestyleManager::RecomputePosition.

	:kip (Kearwood Gilbert) Assignee
	Comment 24 • 11 years ago

Comment on attachment 8417652 [details] [diff] [review]
V4 Fix for Bug 926728

Pushed to try:

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

	Corey Ford [:coyotebush]
	Comment 25 • 11 years ago

Comment on attachment 8417652 [details] [diff] [review]
V4 Fix for Bug 926728

This makes the comment a little inaccurate, and I guess the ComputeStickyOffsets call should probably be changed too? (I don't think there's any use storing offsets on aFrame when aFrame != firstFrame.)

	:kip (Kearwood Gilbert) Assignee
	Comment 26 • 11 years ago

Updated based on Comment 25, re-submitted to try:

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

	David Baron :dbaron:
	Comment 27 • 11 years ago

Comment on attachment 8417663 [details] [diff] [review]
V5 Fix for Bug 926728

I'd suggest calling the variable firstContinuation rather than firstFrame, and maybe expanding the comment to say that it's rare that the frame we already have isn't already the first continuation or IB sibling, but it can happen when we're inside of a situation where styles differ across continuations such as ::first-line or ::first-letter, and in those cases we will generally (but maybe not always) do the work twice

r=dbaron

	:kip (Kearwood Gilbert) Assignee
	Comment 28 • 11 years ago

Updated with feedback from Comment 27

	Ryan VanderMeulen [:RyanVM]
	Comment 29 • 11 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6f4bd1dcbc0f
https://hg.mozilla.org/integration/mozilla-inbound/rev/c77242ba4613

	Ryan VanderMeulen [:RyanVM]
	Comment 30 • 11 years ago

Follow-up to explicitly set the sticky enabled pref so this test will still run as intended after it merges to beta.
https://hg.mozilla.org/integration/mozilla-inbound/rev/88a8ca8a7f0c

	Carsten Book [:Tomcat]
	Comment 31 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/6f4bd1dcbc0f
https://hg.mozilla.org/mozilla-central/rev/c77242ba4613
https://hg.mozilla.org/mozilla-central/rev/88a8ca8a7f0c