assertion: inline-size less than zero: 'aContainingBlockISize >= 0' with fixed position overflow-hidden element

RESOLVED FIXED in Firefox 58

Status

()

Core
XUL
P3
normal
RESOLVED FIXED
11 months ago
7 months ago

People

(Reporter: jaws, Assigned: dholbert)

Tracking

unspecified
mozilla58
Points:
---

Firefox Tracking Flags

(firefox57 wontfix, firefox58 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Bug 1355924 adds a new animation that is implemented by a wide SVG image 630px wide by 20px high.

The markup looks like this:
<toolbarbutton>
  <image id=a/>
  <hbox id=b>
    <image id=c/>
  </hbox>
</toolbarbutton>

#a is an image that has an intrinsic size of 16x16
#b has position:fixed and overflow:hidden, and a size of 18w x 20h
#c has a background-image set, and has a size of 630w x 20h

When the size of #b is set using `width: 18px; height: 20px;` we get an assertion for: "inline-size less than zero: 'aContainingBlockISize >= 0'".

When the size of #b is set using `min-width: 18px; max-width: 18px; min-height: 20px; max-height: 20px;` the assertion goes away.

Both ways of setting width achieve the same visual output.

Updated

10 months ago
Priority: -- → P3
Duplicate of this bug: 1382946
Created attachment 8912447 [details] [diff] [review]
patch needed to trigger bug (not a fix, just part of STR)

STR (using mochitest mentioned in bug 1382946):
 1. Apply this patch.
 2. ./mach mochitest ./layout/style/test/chrome/test_display_mode_reflow.html

ACTUAL RESULTS:
Pretty soon, this goes by in your output:
4 INFO TEST-PASS | layout/style/test/chrome/test_display_mode_reflow.html | offset top returns to original value 
GECKO(3132) | [3132, Main Thread] ###!!! ASSERTION: inline-size less than zero: 'aContainingBlockISize >= 0', file /scratch/work/builds/mozilla-inbound/mozilla/layout/generic/nsFrame.cpp, line 5830
The negative size comes from this line of CalculateHypotheticalPosition:
 aCBSize = aFrame->GetLogicalSize(wm) - borderPadding.Size(wm);
https://dxr.mozilla.org/mozilla-central/rev/33b7b8e81b4befcba503c0e48cd5370aeb715085/layout/generic/ReflowInput.cpp#1133

At this point, we have the following variable values:
 * aFrame->mRect is 0,0,0,0. So aFrame->GetLogicalSize returns a 0-sized rect.
 * borderPadding is top=0, right=120, bottom=0, left=120.
So, aCBSize gets a width (i.e. iSize) of 0 - 120 - 120 = -240.

That aCBSize gets returned (by reference) up to the caller, GetHypotheticalBoxContainer (where it's called "blockContentSize"), and its negative ISize gets passed down into ComputeISizeValue, which calls mFrame->ComputeISizeValue, which has this precondition.

Updated

7 months ago
status-firefox57: --- → wontfix
Hmm -- I tried to circle back to this today, but the tests noted in bug 1382946 [test_display_mode*] are now failing with an unrelated error on my local linux desktop & laptop (including if I update to a cset from ~1 month ago, the day when I posted comment 2). And they don't trigger the assertion (because they don't get to it).

The errors look like this:
15 INFO TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_display_mode.html | TypeError: style is null - Should not throw any errors
queryApplies@chrome://mochitests/content/chrome/layout/style/test/chrome/test_display_mode.html:38:5

17 INFO TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_display_mode_reflow.html | TypeError: secondDiv is null - Should not throw any errors
@chrome://mochitests/content/chrome/layout/style/test/chrome/test_display_mode_reflow.html:38:7

Not sure what changed about my environment to make that problem start happening.

However, it's OK, because I have simpler STR now -- simply activate fullscreen mode via the fullscreen API, e.g. by clicking "Launch Fullscreen" here:
http://davidwalsh.name/demo/fullscreen.php

That reliably triggers the assertion for me, in a build with attachment 8912447 [details] [diff] [review] applied.
OK -- so the question from comment 3 here is, why does our containing block have a 0-sized frame rect (i.e. border-box size), despite having nonzero border + padding?

The answer is: it's "XUL collapsed", which triggers this code:

> 173     // See if we are collapsed. If we are, then simply iterate over all our
> 174     // children and give them a rect of 0 width and height.
> 175	  if (aBox->IsXULCollapsed()) {
> 176	    nsIFrame* child = nsBox::GetChildXULBox(aBox);
> 177	    while(child)
> 178	    {
> 179	      nsBoxFrame::LayoutChildAt(aState, child, nsRect(0,0,0,0));
https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/layout/xul/nsSprocketLayout.cpp#179

The LayoutChildAt() call stomps on the frame's mRect with the nsRect(0,0,0,0) arg there.

We should probably be setting the frame's "LogicalUsedBorderAndPadding" to 0,0,0,0 there as well, to reflect reality.  That's the source of the (currently nonzero) "borderPadding" that we're working with in the code discussed in comment 3.
Specifically, I suspect we should probably add an additional special-case to nsFrame::GetUsedBorder/GetUsedPadding to make it return 0 if we're XUL and XUL-Collapsed.  We already do this for some other special cases, e.g. SVG Text frames, and this seems to be the correct behavior for XUL Collapse, based on this MDN documentation about "visibility:collapse" (which is what's used to determine XUL-collapsedness for XUL frames):

 # For XUL elements, the computed size of the element is always zero,
 # regardless of other styles that would normally affect the size,
 # although margins still take effect.
https://developer.mozilla.org/en-US/docs/Web/CSS/visibility

That's the best documentation I've been able to find on what "XUL Collapsed" means, BTW -- the only other documentation I could find on this feature is a brief hand-wavy statement here ("the element is collapsed and does not appear"):
https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/collapsed
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
(In reply to Daniel Holbert [:dholbert] from comment #6)
> Specifically, I suspect we should probably add an additional special-case to
> nsFrame::GetUsedBorder/GetUsedPadding to make it return 0 if we're XUL and
> XUL-Collapsed.

Er, that's not correct, actually.

In the code I quoted in comment 5, the *parent* is XUL collapsed, and its *children* are the things with a 0-sized rect (whose border/padding we want to ignore, and whose IsXULCollapsed() methods won't return true even though this frame is 0-sized).

Let's just modify the GetHypotheticalBoxContainer code to detect this case (parent being IsXULCollapsed) and avoid subtracting out the borderpadding in that case.
Comment hidden (mozreview-request)

Comment 9

7 months ago
Fwiw, I think we should just WONTFIX minor XUL layout issues like this bug.
To quote roc from three years ago:
"XUL is a dead-end technology and investment in XUL provides minimal returns ---
this includes effort spent reviewing and maintaining XUL."
https://groups.google.com/forum/#!msg/mozilla.dev.platform/HDJl1iBifB8/1TG876POw8cJ
Component: Layout → XUL

Comment 10

7 months ago
mozreview-review
Comment on attachment 8920738 [details]
Bug 1379332: When computing abspos CB content-box size, don't bother subtracting borderpadding if CB is a 0-sized child of a XUL-collapsed frame.

https://reviewboard.mozilla.org/r/191752/#review197374

LGTM, but please don't ask me to review XUL changes in the future,
unless they have the explicit goal of helping us transition from XUL
to HTML/CSS in our UI; or security issues.
Other stuff will get automatic r-.
Attachment #8920738 - Flags: review?(mats) → review+
(In reply to Mats Palmgren (:mats) from comment #10)
> LGTM, but please don't ask me to review XUL changes in the future,
> unless they have the explicit goal of helping us transition from XUL
> to HTML/CSS in our UI; or security issues.

Yeah -- I wouldn't have even bothered to fix this, except that it's caused a super-gross pattern to start accumulating in our frontend CSS (to work around this bug) -- for example:
https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/browser/themes/shared/toolbarbutton-icons.inc.css#108-115

I don't want that pattern to keep spreading, hence trying to get this closed out.  (And I'm cleaining up that CSS hackery in bug 1410561)

Comment 12

7 months ago
Pushed by dholbert@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e91ca49ccf6c
When computing abspos CB content-box size, don't bother subtracting borderpadding if CB is a 0-sized child of a XUL-collapsed frame. r=mats
Apparently the pref(...) line in crashtest.list is making Android upset, because we don't have a default value for the stylo pref there (we just let it be disabled by virtue of being unspecified), so it's "unrecognized" and triggers a failure:
> REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/xul/crashtests/1379332-1.xul | boolean preference 'layout.css.servo.enabled' not known or wrong type
> REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/xul/crashtests/1379332-2.xul | boolean preference 'layout.css.servo.enabled' not known or wrong type

This general issue is already filed as bug 1350948.

It looks like I can work around this by adding "skip-if(Android)" to the front of these lines.  At least, we do that for a few other tests that set prefs, e.g. here:
https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/layout/reftests/svg/reftest.list#34
Created attachment 8921258 [details] [diff] [review]
followup to skip crashtests on android

RyanVM, could you land this followup on Autoland? It should fix the crashtest orange mentioned above.
Flags: needinfo?(ryanvm)

Comment 15

7 months ago
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/9e434f5e600d
followup: Skip this bug's crashtests on Android to avoid triggering "boolean preference...not known or wrong type" error there. (test-only, no review)
(Sorry, I used the wrong bug number in that followup commit message -- I pasted in "bug 1350948" instead of this bug number. Oh well, not a big deal - not a substantial patch, and it is tangentially related to bug 1350948, and I added a comment over there to point back to this bug.)
Flags: needinfo?(ryanvm)
(Ah, and RyanVM backed out and re-landed the followup, with the corrected number, which is how comment 15 ended up on this correct bug despite my typo. Thanks, Ryan!)
https://hg.mozilla.org/mozilla-central/rev/e91ca49ccf6c
https://hg.mozilla.org/mozilla-central/rev/9e434f5e600d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
You need to log in before you can comment on or make changes to this bug.