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

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: bc, Assigned: johns)

Tracking

(Blocks: 1 bug, {assertion})

Trunk
mozilla42
assertion
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox40 affected, firefox42 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
Created attachment 8600650 [details]
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
(Assignee)

Comment 1

3 years ago
Created attachment 8606776 [details] [diff] [review]
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.
Attachment #8606776 - Flags: review?(jst)
(Assignee)

Comment 2

3 years ago
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)
(Assignee)

Comment 6

3 years ago
Created attachment 8629749 [details] [diff] [review]
ResponsiveImageSelector - improve some over-aggressive assertions

(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)
(Assignee)

Updated

3 years ago
Flags: needinfo?(john)
(Assignee)

Comment 7

3 years ago
(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.

Updated

3 years ago
Duplicate of this bug: 1181598
Blocks: 1117607
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+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
The web platform test annotations need an update. I'll push a new try job.
Created attachment 8632153 [details] [diff] [review]
Update web platform test annotations

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
(Assignee)

Comment 19

3 years ago
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
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla42

Updated

3 years ago
Flags: needinfo?(josh)
You need to log in before you can comment on or make changes to this bug.