Bugzilla

Reporter

Updated

•

4 years ago

Keywords: bugmon

Whiteboard: [bugmon:confirm] → [bugmon:confirmed]

Reporter

Comment 1

•

4 years ago

Bugmon Analysis:
Unable to reproduce bug using the following builds:
> mozilla-central 20200908215255-dc90a7a18c07
> mozilla-central 20200908030802-80ac8d8c74d5
Removing bugmon keyword as no further action possible.
Please review the bug and re-add the keyword for further analysis.

Comment 2

•

3 years ago

The fuzzers have been tripping over this for a while and it is triggered frequently. Marking as fuzzblocker.

status-firefox82: affected → wontfix

status-firefox93: --- → wontfix

status-firefox94: --- → affected

status-firefox95: --- → affected

status-firefox-esr91: --- → affected

Whiteboard: [bugmon:confirmed] → [bugmon:confirmed][fuzzblocker]

Comment 3

•

3 years ago

A Pernosco session is available here: https://pernos.co/debug/D3IdoAb7_iSEnR8PbQeP7w/index.html

Reporter

Comment 4

•

2 years ago

Attached file testcase.html — Details

Attachment #9174438 - Attachment is obsolete: true

Reporter

Updated

•

2 years ago

Keywords: bugmon

Reporter

Comment 5

•

2 years ago

:dholbert, due to the simplicity of the attached testcase, this issue is still occurring very frequently. Is this actually an issue or could we relax the assertion?

Flags: needinfo?(dholbert)

Assignee

Comment 6

•

2 years ago

•

Edited

This is the sort of assertion that I would expect might trip from bogus values like e.g. <html style="height:nscoord_MAX"> or similar. If that was what was causing that, then I'd suggest relaxing the assertion.

However, neither of the attached testcases seem to have bogus huge values, so this is likely indicative of a real bug (probably something to do with our printPreview timing/logic, given the printPreview() invocation in both of the testcases).

So: this is probably pointing at something real that we need to fix. I can try to take a closer look in the next day or so; in the meantime, if it'd be possible for you or Tyson to generate a pernosco trace with the new testcase, that would help a lot.

(I tried to repro locally but I'm getting JS error self.printPreview is not a function so I think there's one or several pref-flips that I'm missing.)

Assignee

Updated

•

2 years ago

Severity: normal → S2

Comment 7

•

2 years ago

ni? myself to remember to get a Pernosco trace.

Flags: needinfo?(twsmith)

Comment 8

•

2 years ago

A Pernosco session is available here: https://pernos.co/debug/37Pltfy0paWx9ogK_hESAA/index.html

The Pernosco session in comment 3 is from the initial test case.

Flags: needinfo?(twsmith)

Assignee

Comment 9

•

2 years ago

Thanks.

From a quick look so far, some notes:
(1) We enter PresShell::ResizeReflowIgnoreOverride
(2) That function sets the visible area to have unconstrained height:
https://searchfox.org/mozilla-central/rev/d107bc8aeadcc816ba85cb21c1a6a1aac1d4ef9f/layout/base/PresShell.cpp#2086-2089
(3) We do the reflow
(4) We fail the assert because the reflow failed to update the visible area's unconstrained height (it's still unconstrained):
https://searchfox.org/mozilla-central/rev/d107bc8aeadcc816ba85cb21c1a6a1aac1d4ef9f/layout/base/PresShell.cpp#2129-2131

(Need to dig in a bit more, just dropping what I've got so far)

https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/layout/base/PresShell.cpp#9670-9671

Assignee

Comment 10

•

2 years ago

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

(4) We fail the assert because the reflow failed to update the visible area's unconstrained height (it's still unconstrained):
https://searchfox.org/mozilla-central/rev/d107bc8aeadcc816ba85cb21c1a6a1aac1d4ef9f/layout/base/PresShell.cpp#2129-2131

Actually, we do attempt to update the visible area's unconstrained height, here:


  if (isRoot && size.BSize(wm) == NS_UNCONSTRAINEDSIZE) {
    mPresContext->SetVisibleArea(boundsRelativeToTarget);
  }

...but boundsRelativeToTarget has NS_UNCONSTRAINEDSIZE (i.e. nscoord_MAX) as its height, this doesn't end up helping.

boundsRelativeToTarget comes from the frame tree, and it's an indication that we have frames with nscoord_MAX as their height. Indeed, at this point, our frame tree looks like this (note 1073741823 is nscoord_MAX):

Viewport(-1)@75454a06baa0 (x=0, y=0, w=0, h=1073741823)
[...]
    Canvas(html)(-1) (x=0, y=0, w=0, h=1073741823)
      PageSequence(html)(-1) (x=0, y=0, w=34560, h=1073741823)
        PrintedSheet(-1) (x=1440, y=720, w=48960, h=63360)
          Page(-1) (x=0, y=0, w=48960, h=63360)
            [...]

The top few levels of frames have bogus sizes here -- width=0 and height=nscoord_MAX. The frames inside (PrintedSheet and lower) have finite/meaningful sizes, but we don't actually traverse them during this reflow; we bail out early since this is an incremental reflow and nsPageSequenceFrame doesn't attempt to do incremental reflows right now. We take this early-return:
https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/layout/generic/nsPageSequenceFrame.cpp#281-285
...which sets our ReflowOutput (the desired-size that ends up in our frame rect) based on the mComputedSize that was passed in via the ReflowInput.

This early-return apparently assumes that we'll have a definite computed height in our ReflowInput -- but in this particular case we clearly do not. In this case, our aReflowInput (in nsPageSequenceFrame::Reflow) has mComputedSize=(0,nscoord_MAX) (and all of the ancestor frames have that in their own aReflowInput as well). So that's what we end up putting in the ReflowOutput here.

Assignee

Comment 11

•

2 years ago

•

Edited

Usually, our the ViewportFrame isn't given an unconstrained height (BSize) in its ReflowInput -- it's given a height based on the viewport size which is a physical thing on-screen with a known size.

But it can happen, per https://searchfox.org/mozilla-central/rev/5ce782034b736293836a2850ab698e149ca04657/layout/base/PresShell.cpp#9593 , if we're called from sizeToContent (which we are in this case, and which can be seen in the testcase).

So: essentially, I think this is a case where nsPageSequenceFrame's early-return needs to detect and handle this unconstrained-BSize scenario (by e.g. returning the sum of its children's frame-sizes, plus whatever additional padding/etc. space that it requires). If nsPageSequenceFrame handles this gracefully, then I think its parent frames will respect the desired size that it requests, and they'll use that for their own calculations as well. And we'll end up getting reasonable sizes in our frame tree and thus in boundsRelativeToTarget and thus in SetVisibleArea, and we'll avoid tripping this assert.

Flags: needinfo?(dholbert)

Assignee

Comment 12

•

2 years ago

I think the simplest solution here is just to amend nsPageSequenceFrame::PopulateReflowOutput to not grow-to-fill-the-available-space (which it currently does) if ComputedBSize() is unconstrained.