1631187 - Crash in [@ mozilla::gfx::SourceSurfaceCapture::IsValid]

Status: RESOLVED FIXED

(Core :: Layout: Images, Video, and HTML Frames, defect, P2)

Description

[Calixte Denizet (:calixte)]

This bug is for crash report bp-3e6c3946-5892-4bd5-a913-54ade0200417.

Top 10 frames of crashing thread:

0 xul.dll mozilla::gfx::SourceSurfaceCapture::IsValid const gfx/2d/SourceSurfaceCapture.cpp
1 xul.dll mozilla::image::DrawableFrameRef::DrawableFrameRef image/imgFrame.h:349
2 xul.dll mozilla::image::DecodedSurfaceProvider::DrawableRef image/DecodedSurfaceProvider.cpp:68
3 xul.dll mozilla::image::ISurfaceProvider::Surface image/ISurfaceProvider.h:268
4 xul.dll mozilla::image::SurfaceCacheImpl::LookupBestMatch image/SurfaceCache.cpp:1008
5 xul.dll static mozilla::image::SurfaceCache::LookupBestMatch image/SurfaceCache.cpp:1593
6 xul.dll mozilla::image::RasterImage::LookupFrameInternal image/RasterImage.cpp:317
7 xul.dll mozilla::image::RasterImage::LookupFrame image/RasterImage.cpp:343
8 xul.dll mozilla::image::RasterImage::RequestDecodeForSizeInternal image/RasterImage.cpp:1155
9 xul.dll mozilla::image::RasterImage::RequestDecodeForSize image/RasterImage.cpp:1121

There are 4 crashes (from 3 installations) in nightly 77 starting with buildid 20200417100143. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1616411.

[1] https://hg.mozilla.org/mozilla-central/rev?node=747bdbeee667

Comment 1

[Cameron McCormack (:heycam)]

We're crashing in SourceSurfaceCapture::IsValid as we try to call mSurfToOptimize->IsValid() but the vtable pointer we pull out from mSurfToOptimize is garbage (0xe5e5e5e5c8800000, in this case). The mSurfToOptimize pointer itself looks like it could have been legitimate.

Comment 2

[Cameron McCormack (:heycam)]

Any idea how I might dig into this Timothy?

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Given this is a crash on Windows, without WebRender, mSurfToOptimize is most likely a SourceSurfaceVolatileData:

https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/image/imgFrame.cpp#112

Neither SourceSurfaceVolatileData nor SourceSurfaceAlignedRawData override SourceSurface::IsValid. They use the implementation from the base class SourceSurface which just returns true:

https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/gfx/2d/2D.h#432

We put the SourceSurfaceVolatileData stored in mLockedSurface into a SourceSurfaceCapture here:

https://searchfox.org/mozilla-central/rev/a4d62e09a4c46aef918667fa759bf9ae898dc258/image/imgFrame.cpp#495

There are locks surrounding the use of both mLockedSurface and mSurfToOptimize. Maybe that is relevant (a place we forgot to hold the lock?).

I skimmed the reviews for the potential regressing bug, but nothing stood out for me....

Comment 4

[Rachel Tublitz [:rachel]]

Hey Cameron, should this one still be a P1?

Comment 5

[Julien Cristau [:jcristau] (back April 22)]

the address in comment 1 suggests uaf?

Comment 6

[Sean Voisen (:svoisen)]

Setting to P2 due to low volume. We will keep an eye on it and if crash volume goes up we can reassess.

Comment 7

[Emma Humphries ☕️🎸🧞‍♀️✨ (she/they) [:emceeaich] (Pacific Time) use needinfo]

These are current, open bugs with a Severity of critical. The Severity of these bugs is being changed to S2 to be consistent with the May 4 2020 Severity definitions.

Please let Release Management know if these bugs are still S2.

Comment 8

[Cameron McCormack (:heycam)]

I'm leaving this at S2, but I haven't had any luck investigating it yet. I tried a few URLs from crash reports but didn't reproduce the crash. Of those I looked at, none had JPEGs with orientations.

Comment 9

[[:philipp]]

[Tracking Requested - why for this release]:
this signature is jumping up in volume in early data from the 78.0b rollout.

Comment 10

[Julien Cristau [:jcristau] (back April 22)]

Cameron, any chance you could take another look?

Comment 11

[Timothy Nikkel (:tnikkel)]

Maybe bug 1644208 fixes this?

Comment 12

[Ryan VanderMeulen [:RyanVM]]

(In reply to Timothy Nikkel (:tnikkel) from comment #11)

Maybe bug 1644208 fixes this?

Sure looks like it! No crashes in 78.0b6+ where that fix is present.