This bug is for crash report bp-2eebee1b-c43e-49c5-ad26-f18ba0200619.

Top 10 frames of crashing thread:

0 xul.dll mozilla::image::`anonymous namespace'::ImageDecoderListener::GetFrame image/imgTools.cpp:144
1 xul.dll mozilla::image::imgTools::EncodeImage image/imgTools.cpp:474
2 xul.dll WindowsSMTCProvider::LoadImageAtIndex::<unnamed-tag>::operator const widget/windows/WindowsSMTCProvider.cpp:464
3 xul.dll mozilla::MozPromise<nsCOMPtr<imgIContainer>, bool, 1>::ThenValue<`lambda at /builds/worker/checkouts/gecko/widget/windows/WindowsSMTCProvider.cpp:452:11', `lambda at /builds/worker/checkouts/gecko/widget/windows/WindowsSMTCProvider.cpp:475:11'>::DoResolveOrRejectInternal xpcom/threads/MozPromise.h:769
4 xul.dll mozilla::MozPromise<CopyableTArray<bool>, nsresult, 0>::ThenValueBase::ResolveOrRejectRunnable::Run xpcom/threads/MozPromise.h:410
5 xul.dll mozilla::AutoTaskDispatcher::TaskGroupRunnable::Run xpcom/threads/TaskDispatcher.h:228
6 xul.dll nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1234
7 xul.dll NS_ProcessNextEvent xpcom/threads/nsThreadUtils.cpp:501
8 xul.dll mozilla::ipc::MessagePump::Run ipc/glue/MessagePump.cpp:87
9 xul.dll MessageLoop::RunHandler ipc/chromium/src/base/message_loop.cc:308

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

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

It looks like it's a UAF. Since the crash occurs at here

NS_IMETHOD_(already_AddRefed<mozilla::gfx::SourceSurface>)
GetFrame(uint32_t aWhichFrame, uint32_t aFlags) override {
  return mImage->GetFrame(aWhichFrame, aFlags);
}

(The macro is expanded by NS_FORWARD_IMGICONTAINER(mImage->)), when encoding the image at here.

I guess the reason is mImage is a nullptr. When OnImageReady is called, the given imgIContainer may have a null mImage so mImage-> would cause a UAF.

agi, is there a way to detect whether mImage is set or not when OnImageReady is fired? Can I use SIZE_AVAILABLE as an indicator to see if mImage is set?

I think aosmond is better equipped to answer this question :)

FYI the code linked above doesn't run in the case of the stacktrace in this bug, from what I can see. It's only used when calling DecodeImageFromChannelAsync, although it might be relevant for more code paths.

EDIT: ah nevermind it's actually hitting this. That's interesting.

From what I can see mImage should always be non-null if the download was completed successfully, and it looks like if you're in the HandleFetchSuccess code path that should always be true.

Maybe alwu knows what's going on here.

The assertions we add in HandleFetchSuccess() are debug-only, so they might not be triggled on an offical Nightly build.

It seems that we might get a null image when (1) we encouter an error during fetching or (2) there is no data from the URL from which we requested an image. In the second situation, I guess the aStatus might still be a success (and OnDataAvailable() wasn't called) and we should not set the pointer of the image container ar that time if we don't have mImage yet.

It's possible only OnStartRequest() and OnStopRequest() was called, but no OnDataAvailable(). If so, when OnStopRequest() is being called, we would have no image but the channel status is a success.

Therefore, before setting the image container, we should also ensure we get the image already.

The image container might be null, so we should handle this case properly as a fail case.

https://hg.mozilla.org/mozilla-central/rev/21576931d335
https://hg.mozilla.org/mozilla-central/rev/c979b29463be

The patch landed in nightly and beta is affected.
:alwu, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.

Comment on attachment 9158703 [details]
Bug 1647070 - part1 : set container only when we have an image and no error occurs.

Beta/Release Uplift Approval Request

• User impact if declined: users might encouter a crash when using media control on certain sites.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce: No
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): In these changes, we add some additional check to avoid from dereferencing a null pointer, which is no harm at all.
• String changes made/needed: No

Comment on attachment 9158703 [details]
Bug 1647070 - part1 : set container only when we have an image and no error occurs.

Approved for 79.0b5.

https://hg.mozilla.org/releases/mozilla-beta/rev/be2182224fb1
https://hg.mozilla.org/releases/mozilla-beta/rev/e756e35190bf