1364275 - counter-reset on ::-moz-range-track and ::-moz-range-progress causes Assertion failure: cmp != 0 (same content, different frames), at nsGenConList.cpp:105

Status: NEW

(Core :: Layout, defect, P3)

Description

[Jason Kratzer [:jkratzer]]

Attachment: Testcase

Testcase found by fuzzing mozilla-central rev 20170511-3b96f2773257.

Assertion failure: cmp != 0 (same content, different frames), at /home/worker/workspace/build/src/layout/base/nsGenConList.cpp:105

ASAN:DEADLYSIGNAL
=================================================================
==22431==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f8f5919103f bp 0x7ffc6d2d2590 sp 0x7ffc6d2d24c0 T0)
==22431==The signal is caused by a WRITE memory access.
==22431==Hint: address points to the zero page.
    #0 0x7f8f5919103e in nsGenConList::NodeAfter(nsGenConNode const*, nsGenConNode const*) /home/worker/workspace/build/src/layout/base/nsGenConList.cpp:105:3
    #1 0x7f8f59191252 in nsGenConList::Insert(nsGenConNode*) /home/worker/workspace/build/src/layout/base/nsGenConList.cpp:113:26
    #2 0x7f8f59174561 in nsCounterList::Insert(nsCounterNode*) /home/worker/workspace/build/src/layout/base/nsCounterManager.h:180:23
    #3 0x7f8f5917579d in nsCounterManager::AddResetOrIncrement(nsIFrame*, int, nsStyleCounterData const&, nsCounterNode::Type) /home/worker/workspace/build/src/layout/base/nsCounterManager.cpp:241:16
    #4 0x7f8f59154d4f in nsCounterManager::AddCounterResetsAndIncrements(nsIFrame*) /home/worker/workspace/build/src/layout/base/nsCounterManager.cpp:222:16
    #5 0x7f8f591464b8 in nsCSSFrameConstructor::InitAndRestoreFrame(nsFrameConstructorState const&, nsIContent*, nsContainerFrame*, nsIFrame*, bool) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:5016:23
    #6 0x7f8f5914ba92 in nsCSSFrameConstructor::ConstructBlock(nsFrameConstructorState&, nsIContent*, nsContainerFrame*, nsContainerFrame*, nsStyleContext*, nsContainerFrame**, nsFrameItems&, nsIFrame*, PendingBinding*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:12152:3
    #7 0x7f8f5915058c in nsCSSFrameConstructor::ConstructNonScrollableBlockWithConstructor(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&, nsBlockFrame* (*)(nsIPresShell*, nsStyleContext*)) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4984:3
    #8 0x7f8f591534a7 in nsCSSFrameConstructor::ConstructNonScrollableBlock(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItem&, nsContainerFrame*, nsStyleDisplay const*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4948:10
    #9 0x7f8f591522b8 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:3898:7
    #10 0x7f8f591589a0 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6256:3
    #11 0x7f8f591467d5 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10877:5
    #12 0x7f8f59147221 in nsCSSFrameConstructor::ProcessChildren(nsFrameConstructorState&, nsIContent*, nsStyleContext*, nsContainerFrame*, bool, nsFrameItems&, bool, PendingBinding*, nsIFrame*) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:11180:3
    #13 0x7f8f59152a23 in nsCSSFrameConstructor::ConstructFrameFromItemInternal(nsCSSFrameConstructor::FrameConstructionItem&, nsFrameConstructorState&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:4080:9
    #14 0x7f8f591589a0 in nsCSSFrameConstructor::ConstructFramesFromItem(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList::Iterator&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:6256:3
    #15 0x7f8f591467d5 in nsCSSFrameConstructor::ConstructFramesFromItemList(nsFrameConstructorState&, nsCSSFrameConstructor::FrameConstructionItemList&, nsContainerFrame*, nsFrameItems&) /home/worker/workspace/build/src/layout/base/nsCSSFrameConstructor.cpp:10877:5

Comment 1

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

jwatt, could you have a look?

Comment 2

[Ryan VanderMeulen [:RyanVM]]

Still reproducible on trunk with or without Stylo enabled. Reproduces more than a year back (the furthest mozregression can bisect debug builds).

Comment 3

[Firefox Bug Husbandry Bot]

https://wiki.mozilla.org/Bug_Triage/Projects/Bug_Handling/Bug_Husbandry#Move_fix-optionals

Comment 4

[Jonathan Watt [:jwatt]]

FWIW the assertion was originally added in bug 24861:

https://github.com/mozilla/gecko-dev/commit/fcaa82359f672f3390e9043236756e6b200174ac

Comment 5

[Jonathan Watt [:jwatt]]

The testcase contains an <input type=range> and the style:

  *::-moz-range-track, *::-moz-range-progress {
    counter-reset: bar !important;
  }

Both those pseudo-elements are created as NAC for the range.

We assert under the stack:

  nsGenConList::NodeAfter
  nsGenConList::Insert
  nsCounterList::Insert
  nsCounterManager::AddResetOrIncrement
  nsCounterManager::AddCounterResetsAndIncrements
  nsCSSFrameConstructor::InitAndRestoreFrame

at the code:

  int32_t cmp = nsLayoutUtils::DoCompareTreePosition(content1, content2,
                                                     pseudoType1, -pseudoType2);
  MOZ_ASSERT(cmp != 0, "same content, different frames");

nsLayoutUtils::DoCompareTreePosition returns 0 because at the end it recognizes that the two nsIContents passed in are siblings so it uses FragmentOrElement::ComputeIndexOf to calculate their relative index, but ComputeIndexOf doesn't handle anonymous content (it returns -1 for both calls), and we end up returning from DoCompareTreePosition from the block:

  if (index1 < 0 || index2 < 0) {
    // one of them must be anonymous; we can't determine the order
    return 0;
  }

https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/layout/base/nsLayoutUtils.cpp#1696

Comment 6

[Jonathan Watt [:jwatt]]

There's a variant of nsLayoutUtils::DoCompareTreePosition that takes nsIFrame* instead of nsIContent* which looks like it should do the right thing:

https://dxr.mozilla.org/mozilla-central/rev/681eb7dfa324dd50403c382888929ea8b8b11b00/layout/base/nsLayoutUtils.cpp#1750

This requires an ancestor nsTArray<nsIFrame*> to be calculated ahead of time and passed in though, and it's unclear to me whether this variant is slower. If it is it's probably not worth using. Supporting 'counter-reset' for two pseudo-elements of the same NAC parent seems like an extreme edge case.

Comment 7

[Jonathan Watt [:jwatt]]

(In reply to Jonathan Watt [:jwatt] (mostly away until Tue 10th) from comment #5)
>   if (index1 < 0 || index2 < 0) {
>     // one of them must be anonymous; we can't determine the order
>     return 0;
>   }

I suppose we could make this `if` block smarter without impacting perf in the common case.

Comment 8

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Watt [:jwatt] (mostly away until Tue 10th) from comment #6)
> There's a variant of nsLayoutUtils::DoCompareTreePosition that takes
> nsIFrame* instead of nsIContent* which looks like it should do the right
> thing:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> 681eb7dfa324dd50403c382888929ea8b8b11b00/layout/base/nsLayoutUtils.cpp#1750
> 
> This requires an ancestor nsTArray<nsIFrame*> to be calculated ahead of time
> and passed in though, and it's unclear to me whether this variant is slower.
> If it is it's probably not worth using. Supporting 'counter-reset' for two
> pseudo-elements of the same NAC parent seems like an extreme edge case.

It would not. First because it's bogus (see bug 1427635), and second because at the time we recalc counters the frame tree is not completely setup.

I don't think we should support counters on pseudo-elements other than ::before and ::after. It would potentially expose the DOM structure we use for NAC, which is an anti-goal.

Comment 9

[Jonathan Watt [:jwatt]]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #8)
> I don't think we should support counters on pseudo-elements other than
> ::before and ::after.

Seems reasonable.

> It would potentially expose the DOM structure we use
> for NAC, which is an anti-goal.

If we expose a pseudo-element to content then we've already exposed the DOM structure we use for NAC, regardless of whether or not counters are supported on that pseudo-element, no?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

(In reply to Jonathan Watt [:jwatt] (mostly away until Tue 10th) from comment #9)
> If we expose a pseudo-element to content then we've already exposed the DOM
> structure we use for NAC, regardless of whether or not counters are
> supported on that pseudo-element, no?

No, because we don't follow the normal inheritance chain, which would be what would expose something, unless I'm misunderstanding what you mean.