Closed Bug 1256981 Opened 4 years ago Closed 3 years ago

crash in mozilla::image::imgFrame::Optimize

Categories

(Core :: ImageLib, defect, critical)

x86
Windows NT
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla50
Tracking Status
firefox47 --- affected
firefox48 --- affected
firefox-esr45 --- affected
firefox50 --- verified
firefox51 --- unaffected
firefox52 --- unaffected

People

(Reporter: mats, Unassigned)

Details

(Keywords: crash, Whiteboard: gfx-noted)

Crash Data

Attachments

(1 file, 1 obsolete file)

This bug was filed from the Socorro interface and is 
report bp-fd0e43df-7790-4c74-a161-0c3722160315.
=============================================================

This signature has relatively high volume in Firefox 46b1 crash data,
it's currently at #36 in the Top Crashers list.  It appears to be
a long standing crash signature though.

Product	 Version	Count	Percentage
Firefox	44.0.2	         251	37.9%
Firefox	46.0b1	         136	20.5%
Firefox	45.0	         66	10.0%
Firefox	45.0b99	         47	7.1%
Firefox	38.7.0esr	 34	5.1%
[... long tail ...]

Operating System	 Count	Percentage
Windows 7	 350	52.9%
Windows 10	 182	27.5%
Windows 8.1	 92	13.9%
Windows 8	 30	4.5%
Android	         6	0.9%
Windows XP	 2	0.3%

Stack:
mozilla::image::imgFrame::Optimize()
mozilla::image::imgFrame::UnlockImageData()
mozilla::image::RawAccessFrameRef::~RawAccessFrameRef()
mozilla::image::Decoder::~Decoder()
mozilla::image::nsJPEGDecoder::`scalar deleting destructor'(unsigned int)
mozilla::image::Decoder::Release()
mozilla::image::NotifyDecodeCompleteWorker::`scalar deleting destructor'(unsigned int)
nsRunnable::Release()
nsThread::ProcessNextEvent(bool, bool*)
mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*)
MessageLoop::RunHandler()
nsThreadManager::NewThread(unsigned int, unsigned int, nsIThread**)
nsAppShell::Run()
XREMain::XRE_mainRun()
[...]
(In reply to Mats Palmgren (:mats) from comment #0)
> This bug was filed from the Socorro interface and is 
> report bp-fd0e43df-7790-4c74-a161-0c3722160315.
> =============================================================
> 
> This signature has relatively high volume in Firefox 46b1 crash data,
> it's currently at #36 in the Top Crashers list.  It appears to be
> a long standing crash signature though.

Can you dig through the raw dumps for an image URL we can test with? Thanks!
I looked through the URLs but nothing stood out to me.
Maybe Bob can see if there are some URLs we can use for trying to
produce the crash in fuzzing?
Flags: needinfo?(bob)
I haven't seen any with mozilla::image::imgFrame::Optimize(). Overnight, I retested all the mozilla:image::* crashes we've seen and again didn't get the Optimize crash. I do see a number of mozilla:image::* debug fatal assertions which are still reproducible on Trunk though and at least a couple of mozilla::image::SourceBuffer related debug crashes. No opt crashes though.
Flags: needinfo?(bob)
It appears superficially like this is being called during shutdown, from an atexit handler. At that time, both the platform and screen reference draw target are null, so it does a null dereference there.

I guess it is arguable whether it should really be calling imgFrame::Optimize() at all here, or whether the innards of Optimize need to account for this.
Whiteboard: gfx-noted
(In reply to Lee Salzman [:lsalzman] from comment #4)
> It appears superficially like this is being called during shutdown, from an
> atexit handler. At that time, both the platform and screen reference draw
> target are null, so it does a null dereference there.
> 
> I guess it is arguable whether it should really be calling
> imgFrame::Optimize() at all here, or whether the innards of Optimize need to
> account for this.

It appears there already is an early return if shutdown has started here:
https://hg.mozilla.org/releases/mozilla-release/annotate/e0e51efe7b15/image/imgFrame.cpp#l338

It observes "xpcom-shutdown":
http://hg.mozilla.org/mozilla-central/annotate/b6683e141c47/image/ShutdownTracker.cpp#l35

Maybe one of the other "xpcom-shutdown" observers (of which there are many)
is causing this object to be released, before our shutdown observer is called?

Should we try observing "xpcom-will-shutdown" instead?
Flags: needinfo?(seth)
The crash volume is still quite high for this signature, ~3000 in the past 28 days.
(In reply to Mats Palmgren (:mats) from comment #5)
> (In reply to Lee Salzman [:lsalzman] from comment #4)
> > It appears superficially like this is being called during shutdown, from an
> > atexit handler. At that time, both the platform and screen reference draw
> > target are null, so it does a null dereference there.
> > 
> > I guess it is arguable whether it should really be calling
> > imgFrame::Optimize() at all here, or whether the innards of Optimize need to
> > account for this.
> 
> It appears there already is an early return if shutdown has started here:
> https://hg.mozilla.org/releases/mozilla-release/annotate/e0e51efe7b15/image/
> imgFrame.cpp#l338
> 
> It observes "xpcom-shutdown":
> http://hg.mozilla.org/mozilla-central/annotate/b6683e141c47/image/
> ShutdownTracker.cpp#l35
> 
> Maybe one of the other "xpcom-shutdown" observers (of which there are many)
> is causing this object to be released, before our shutdown observer is
> called?

The destructor for RawAccessFrameRef is on the stack for all of these, RawAccessFrameRef holds a reference to the imgFrame, so wouldn't it be impossible for the imgFrame to be released before this?
I didn't mean to imply the imgFrame is released, but rather that it's clear from
the stacks that a RawAccessFrameRef is, which leads to a call to imgFrame::Optimize().
My theory is that gfx has already shutdown at this point, so we have a null-pointer
crash at 'gfxPlatform::GetPlatform()->'.

That's what the early return is trying to protect against:
"// Don't optimize during shutdown because gfxPlatform may not be available."
(In reply to Mats Palmgren (:mats) from comment #5)
> It appears there already is an early return if shutdown has started here:
> https://hg.mozilla.org/releases/mozilla-release/annotate/e0e51efe7b15/image/
> imgFrame.cpp#l338
> 
> It observes "xpcom-shutdown":
> http://hg.mozilla.org/mozilla-central/annotate/b6683e141c47/image/
> ShutdownTracker.cpp#l35
> 
> Maybe one of the other "xpcom-shutdown" observers (of which there are many)
> is causing this object to be released, before our shutdown observer is
> called?
> 
> Should we try observing "xpcom-will-shutdown" instead?

Yep, I'm fine with trying that. I'll write up a patch.
Flags: needinfo?(seth)
Updated the patch to fix a mention of 'xpcom-shutdown' in a header file comment.
Attachment #8765099 - Flags: review?(dholbert)
Attachment #8765098 - Attachment is obsolete: true
Attachment #8765098 - Flags: review?(dholbert)
Comment on attachment 8765099 [details] [diff] [review]
Make ShutdownTracker observe xpcom-will-shutdown instead of xpcom-shutdown.

Review of attachment 8765099 [details] [diff] [review]:
-----------------------------------------------------------------

> Bug 1256981 - Make ShutdownTracker observe xpcom-will-shutdown instead of xpcom-shutdown. r=dholbert

Maybe s/ShutdownTracker/imagelib's ShutdownTracker/ in the commit message, to make it a bit clearer what realm of code this is touching.

r=me, in any case
Attachment #8765099 - Flags: review?(dholbert) → review+
Pushed by mfowler@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/05849fd36220
Make ShutdownTracker observe xpcom-will-shutdown instead of xpcom-shutdown. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/05849fd36220
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Crash volume for signature 'mozilla::image::imgFrame::Optimize':
 - nightly (version 50): 0 crash from 2016-06-06.
 - aurora  (version 49): 0 crash from 2016-06-07.
 - beta    (version 48): 2 crashes from 2016-06-06.
 - release (version 47): 3 crashes from 2016-05-31.
 - esr     (version 45): 432 crashes from 2016-04-07.

Crash volume on the last weeks:
             Week N-1   Week N-2   Week N-3   Week N-4   Week N-5   Week N-6   Week N-7
 - nightly          0          0          0          0          0          0          0
 - aurora           0          0          0          0          0          0          0
 - beta             0          0          0          2          0          0          0
 - release          2          0          0          0          0          1          0
 - esr             52         38         60         50         44         54         29

Affected platform: Windows
I believe we can safely mark this verified fixed on Fx50, based on the crash
data available for the last 3 months.

  SIGNATURE   | mozilla::image::imgFrame::Optimize
  ------------------------------------------------
  CRASH STATS | http://tinyurl.com/z7a7m56
  ------------------------------------------------
  OVERVIEW    | 0 crashes on nightly 52
	      | 0 crashes on nightly 51
	      | 0 crashes on aurora 51
	      | 0 crashes on nightly 50
	      | 0 crashes on aurora 50
	      | 0 crashes on beta 50
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.