1472520 - Crash in mozilla::Maybe<T>::operator-

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect, P3)

Description

[Mats Palmgren (inactive)]

This bug was filed from the Socorro interface and is
report bp-edc1b161-2205-4b4f-bb58-119a60180628.
=============================================================

Looks like a regression in v62.

MOZ_CRASH Reason: MOZ_RELEASE_ASSERT(mIsSome)


Top 10 frames of crashing thread:

0 xul.dll mozilla::Maybe<mozilla::image::SourceBufferIterator>::operator-> mfbt/Maybe.h:557
1 xul.dll mozilla::image::Decoder::Telemetry image/Decoder.cpp:279
2 xul.dll mozilla::image::IDecodingTask::NotifyDecodeComplete image/IDecodingTask.cpp:110
3 xul.dll mozilla::image::DecodedSurfaceProvider::FinishDecoding image/DecodedSurfaceProvider.cpp:201
4 xul.dll mozilla::image::DecodedSurfaceProvider::Run image/DecodedSurfaceProvider.cpp:145
5 xul.dll mozilla::image::DecodePoolWorker::Run image/DecodePool.cpp:284
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1051
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:519
8 xul.dll mozilla::ipc::MessagePumpForNonMainThreads::Run ipc/glue/MessagePump.cpp:334
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:318

=============================================================

Comment 1

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

Since the iterator is Nothing, this suggests this was an ICO decoder, because that's the only time we replace the iterator we started with. I believe a malformed ICO can hit this code path, although it is surprising to see this happening in 62.0, since I believe the root cause was introduced in bug 1315554.

Comment 2

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

Attachment: 0001-Bug-1472520-Fix-a-crash-when-generating-image-decode.patch, v1

Comment 3

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

(In reply to Andrew Osmond [:aosmond] from comment #2)
> Created attachment 8989400 [details] [diff] [review]
> 0001-Bug-1472520-Fix-a-crash-when-generating-image-decode.patch, v1

Note that DecoderTelemetry::mChunkCount is already checked to be non-zero before adding it to telemetry. So I only needed an extra check for DecoderTelemetry::mBytesDecoded.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5019176ab037
Fix a crash when generating image decoder telemetry. r=tnikkel

Comment 5

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/5019176ab037

Comment 6

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

Comment on attachment 8989400 [details] [diff] [review]
0001-Bug-1472520-Fix-a-crash-when-generating-image-decode.patch, v1

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1315554
[User impact if declined]: May experience rare crash when visiting a page with a malformed ICO.
[Is this code covered by automated tests?]: The success paths and some badly formed ones are, but not the path that triggers the crash.
[Has the fix been verified in Nightly?]: No, we only see a low volume in beta.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: All we changed is to make sure we have a valid object instead of dereferencing it immediately. It is unlikely to be worse than the crash. Worst case the telemetry results end up being incorrect somehow.
[String changes made/needed]: None.

Comment 7

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8989400 [details] [diff] [review]
0001-Bug-1472520-Fix-a-crash-when-generating-image-decode.patch, v1

Crash fix for a new regression in beta. This should land for beta 7.

Comment 8

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/ad56b08003a8