Closed Bug 1160819 Opened 4 years ago Closed 4 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla42
Tracking Status
firefox40 --- affected
firefox42 --- fixed

People

(Reporter: bc, Assigned: johns)

References

(Blocks 1 open bug)

Details

(Keywords: assertion)

Attachments

(3 files, 1 obsolete file)

Attached file 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
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.
Attachment #8606776 - Flags: review?(jst)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1464345ee249
Assignee: nobody → john
Status: NEW → ASSIGNED
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?
Ping on previous question...
Flags: needinfo?(john)
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...
Flags: needinfo?(josh)
(In reply to Johnny Stenback  (:jst, jst@mozilla.com) 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.
Attachment #8606776 - Attachment is obsolete: true
Attachment #8606776 - Flags: review?(jst)
Attachment #8629749 - Flags: review?(jst)
Flags: needinfo?(john)
(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.
Duplicate of this bug: 1181598
Johnny, do you think we have enough info for an r+ on this one? Unfortunately it's blocking me in bug 1117607.
Flags: needinfo?(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.
Flags: needinfo?(jst)
Comment on attachment 8629749 [details] [diff] [review]
ResponsiveImageSelector - improve some over-aggressive assertions

r=jst with the assertion changed per earlier comment. Thanks!
Attachment #8629749 - Flags: review?(jst) → review+
The web platform test annotations need an update. I'll push a new try job.
OK, with these updated test annotations, this should be ready to land.
Attachment #8632153 - Flags: review?(john)
Assignee: john → seth
Whoops, bzexport changed the assignee. It's entirely too smart sometimes.
Assignee: seth → john
Comment on attachment 8632153 [details] [diff] [review]
Update web platform test annotations

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

LGTM!
Attachment #8632153 - Flags: review?(john) → review+
https://hg.mozilla.org/mozilla-central/rev/ccdae73ea161
https://hg.mozilla.org/mozilla-central/rev/9203d3ff7592
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla42
Flags: needinfo?(josh)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.