Closed Bug 1237633 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox46 --- affected
firefox48 --- fixed

People

(Reporter: cbook, Assigned: edgar)

References

(Blocks 1 open bug, )

Details

(Keywords: assertion, Whiteboard: [tw-dom])

Attachments

(2 files, 4 obsolete files)

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]
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
>  }
>}
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
Specifically, we claim that 100% isn't value in the calc() and is replaced by 0, so the subtraction yields a negative value.
Given that we specifically address the situation via std::max on the following line, this doesn't need to be a fatal assertion.
Assignee: nobody → josh
Status: NEW → ASSIGNED
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?)
Attachment #8705295 - Flags: review?(john) → review+
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.
(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)
josh, john, is there any idea/eta when this can be fixed ?
Flags: needinfo?(josh)
Flags: needinfo?(john)
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.
Flags: needinfo?(josh)
Attached patch WIP (obsolete) — Splinter Review
This is my WIP patch for posterity's sake.
Attached patch WIP (obsolete) — Splinter Review
I lied, _this_ is the patch.
Attachment #8727844 - Attachment is obsolete: true
Flags: needinfo?(john)
Whiteboard: [tw-dom]
(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).
Yeah, I'm curious how this behaves after the changes in bug 1017878.
(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.
(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.
Attachment #8727845 - Attachment is obsolete: true
I uploaded a wrong file, here is the correct one.
Attachment #8739990 - Attachment is obsolete: true
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.
Attachment #8740007 - Flags: review?(josh)
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.
Attachment #8740003 - Flags: review?(josh)
Attachment #8740007 - Flags: review?(josh) → review+
Attachment #8740003 - Flags: review?(josh) → review+
Assignee: josh → echen
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ba9323ccb991
https://hg.mozilla.org/mozilla-central/rev/a5b5494464dd
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.