Closed
Bug 1035168
Opened 10 years ago
Closed 10 years ago
Nightly crash in _VEC_memcpy | mozilla::gfx::CopyToImageSurface with address 0x0
Categories
(Core :: Graphics, defect)
Tracking
()
VERIFIED
FIXED
mozilla33
Tracking | Status | |
---|---|---|
firefox31 | --- | unaffected |
firefox32 | + | verified |
firefox33 | --- | verified |
People
(Reporter: kairo, Assigned: mattwoodrow)
Details
(Keywords: crash)
Crash Data
Attachments
(2 files)
3.24 KB,
patch
|
bas.schouten
:
review+
lmandel
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
1.76 KB,
patch
|
bas.schouten
:
review+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-a031b3b5-2db3-41ee-8053-61adb2140707. ============================================================= 0 msvcr100.dll _VEC_memcpy 1 xul.dll mozilla::gfx::CopyToImageSurface(unsigned char *,mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const &,int,mozilla::gfx::SurfaceFormat) gfx/2d/DrawTargetCairo.cpp 2 xul.dll mozilla::gfx::GetCairoSurfaceForSourceSurface(mozilla::gfx::SourceSurface *,bool) gfx/2d/DrawTargetCairo.cpp 3 xul.dll mozilla::gfx::DrawTargetCairo::DrawSurface(mozilla::gfx::SourceSurface *,mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const &,mozilla::gfx::RectTyped<mozilla::gfx::UnknownUnits> const &,mozilla::gfx::DrawSurfaceOptions const &,mozilla::gfx::DrawOptions const &) gfx/2d/DrawTargetCairo.cpp 4 xul.dll imgFrame::LockImageData() image/src/imgFrame.cpp 5 xul.dll mozilla::image::RasterImage::DrawWithPreDownscaleIfNeeded(imgFrame *,gfxContext *,GraphicsFilter,gfxMatrix const &,gfxRect const &,nsIntRect const &,unsigned int) image/src/RasterImage.cpp [...] I saw a case with a somewhat different stack but the same few top frames, see bp-0f9cd839-d27f-4f86-a204-39c0a2140707 The address of all those crashes is 0x0, so it smells like a null pointer deref to me. The vast majority of those crashes is on Win7, but some are 8.1 or Vista. Also, the vast majority of those crashes are on Intel graphics, a very few on AMD. URLs are all over the place, with Facebook as the usual suspect on top. More details and crashes can be found at https://crash-stats.mozilla.com/report/list?signature=_VEC_memcpy%20%7C%20mozilla%3A%3Agfx%3A%3ACopyToImageSurface%28unsigned%20char%2A%2C%20mozilla%3A%3Agfx%3A%3AIntSizeTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E%20const%26%2C%20int%2C%20mozilla%3A%3Agfx%3A%3ASurfaceFormat%29 This started happening with the 20140702030201 build on Nightly, so the regression range seems to be http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=83c09fe3a658&tochange=7075808c3306
Reporter | ||
Comment 1•10 years ago
|
||
https://crash-stats.mozilla.com/report/list?signature=memcpy%20%7C%20mozilla%3A%3Agfx%3A%3ACopyToImageSurface%28unsigned%20char%2A%2C%20mozilla%3A%3Agfx%3A%3AIntSizeTyped%3Cmozilla%3A%3Agfx%3A%3AUnknownUnits%3E%20const%26%2C%20int%2C%20mozilla%3A%3Agfx%3A%3ASurfaceFormat%29 seems to be the same issue.
Crash Signature: [@ _VEC_memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)] → [@ _VEC_memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)]
[@ memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mo…
It may have been: db4f6e215872 Matt Woodrow — Bug 997304 - Copy the image data if it's not a suitable size for cairo. r=Bas Let me know if I'm reading this correctly: http://hg.mozilla.org/mozilla-central/rev/db4f6e215872#l1.58 If there is an invalid stride and data->GetData() is null, then we do CopyToImageSurface and the memcpy crashes with a null src. Does that seem plausible?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 3•10 years ago
|
||
Yeah, sounds plausible. It looks like cairo verifies the stride (and returns the CAIRO_STATUS_INVALID_STRIDE error surface) before checking if the data parameter is non-null. A stride of 0 (which is what I'd expect if GetData returns nullptr) would have this effect.
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 4•10 years ago
|
||
Assignee: nobody → matt.woodrow
Attachment #8452106 -
Flags: review?(bas)
Updated•10 years ago
|
Attachment #8452106 -
Flags: review?(bas) → review+
Assignee | ||
Comment 5•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd41dff8f475
Assignee | ||
Comment 6•10 years ago
|
||
This got backed out because of win7 assertions. SourceSurfaceD2D asserts if we try mix usage of GetData() and Map(), which happens when FilterNodeSoftware checks the stride before calling DrawSurface.
Attachment #8455094 -
Flags: review?(bas)
Comment 7•10 years ago
|
||
Comment on attachment 8455094 [details] [diff] [review] Avoid calling GetData/Stride on a surface that we will later Map. Review of attachment 8455094 [details] [diff] [review]: ----------------------------------------------------------------- ::: gfx/2d/FilterNodeSoftware.cpp @@ +820,5 @@ > + DataSourceSurface::MappedSurface map; > + if (result->Map(DataSourceSurface::READ, &map)) { > + // Unmap immediately since CloneAligned hasn't been updated > + // to use the Map API yet. We can still read the stride/data > + // values as long as we don't try to dereference them. nit: Add a todo here, this is pretty bad, as there's technically no guarantee a subsequent map will have the same stride.
Attachment #8455094 -
Flags: review?(bas) → review+
Assignee | ||
Comment 8•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/524dc207d68f https://hg.mozilla.org/integration/mozilla-inbound/rev/45e32200a0c4
Comment 9•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/524dc207d68f https://hg.mozilla.org/mozilla-central/rev/45e32200a0c4
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
Reporter | ||
Comment 10•10 years ago
|
||
Hrm, and just as we fixed this, bug 997304 was uplifted to 32 without uplifting this one as well. :(
status-firefox31:
--- → unaffected
status-firefox32:
--- → affected
status-firefox33:
--- → fixed
tracking-firefox32:
--- → ?
Comment 11•10 years ago
|
||
Matt - Given that this impacts 32, can we nom for beta uplift?
Flags: needinfo?(matt.woodrow)
Assignee | ||
Comment 12•10 years ago
|
||
Comment on attachment 8452106 [details] [diff] [review] Check that the data is available Approval Request Comment [Feature/regressing bug #]: Bug Bug 997304 [User impact if declined]: Occasional crashes, unsure of the frequency. [Describe test coverage new/current, TBPL]: Confirmed that crash reports stopped being filed [Risks and why]: Very low risk, basically just a null check and a small change to avoid a deprecated API (since it caused assertions). [String/UUID change made/needed]: None
Attachment #8452106 -
Flags: approval-mozilla-beta?
Flags: needinfo?(matt.woodrow)
Comment 13•10 years ago
|
||
Comment on attachment 8452106 [details] [diff] [review] Check that the data is available beta+
Attachment #8452106 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment 14•10 years ago
|
||
https://hg.mozilla.org/releases/mozilla-beta/rev/dd517186b945 https://hg.mozilla.org/releases/mozilla-beta/rev/2bc7c49698cc
Comment 15•10 years ago
|
||
Looking through Socorro, I can see the following: 1. [@ _VEC_memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)] - there are NO more crashes in builds after July 22nd (last build with this crash was 32.0, BuildID=20140722195627) 2. [@ memcpy | mozilla::gfx::CopyToImageSurface(unsigned char*, mozilla::gfx::IntSizeTyped<mozilla::gfx::UnknownUnits> const&, int, mozilla::gfx::SurfaceFormat)] - there are 2 crashes in builds older than July 22nd - Firefox 32 - 1 crash in Beta 5 build with ID=20140807212602 - Firefox 34 - 1 crash in Nightly build with ID=20140804030205 Given the very low occurrence in Builds after July 22nd, I'm closing this issue.
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in
before you can comment on or make changes to this bug.
Description
•