1237633 - Assertion failure: effectiveWidth >= 0, at c:/Users/mozilla/debug-build/mozilla-central/dom/base/ResponsiveImageSelector.cpp:399

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Carsten Book [:Tomcat]]

Found on bughunter and reproduced on a windows 7 trunk debug build based on tip

Steps to reproduce:
-> Load https://encrypted.google.com/url?sa=t&rct=j&q=&esrc=s&source=web&cd=6&cad=rja&uact=8&ved=0CDQQjBAwBQ&url=http://www.fieldmuseum.org/at-the-field/exhibitions&ei=OcbvVK-4IY-zyASI0YCIDw&usg=AFQjCNEShl3HvQTgNsoaf62NfwuItOzPGw&sig2=-hvqIjTWaau6asxKuJ

Assertion failure: effectiveWidth >= 0, at c:/Users/mozilla/debug-build/mozilla-central/dom/base/ResponsiveImageSelector.cpp:399
#01: mozilla::dom::ResponsiveImageSelector::SelectImage[xul +0x1ed711a]
#02: mozilla::dom::ResponsiveImageSelector::GetSelectedImageURLSpec[xul +0x1ecb3c1]
#03: mozilla::dom::HTMLImageElement::SelectSourceForTagWithAttrs[xul +0x345959d]
#04: nsDocument::ResolvePreloadImage[xul +0x1ff27dd]
#05: nsHtml5TreeOpExecutor::PreloadImage[xul +0x1812081]
#06: nsHtml5SpeculativeLoad::Perform[xul +0x1811adf]
#07: nsHtml5TreeOpExecutor::RunFlushLoop[xul +0x1813a60]
#08: nsHtml5ExecutorFlusher::Run[xul +0x1813553]
#09: nsThread::ProcessNextEvent[xul +0x5b242f]
#10: NS_ProcessNextEvent[xul +0x6179c2]
#11: mozilla::ipc::MessagePump::Run[xul +0xbd60b1]
#12: MessageLoop::RunInternal[xul +0xb5dd4d]
#13: MessageLoop::RunHandler[xul +0xb5dcc2]
#14: MessageLoop::Run[xul +0xb5d8cd]
#15: nsBaseAppShell::Run[xul +0x40639c0]
#16: nsAppShell::Run[xul +0x41080e7]
#17: nsAppStartup::Run[xul +0x50ebf7f]
#18: XREMain::XRE_mainRun[xul +0x519dbaa]
#19: XREMain::XRE_main[xul +0x519a976]
#20: XRE_main[xul +0x51a049a]
#21: do_main (c:\users\mozilla\debug-build\mozilla-central\browser\app\nsbrowserapp.cpp:212)
#22: NS_internal_main (c:\users\mozilla\debug-build\mozilla-central\browser\app\nsbrowserapp.cpp:352)
#23: wmain (c:\users\mozilla\debug-build\mozilla-central\toolkit\xre\nswindowswmain.cpp:131)
#24: __tmainCRTStartup (f:\dd\vctools\crt\crtw32\startup\crt0.c:255)
#25: BaseThreadInitThunk[kernel32 +0x4ee6c]
#26: RtlInitializeExceptionChain[ntdll +0x63ab3]
#27: RtlInitializeExceptionChain[ntdll +0x63a86]

Comment 1

[Josh Matthews [:jdm]]

Huh:
>(lldb) p effectiveWidth
>(nscoord) $3 = -18000
>(lldb) p i
>(unsigned int) $0 = 3
>(lldb) p numSizes
>(unsigned int) $1 = 5
>(lldb) p mSizeValues[i]
>(nsTArray_Impl<nsCSSValue, nsTArrayInfallibleAllocator>::elem_type) $2 = {
>  mUnit = eCSSUnit_Calc
>  mValue = {
>    mInt = 534754592
>    mFloat = 0.0000000000000000000947438197
>    mString = 0x000000011fdfb520
>    mColor = 534754592
>    mArray = 0x000000011fdfb520
>    mURL = 0x000000011fdfb520
>    mImage = 0x000000011fdfb520
>    mGridTemplateAreas = 0x000000011fdfb520
>    mGradient = 0x000000011fdfb520
>    mTokenStream = 0x000000011fdfb520
>    mPair = 0x000000011fdfb520
>    mRect = 0x000000011fdfb520
>    mTriplet = 0x000000011fdfb520
>    mList = 0x000000011fdfb520
>    mListDependent = 0x000000011fdfb520
>    mSharedList = 0x000000011fdfb520
>    mPairList = 0x000000011fdfb520
>    mPairListDependent = 0x000000011fdfb520
>    mFloatColor = 0x000000011fdfb520
>    mFontFamilyList = 0x000000011fdfb520
>  }
>}

Comment 2

[Josh Matthews [:jdm]]

These nonfatal assertions appear first and look relevant:
>[Child 97399] ###!!! ASSERTION: not a length or calc unit: 'aValue.IsLengthUnit() || aValue.IsCalcUnit()', file /Users/jdm/src/mozilla-central/layout/style/nsRuleNode.cpp, line 403
>[Child 97399] ###!!! ASSERTION: unexpected unit: 'Not Reached', file /Users/jdm/src/mozilla-central/layout/style/nsRuleNode.cpp, line 569

Comment 3

[Josh Matthews [:jdm]]

Minimal testcase:
><img srcset="https://www.fieldmuseum.org/sites/default/files/styles/5x3_2400w/public/gtroyerjoy/2015/03/06/galapagos-web-banner-3.jpg?itok=zP1PCrRE 2400w" sizes="(min-width: 62.5625em) calc(100% - 300px)">

Comment 4

[Josh Matthews [:jdm]]

Specifically, we claim that 100% isn't value in the calc() and is replaced by 0, so the subtraction yields a negative value.

Comment 5

[Josh Matthews [:jdm]]

Given that we specifically address the situation via std::max on the following line, this doesn't need to be a fatal assertion.

Comment 6

[Josh Matthews [:jdm]]

Attachment: Avoid fatal assertion when a responsive image's size specifier is invalid

Comment 7

[John Schoenick [:johns]]

Comment on attachment 8705295 [details] [diff] [review]
Avoid fatal assertion when a responsive image's size specifier is invalid

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

LGTM.

(Is behaving as if the source is size 0 if it ends up negative okay re: spec?)

Comment 8

[Josh Matthews [:jdm]]

Mmm, the spec algorithm for "parse a sizes attribute" has an optional fallback width value (or default 100vw), which should be derived from the img element's `width` attribute if present (see step 3 of https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-source-set). Looks like more work is necessary; thanks for pointing that out.

Comment 9

[John Schoenick [:johns]]

(In reply to Josh Matthews [:jdm] from comment #8)
> Mmm, the spec algorithm for "parse a sizes attribute" has an optional
> fallback width value (or default 100vw), which should be derived from the
> img element's `width` attribute if present (see step 3 of
> https://html.spec.whatwg.org/multipage/embedded-content.html#update-the-
> source-set). Looks like more work is necessary; thanks for pointing that out.

We don't have the fallback width logic (which is new), but we do implement the the default 100vw above:
>      nsCSSValue defaultWidth(100.0f, eCSSUnit_ViewportWidth);
>      effectiveWidth = nsRuleNode::CalcLengthWithInitialFont(pctx,
>                                                             defaultWidth);

I think just passing a default width to ParseSizes that HTMLImageElement gets from that attr, and using it instead of 100vw in that spot, would satisfy the spec (but I didn't carefully walk through it)

Comment 10

[Carsten Book [:Tomcat]]

josh, john, is there any idea/eta when this can be fixed ?

Comment 11

[Josh Matthews [:jdm]]

No. I started looking into it and discovered that implementing comment 9 was actually significantly more complex than I expected. I haven't continued looking into it since.

Comment 12

[Josh Matthews [:jdm]]

Attachment: WIP

This is my WIP patch for posterity's sake.

Comment 13

[Josh Matthews [:jdm]]

Attachment: WIP

I lied, _this_ is the patch.

Comment 14

[Edgar Chen [:edgar]]

(In reply to Josh Matthews [:jdm] from comment #4)
> Specifically, we claim that 100% isn't value in the calc() and is replaced
> by 0, so the subtraction yields a negative value.

Mmm, according to the spec, "Percentages are not allowed in a <source-size-value>, to avoid confusion about what it would be relative to." (See https://html.spec.whatwg.org/multipage/embedded-content.html#source-size-value). For this case, looks like spec expects to get an parsing error on sizes attribute and use default 100vw instead (I've also noticed similar cases when investigating the remaining failures of parse-a-sizes-attribute.html, see item 3 of bug 1017878 comment #8).

Comment 15

[Josh Matthews [:jdm]]

Yeah, I'm curious how this behaves after the changes in bug 1017878.

Comment 16

[Edgar Chen [:edgar]]

(In reply to Josh Matthews [:jdm] from comment #15)
> Yeah, I'm curious how this behaves after the changes in bug 1017878.

After the changes in bug 1017878, we will get an parsing error for `calc(100% - 300px)` and falls back to default value 100vw, so we won't get a negative value and a crash in this case. But I think we still need the patch (attachment 8705295 [details] [diff] [review]) that removes the fatal assertion for the other cases that may get a negative value, e.g. calc(300px - 100vw).

Here my plan:
1. Moves the patch for % from bug 1017878 to here.
2. Updates the test in attachment 8705295 [details] [diff] [review], since we no longer replace 100% by 0.
3. Files a separated bug to support falling back to `width` attribute.

Thank you.

Comment 17

[Edgar Chen [:edgar]]

(In reply to Edgar Chen [:edgar][:echen] from comment #16)
>  
> 3. Files a separated bug to support falling back to `width` attribute.
Filed bug 1263625.

Comment 18

[Edgar Chen [:edgar]]

Attachment: Part 1: Percentages are not allowed in a <source-size-value>, v1

Comment 19

[Edgar Chen [:edgar]]

Attachment: Part 2: Avoid fatal assertion when a responsive image's size specifier is invalid; r=johns

Comment 20

[Edgar Chen [:edgar]]

Attachment: Part 1: Percentages are not allowed in a <source-size-value>, v1

I uploaded a wrong file, here is the correct one.

Comment 21

[Edgar Chen [:edgar]]

Comment on attachment 8740007 [details] [diff] [review]
Part 1: Percentages are not allowed in a <source-size-value>, v1

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

ParseNonNegativeVariant() will return a parsing error if percentages shown in a <source-size-value> and then ComputeFinalWidthForCurrentViewport() will use default value 100vw since mSizeValues is empty.

Comment 22

[Edgar Chen [:edgar]]

Comment on attachment 8740003 [details] [diff] [review]
Part 2: Avoid fatal assertion when a responsive image's size specifier is invalid; r=johns

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

I modify the test to "calc(300px - 100vw)" since percentage is not allowed in sizes attribute after applying part1. Thank you.

Comment 23

[Edgar Chen [:edgar]]

Try result: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b50a8650473ed1a94e009cf192b6f8da114bdbec&group_state=expanded

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ba9323ccb991
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b5494464dd

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ba9323ccb991
https://hg.mozilla.org/mozilla-central/rev/a5b5494464dd