1133119 - crash in memmove | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntRectTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is 
report bp-d84f9347-eeb0-46b9-9fb2-789012150212.
=============================================================

Looks like this signature has a spike in recent versions:

Product	       Version	Percentage	Number Of Crashes
Firefox 	38.0a1 	98.09 %	        154
Firefox 	37.0a1 	1.27 %	        2
Thunderbird 	38.0a1 	0.64 %	        1 

All crashes are EXCEPTION_ACCESS_VIOLATION_READ on address 0x0,
so it looks like 'source' is null here (which means 'aData' was null):
http://hg.mozilla.org/mozilla-central/annotate/3094601af679/gfx/2d/DrawTargetCairo.cpp#l234

So it looks like it's the same problem as in bug 1035168?

(BTW, this method could use MOZ_ASSERT(aData) and aRect.x/y >= 0 at the top.
And MOZ_ASSERT(surfData) after line 224, if we believe it should never
return NULL there.)

All crashes with this signature are on 64-bit Windows, fwiw.

Comment 1

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: More null checks, and have Map fail if the data is null. r=mattwoodrow

Comment 2

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8571483 [details] [diff] [review]
More null checks, and have Map fail if the data is null. r=mattwoodrow

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

::: gfx/2d/2D.h
@@ +408,5 @@
>     * Can return 0 if there was OOM allocating surface data.
>     */
>    virtual int32_t Stride() = 0;
>  
> +  /** 

Trailing whitespace.

@@ +416,5 @@
>    {
>      aMappedSurface->mData = GetData();
>      aMappedSurface->mStride = Stride();
>      mIsMapped = true;
> +    return !!aMappedSurface->mData;

We shouldn't set mIsMapped=true if this is going to return false.

::: gfx/2d/SourceSurfaceD2D.cpp
@@ +280,5 @@
>    aMappedSurface->mData = (uint8_t*)map.pData;
>    aMappedSurface->mStride = map.RowPitch;
>    mIsMapped = true;
>  
> +  return !!aMappedSurface->mData;

Same with mIsMapped here, and the other two.

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Attachment: More null checks, and have Map fail if the data is null. r=mattwoodrow

Comment 4

[Milan Sreckovic [:milan] (needinfo for best results)]

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

Comment 5

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/54eee85f489f

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/54eee85f489f

Comment 7

[Sylvestre Ledru [:Sylvestre]]

Milan, as you suggested in bug 1153558, an uplift would be nice. Thanks.

Comment 8

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8572014 [details] [diff] [review]
More null checks, and have Map fail if the data is null. r=mattwoodrow

Approval Request Comment
[Feature/regressing bug #]:
[User impact if declined]: 85% of memcpy crashes come from this signature (see bug 1153558, comment 9)
[Describe test coverage new/current, TreeHerder]:
[Risks and why]: 
[String/UUID change made/needed]:

Comment 9

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8572014 [details] [diff] [review]
More null checks, and have Map fail if the data is null. r=mattwoodrow

[Triage Comment]
we merged m-b in m-r.

Taking it as it landed a month ago and, hopefully, will fix many crashes.

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-release/rev/90d2538212ab

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/90d2538212ab