1160819 - Assertion failure: false (0 or negative matching width is invalid per spec)

Status: RESOLVED FIXED

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

Description

[Bob Clary [:bc] (inactive)]

Attachment: crash report

1. http://www.marksandspencer.com/s/home-and-furniture/office
2. Assertion failure: false (0 or negative matching width is invalid per spec), at c:\mozilla\builds\nightly\mozilla\dom\base\ResponsiveImageSelector.cpp:707


Beta/38, Aurora/39, Nightly/40 OSX, Linux, Windows 7

Comment 1

[John Schoenick [:johns]]

Attachment: ResponsiveImageSelector - improve some over-aggressive assertions

Some assertions in this file are too aggressive - this particular assertion isn't right, we can have 0 width, and out of range width will be caught by the density assertion below.

Comment 2

[John Schoenick [:johns]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1464345ee249

Comment 3

[Johnny Stenback (:jst)]

Would we not want to protect against negative values here with something other than asserts? Or is that properly dealt with by some code that isn't obvious from the context in this patch?

Comment 4

[Johnny Stenback (:jst)]

Ping on previous question...

Comment 5

[Boris Zbarsky [:bzbarsky]]

jdm, could you take a look at this?  We're hitting this on debug web platform tests...

In the case when we hit this, we have:

(gdb) p mSizeQueries[0].mRawPtr->Matches(Document()->GetShell()->GetPresContext(), 0)
$15 = true
(gdb) p mSizeValues[0]
$12 = (elem_type &) @0x1291384e8: {
  mUnit = eCSSUnit_Pixel, 
  mValue = {
    mInt = 0, 
    mFloat = 0, 

so ResponsiveImageSelector::ComputeFinalWidthForCurrentViewport returns 0 and we hit the assert in question...

Comment 6

[John Schoenick [:johns]]

Attachment: ResponsiveImageSelector - improve some over-aggressive assertions

(In reply to Johnny Stenback  (:jst, [e-mail]) from comment #3)
> Would we not want to protect against negative values here with something
> other than asserts? Or is that properly dealt with by some code that isn't
> obvious from the context in this patch?

Negative numbers should be protected from by ComputeFinalWidthForViewport, but callers could misuse this in the future so changing this assert to | < 0 | rather than <=0 probably makes sense.

Comment 7

[John Schoenick [:johns]]

(In reply to Boris Zbarsky [:bz] from comment #5)
> jdm, could you take a look at this?  We're hitting this on debug web
> platform tests...
> 
> In the case when we hit this, we have:
> 
> (gdb) p
> mSizeQueries[0].mRawPtr->Matches(Document()->GetShell()->GetPresContext(), 0)
> $15 = true
> (gdb) p mSizeValues[0]
> $12 = (elem_type &) @0x1291384e8: {
>   mUnit = eCSSUnit_Pixel, 
>   mValue = {
>     mInt = 0, 
>     mFloat = 0, 
> 
> so ResponsiveImageSelector::ComputeFinalWidthForCurrentViewport returns 0
> and we hit the assert in question...

This patch should fix this issue -- we'll end up with infinite density, which will result in 0 intrinsic height/width for the image. However, our responsive image selection behavior here may need followup - for a 0x0 image, every candidate will have infinite density and be equally preferable. Since we have no idea what size the image will be rendered at if ever, we may want to default to the smallest (or largest?) available source based on some fallback logic.

Comment 9

[Seth Fowler [:seth] [:s2h]]

Johnny, do you think we have enough info for an r+ on this one? Unfortunately it's blocking me in bug 1117607.

Comment 10

[Johnny Stenback (:jst)]

(In reply to John Schoenick [:johns] from comment #7)
> (In reply to Boris Zbarsky [:bz] from comment #5)
> > jdm, could you take a look at this?  We're hitting this on debug web
> > platform tests...
> > 
> > In the case when we hit this, we have:
> > 
> > (gdb) p
> > mSizeQueries[0].mRawPtr->Matches(Document()->GetShell()->GetPresContext(), 0)
> > $15 = true
> > (gdb) p mSizeValues[0]
> > $12 = (elem_type &) @0x1291384e8: {
> >   mUnit = eCSSUnit_Pixel, 
> >   mValue = {
> >     mInt = 0, 
> >     mFloat = 0, 
> > 
> > so ResponsiveImageSelector::ComputeFinalWidthForCurrentViewport returns 0
> > and we hit the assert in question...
> 
> This patch should fix this issue -- we'll end up with infinite density,
> which will result in 0 intrinsic height/width for the image. However, our
> responsive image selection behavior here may need followup - for a 0x0
> image, every candidate will have infinite density and be equally preferable.
> Since we have no idea what size the image will be rendered at if ever, we
> may want to default to the smallest (or largest?) available source based on
> some fallback logic.

Good point, we should file a followup here to default to the smallest available source for a 0x0 image.

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 8629749 [details] [diff] [review]
ResponsiveImageSelector - improve some over-aggressive assertions

r=jst with the assertion changed per earlier comment. Thanks!

Comment 12

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9859cb428ac9

Comment 13

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=try&job_id=9221696
repository: try
start_time: 2015-07-09T02:55:56
who: nnethercote[at]mozilla[dot]com
machine: t-snow-r4-0016
buildname: Rev4 MacOSX Snow Leopard 10.6 try debug test web-platform-tests-3
revision: c620981ea645

PROCESS | 1053 | Assertion failure: false (0 or negative matching width is invalid per spec), at /builds/slave/try-m64-d-00000000000000000000/build/src/dom/base/ResponsiveImageSelector.cpp:707
TEST-UNEXPECTED-TIMEOUT | /html/semantics/embedded-content/the-img-element/current-pixel-density/basic.html | expected CRASH
Return code: 1

Comment 14

[Legacy TBPL/Treeherder Robot]

log: https://treeherder.mozilla.org/logviewer.html#?repo=try&job_id=9220527
repository: try
start_time: 2015-07-09T02:09:24
who: nnethercote[at]mozilla[dot]com
machine: t-snow-r4-0145
buildname: Rev4 MacOSX Snow Leopard 10.6 try debug test web-platform-tests-3
revision: c620981ea645

PROCESS | 1105 | Assertion failure: false (0 or negative matching width is invalid per spec), at /builds/slave/try-m64-d-00000000000000000000/build/src/dom/base/ResponsiveImageSelector.cpp:707
TEST-UNEXPECTED-TIMEOUT | /html/semantics/embedded-content/the-img-element/current-pixel-density/basic.html | expected CRASH
Return code: 1

Comment 15

[Seth Fowler [:seth] [:s2h]]

The web platform test annotations need an update. I'll push a new try job.

Comment 16

[Seth Fowler [:seth] [:s2h]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=239b288c1123

Comment 17

[Seth Fowler [:seth] [:s2h]]

Attachment: Update web platform test annotations

OK, with these updated test annotations, this should be ready to land.

Comment 18

[Seth Fowler [:seth] [:s2h]]

Whoops, bzexport changed the assignee. It's entirely too smart sometimes.

Comment 19

[John Schoenick [:johns]]

Comment on attachment 8632153 [details] [diff] [review]
Update web platform test annotations

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

LGTM!

Comment 20

[Seth Fowler [:seth] [:s2h]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ccdae73ea161
https://hg.mozilla.org/integration/mozilla-inbound/rev/9203d3ff7592

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ccdae73ea161
https://hg.mozilla.org/mozilla-central/rev/9203d3ff7592