Open Bug 1524064 Opened 5 years ago Updated 3 days ago

###!!! ASSERTION: overflow rect must include border rect, and the clamping logic here depends on that: 'overflowRect.Contains(borderRect)', file layout/generic/ScrollAnchorContainer.cpp, line 118

Categories

(Core :: Layout: Scrolling and Overflow, defect, P3)

Product:
Core
Component:
Layout: Scrolling and Overflow
Type:
defect

Tracking

()

Tracking Flags:
Tracking Status
firefox67 --- affected

People

(Reporter: dholbert, Unassigned)

References

(Blocks 2 open bugs)

Details

(4 keywords, Whiteboard: [stockwell unknown])

Attachments

(2 files, 1 obsolete file)

reduced testcase 1
323 bytes, text/html
Details
Bug 1524064: annotate crashtest 1547420-1.html as failing one extra assertion on Android. (no review, just a test-manifest update to reflect reality)
48 bytes, text/x-phabricator-request
Details | Review

STR:

  1. Log into linkedin with valid credentials, using a debug build of Firefox.
  2. Visit https://www.linkedin.com/jobs/ (this should show a grid of job postings)
  3. Scroll the page

ACTUAL RESULTS:
With each scroll tick, this appears in my terminal:

###!!! ASSERTION: overflow rect must include border rect, and the clamping logic here depends on that: 'overflowRect.Contains(borderRect)', file /scratch/work/builds/mozilla-central/mozilla/layout/generic/ScrollAnchorContainer.cpp, line 118

EXPECTED RESULTS:
No assertion failure.

If I break on the assertion failure, I can see the following:
overflowRect has x,y,w,h=0
borderRect has x,y=0 and w,h=60,60
aCandidateFrame (the thing whose rects we're considering) is an abspos scrollable frame with "height:1px; width:1px"

Attached file reduced testcase 1 —

Here's a reduced testcase that triggers this issue for me. It's using the "clip" property which apparently reduces the overflow areas (even down to being 0-sized) without reducing the size of the border rect.

Here's a partial frame dump of reduced testcase 1 -- notice the zero-sized vis-overflow and scr-overflow areas (0,0,0,0) despite nonzero-sized border box (height & width of 600 i.e. 10px)

    AbsoluteList 7f1d586a38e0 <
      HTMLScroll(div)(1)@7f1d58e1ab80 parent=7f1d58e1a0b0 {480,480,600,600} vis-overflow=0,0,0,0 scr-overflow=0,0,0,0 [state=0080068000000100] [content=7f1d4ce65ca0] [cs=7f1d34407408]<
        Block(div)(1)@7f1d58e1adb0 parent=7f1d58e1ab80 {0,0,600,600} [state=000000a000d00200] [content=7f1d4ce65ca0] [cs=7f1d4d783808:-moz-scrolled-content]<
        >
      >
    >

(So, the question here is, does our logic do the right thing for this clipped scenario? If so, great, and we should perhaps remove the assertion. If not, we perhaps need to adjust the logic a bit.)

Flags: needinfo?(rhunt)
Keywords: testcase

So if I understand correctly, adding a clip can reduce how we calculate the scrollable overflow rect.

This seems like it would affect us if the clip reduces the scrollable overflow rect to be zero sized, making something unable to be a scroll anchor that otherwise could be.

I modified your test case a bit to try and get this to happen [1]. We're unable to select a scroll anchor in that case and adjust with the yellow animation. Chrome actually selects an anchor here, so it looks like they don't pay attention to 'clip' here. We probably should do the same.

[1] https://eqrion.github.io/web-tests/scrolling/scroll-anchor-overflow-clip.html

Flags: needinfo?(rhunt)

(In reply to Ryan Hunt [:rhunt] from comment #4)

So if I understand correctly, adding a clip can reduce how we calculate the scrollable overflow rect.

I think so. Clipped-away content is still sized & positioned the same, but it doesn't create scrollable or visual overflow, because it's not painted.

This seems like it would affect us if the clip reduces the scrollable overflow rect to be zero sized, making something unable to be a scroll anchor that otherwise could be.

There's one other edge case to consider: what if the clip styling reduces the scrollable overflow rect partially, but enough so that the portion intersecting the scrollframe is entirely clipped away?

Also: this seems somewhat related to https://github.com/w3c/csswg-drafts/issues/3483

I modified your test case a bit to try and get this to happen [1]. We're unable to select a scroll anchor in that case and adjust with the yellow animation. Chrome actually selects an anchor here, so it looks like they don't pay attention to 'clip' here. We probably should do the same.

I tend to think our behavior makes sense, and Chrome's behavior is probably a bug or at least not intentional.

It's odd to me that Chrome rejects some elements with 0-sized scrollable overflow areas but apparently accepts others per your observations here. I think it makes sense to stick to having scrollable-overflow-boxes be the key consideration here, and it seems like we should avoid adding exceptions and special cases unless there's very good reason.

So, I tend to think we should consider this discrepancy a Chrome bug (at least because it violates the letter of the spec) and file an issue on them, and adjust our implementation to make our behavior more intentional & to stop violating this assertion. If webcompat demands that we match Chrome, we can change down the line -- but I'm skeptical that fully-clipped content would make for a good choice of scroll anchor.

(In reply to Daniel Holbert [:dholbert] from comment #5)

(In reply to Ryan Hunt [:rhunt] from comment #4)

So if I understand correctly, adding a clip can reduce how we calculate the scrollable overflow rect.

I think so. Clipped-away content is still sized & positioned the same, but
it doesn't create scrollable or visual overflow, because it's not painted.

This seems like it would affect us if the clip reduces the scrollable overflow rect to be zero sized, making something unable to be a scroll anchor that otherwise could be.

There's one other edge case to consider: what if the clip styling reduces
the scrollable overflow rect partially, but enough so that the portion
intersecting the scrollframe is entirely clipped away?

Also: this seems somewhat related to
https://github.com/w3c/csswg-drafts/issues/3483

I modified your test case a bit to try and get this to happen [1]. We're unable to select a scroll anchor in that case and adjust with the yellow animation. Chrome actually selects an anchor here, so it looks like they don't pay attention to 'clip' here. We probably should do the same.

I tend to think our behavior makes sense, and Chrome's behavior is probably
a bug or at least not intentional.

It's odd to me that Chrome [rejects some elements with 0-sized scrollable
overflow
areas](https://github.com/w3c/csswg-drafts/issues/3483#issuecomment-
452124592) but apparently accepts others per your observations here. I think
it makes sense to stick to having scrollable-overflow-boxes be the key
consideration here, and it seems like we should avoid adding exceptions and
special cases unless there's very good reason.

So, I tend to think we should consider this discrepancy a Chrome bug (at
least because it violates the letter of the spec) and file an issue on them,
and adjust our implementation to make our behavior more intentional & to
stop violating this assertion. If webcompat demands that we match Chrome,
we can change down the line -- but I'm skeptical that fully-clipped content
would make for a good choice of scroll anchor.

Yes, I agree with all of that. I'll put up a patch to make this change.

This commit removes the assumption that the overflow rect contains the
border rect.

Instead we now will adjust the overflow rect's start edge to be no less
than the border rect's start edge.

Partial screen clamping
on version 66.0.5 (user version)
Step to reproduce:

1- Create another profile
2- set window to fullscreen
3- log in to https://web.whatsapp.com/
4- open another profile window
5- all profile will be clamping in bottom side
6- window in draggable mode will work properly
7- switch window to fullscreen will return the problem

link of image of problem
https://github.com/cesarjhony/arquivospublicos/blob/master/bug.png

@cesarjhony: thanks for the information, but could you file a new bug report for that problem, here:
https://bugzilla.mozilla.org/enter_bug.cgi?product=Core&component=Layout

This particular bug report (bug 1524064) is specifically about some debug output -- an assertion failing debug builds, which has no user-visible consequences. Your described problem is almost certainly unrelated.

Flags: needinfo?(cesarjhony)
Attachment #9044069 - Attachment is obsolete: true

As it looks like this assertion causes an early return for Element::scrollIntoView() and as such the screenshots as taken with Marionette do not contain the actual expected element.

To reproduce the problem just open https://www.hskupin.info and scroll down.

Blocks: 1570605

How does the assertion make anything return early? I'm confused.

Flags: needinfo?(hskupin)

Sorry, but that was my fault. The problem seems to be something else.

No longer blocks: 1570605
Flags: needinfo?(hskupin)

FWIW, I see this assertion when mousewheel-scrolling on https://www.pointp.fr/infos-agence/maromme-st-jean-point-p-3899#

I get a lot of these....

Blocks: logspam

Redirect a needinfo that is pending on an inactive user to the triage owner.
:hiro, since the bug has recent activity, could you have a look please?

For more information, please visit auto_nag documentation.

Flags: needinfo?(cesarjhony) → needinfo?(hikezoe.birchill)

The needinfo was just a low-priority poke to spin comment 9 off to a separate bug (since it's unrelated, and can also use some further clarification/interpretation about what precisely was going wrong; I don't fully understand comment 9 right now, and perhaps whatever that issue was has been fixed by now in any case.)

If cesarjhony is still following this bug and can still reproduce comment 9 and doesn't mind filing it separately, that would be great. Otherwise the needinfo isn't really actionable/escalation-worthy.

Flags: needinfo?(hikezoe.birchill)

This has spiked a bit in failures in the last 14 days, recent logcat here: https://firefoxci.taskcluster-artifacts.net/KMSldWOBRLOg9iRB42UnzQ/0/public/test_info/logcat-emulator-5554.log
All assertions are happening on layout/painting/crashtests/1547420-1.html.

Update:
There have been 39 failures within the last 7 days:

  • 8 failures on Android 7.0 x86-64 Lite WebRender debug
  • 16 failures on Android 7.0 x86-64 WebRender debug
  • 15 failures on Android 8.0 Pixel2 AArch64 WebRender debug

Recent failure log: https://treeherder.mozilla.org/logviewer?job_id=381027768&repo=mozilla-central&lineNumber=4947

[task 2022-06-12T10:35:23.829Z] 10:35:23     INFO -  REFTEST TEST-START | layout/painting/crashtests/1547420-1.html
[task 2022-06-12T10:35:23.829Z] 10:35:23     INFO -  REFTEST TEST-LOAD | http://10.7.205.238:8854/tests/layout/painting/crashtests/1547420-1.html | 809 / 1062 (76%)
[task 2022-06-12T10:35:23.829Z] 10:35:23     INFO -  REFTEST TEST-PASS | layout/painting/crashtests/1547420-1.html | (LOAD ONLY)
[task 2022-06-12T10:35:23.829Z] 10:35:23     INFO -  REFTEST TEST-END | layout/painting/crashtests/1547420-1.html
[task 2022-06-12T10:35:23.829Z] 10:35:23  WARNING -  REFTEST TEST-UNEXPECTED-FAIL | layout/painting/crashtests/1547420-1.html | assertion count 2 is more than expected 0 to 1 assertions
Whiteboard: [stockwell needswork:owner]

Daniel, could you have a look over what's going on here, there are 132 total failures in the last 30 days all on layout/painting/crashtests/1547420-1.html. Should we just disable the test on Android?

Flags: needinfo?(dholbert)

We should probably add an assertion annotation to allow that test to fail this assertion on Android, yeah.

It looks like the test already has an assertion-annotation and we're just overshooting the count by 1:
assertion count 2 is more than expected 0 to 1 assertions

Patch pushed, to allow 2 assertions for the crashtest in question (1547420-1.html) on Android.

This bug remains open tracking the fact-that-this-assertion-can-be-made-to-fail.

Flags: needinfo?(dholbert)
Keywords: leave-open
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ec39bab8add2
annotate crashtest 1547420-1.html as failing one extra assertion on Android. (no review, just a test-manifest update to reflect reality)
Severity: normal → S3

I see this a lot when I scroll with mouse wheel in TL of twitter.

See Also: → 1891279
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: