Closed Bug 1472520 Opened 3 years ago Closed 3 years ago

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


(Core :: ImageLib, defect, P3)

62 Branch
Windows 10



Tracking Status
firefox-esr52 --- unaffected
firefox-esr60 --- wontfix
firefox61 --- wontfix
firefox62 --- fixed
firefox63 --- fixed


(Reporter: mats, Assigned: aosmond)



(Keywords: assertion, crash, regression, Whiteboard: [gfx-noted])

Crash Data


(1 file)

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

Looks like a regression in v62.


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/

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.
Assignee: nobody → aosmond
Priority: -- → P3
Whiteboard: [gfx-noted]
(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.
Attachment #8989400 - Flags: review?(tnikkel) → review+
Pushed by
Fix a crash when generating image decoder telemetry. r=tnikkel
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
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.
Attachment #8989400 - Flags: approval-mozilla-beta?
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.
Attachment #8989400 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.