crash in imgFrame::~imgFrame()

VERIFIED FIXED in Firefox 31

Status

()

defect
--
critical
VERIFIED FIXED
5 years ago
5 years ago

People

(Reporter: bzumwalt, Assigned: boris)

Tracking

({crash, regression, reproducible})

Trunk
mozilla33
ARM
Gonk (Firefox OS)
Points:
---

Firefox Tracking Flags

(blocking-b2g:1.4+, firefox31 fixed, firefox32 fixed, firefox33 fixed, b2g-v1.4 verified, b2g-v2.0 verified, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [sprd314460][partner-blocker][b2g-crash], crash signature)

Attachments

(3 attachments, 9 obsolete attachments)

7.03 KB, text/plain
Details
40.69 KB, text/plain
Details
2.33 KB, patch
boris
: review+
Details | Diff | Splinter Review
Posted file Logcat
This bug was filed from the Socorro interface and is 
report bp-f332c023-f587-43f4-b98b-a09492140430.
=============================================================
[@ imgFrame::~imgFrame() ]

Description:
Crash occurs when user is swiping through pictures in Gallery app. Crash experienced multiple times on three different Open C devices with two different memory cards. Pictures present in gallery are sourced from pictures taken from device as well as pictures from external sources.

Rough Repro Steps:
1) Updated Open C to BuildID: 20140430040206
2) Launch gallery with a variety of pictures (see description) and select a picture
3) Slide finger left or right to change to next picture in gallery
4) Repeat step 3 for ~1 minute

Environmental Variables:
Device: Open C 2.0 Master M-C Mozilla RIL
BuildID: 20140430040206
Gaia: badc73ee7f108fa631150bded0cc57e92aad810e
Gecko: e19812f56952
Version: 32.0a1
Firmware Version: P821A10-ENG_20140410
'qawanted' to test on Buri trunk and Open_C 1.4

important steps:
> 3) Slide finger left or right to change to next picture in gallery
> 4) Repeat step 3 for ~1 minute (sometimes takes longer)
Keywords: qawanted
I'd also like to know if this is consistently reproducible with the STR given above.
blocking-b2g: --- → backlog
Component: Gaia::Gallery → Graphics
Product: Firefox OS → Core
Version: unspecified → Trunk
Unable to repro crash on Buri 2.0 or Open C 1.4

Crash still repros on Open C 2.0 using multiple different devices

Repro rate is 100%, but user may need to scroll through pictures for over one minute for crash to occur. Leaving QA Wanted for repro rate as, although this issue is reproducible 100% by me using multiple memory cards and other peoples devices, it has proven difficult for others to reliably reproduce.

Buri Environmental Variables:
Device: Buri v2.0 Master M-C Mozilla RIL
BuildID: 20140501040203
Gaia: 8873166797fecfa65c0afa644d2ebcc7d7cb4ed3
Gecko: b227a707080f
Version: 32.0a1
Firmware Version: v1.2-device.cfg

Open C 1.4 Environmental Variables:
Device: Open C v1.4 Mozilla RIL
BuildID: 20140501000200
Gaia: 011330c45f575d5a87332e605be47f570faf10ca
Gecko: a9db863ae8c2
Version: 30.0
Firmware Version: P821A10-ENG_20140410
3 of 4 testers were able to reproduce this on the same Open C device using the same SD card and following the provided STR. 

During step 3, the testers would sometimes change swipe direction.

The quickest repro occurred roughly within 45 seconds, one was around 1:30, and the longest took about 3:30. One tester was not able to reproduce this issue.

Repro Rate: 75%
Keywords: qawanted
blocking-b2g: backlog → 2.0?
Assigning self as QA Contact, finding regression window
QA Contact: bzumwalt
Crash occurs on earliest available nightly build for 2.0 Master M-C for Open C:

Environmental Variables:
Device: Open_C v2.0 Master M-C Mozilla RIL
BuildID: 20140418040202
Gaia: 375d87bfef8a5d7135f139da8c17f237b990d3f5
Gecko: 7fe3ee0cf8be
Version: 31.0a1
Firmware Version: P821A10-ENG_20140429

Will continue to further narrow regression-window
blocking-b2g: 2.0? → 2.0+
We can't do a regression window here because the earliest master build we have available reproduces the bug, as per comment 6 stated. Removing regression window wanted tag.
Hi Jet, could you find someone to take a look at this please?
Flags: needinfo?(bugs)
Crash in the imgFrame destructor indicates a double-free. Need logs from the crashing device please.
Flags: needinfo?(nhirata.bugzilla)
Flags: needinfo?(mwu)
Flags: needinfo?(bugs)
(In reply to Jet Villegas (:jet) from comment #9)
> Crash in the imgFrame destructor indicates a double-free. Need logs from the
> crashing device please.

What logs? There's a logcat here & a crash report URL.
Jason: I was looking for a longer backtrace than what was in the logcat. The short log there indicates 2 decoder failures, and it would be good to know if that was occurring earlier during execution. 

Michael: can you comment on recent changes to imgFrame handling when a decode fails like that?
Ok - adding qawanted to get a longer logcat when this crash reproduces?
Keywords: qawanted
QA Contact: bzumwalt → pcheng
Attaching a logcat starting from prior to opening Gallery app to after Gallery app exits due to crash.

Tested on today's master Flame build on an Open C with FFOS_US_EBAY_P821A10V1.0.0B06_LOG_DL base image.
I'm not sure another logcat is going to be useful at all. I'd like to get the contents of the sdcard that's causing the crash so we can reproduce the crash under gdb.
Flags: needinfo?(mwu)
we need the contents of the card, please. It might be stuff on the device itself... 

should be able to:
adb pull /storage/sdcard sdcard/
adb pull /storage/sdcard0 sdcard0/
adb pull /storage/sdcard1 sdcard1/

and then:
zip sdcard.zip sdcard*

I'm not sure if it will fit as an attachment; you may need to share it out as a docs.google.com file.
Flags: needinfo?(nhirata.bugzilla) → needinfo?(jzimbrick)
Flags: needinfo?(jzimbrick)
Keywords: qawanted
First - Open C and Flame are close enough that Open C should be using Flame nightly builds, right?

The images are the standard ones, I don't expect that there is anything "wrong" with them. On the larger screen, it looks like the thumbnails in exif are not large enough (do they exist at all?), so we have to decode the full image - we're not in OOM situation by any chance?  I know there is no indication in the log on it, but maybe worth a check.
I couldn't reproduce this issue on my n4 and openc with master m-c.
Can anyone share the gallery content which could use to reproduce this issue?
I can also reproduce this bug.

This crash happened in RasterImage::Discard()
{
...
  // now, mScaleResult.frame.get() == (0x0)
  mFrameBlender.Discard();            

  // now, mScaleResult.frame.get() == (0x5a5a5a5a)  (When the gallary crashed, it was always 0x5a5a5a5a.)
  mScaleResult.status = SCALE_INVALID;
  mScaleResult.frame = nullptr;  // the crash happened (try to free 0x5a5a5a5a.)
...
}

Here is my log:
05-27 22:40:40.359  4434  4434 I Gecko   : RasterImage::Discard -> mFrameBlender.Discard() (mScaleResult.frame is 0x0)
05-27 22:40:40.359  4434  4434 I Gecko   : ~imgFrame() is called (this == 0xb08e5e00)
05-27 22:40:40.369  4434  4434 I Gecko   : RasterImage::Discard -> mScaleResult.frame = nullptr (mScaleResult.frame == 0x5a5a5a5a5a)
05-27 22:40:40.369  4434  4434 I Gecko   : ~imgFrame() is called (this == 0x5a5a5a5a)
// crash here

Looks like mFrameBlender.Discard() has some memory issues, so mScaleResult.frame is assigned to (0x5a5a5a5a). (Actually, it should be (0x0) in most cases.)

PS:
I flashed flame code into openC to try to reproduce this bug.
Target:    PVT.master.flame (eng)
BuildID:   20140430040206
Version    32.0a1
gecko:     d6d67da827d0180b95db2592b725321f6102d141
Looks like that the memory allocation has problem

FrameBlender::ClearFrames()
{
  mFrames = new FrameSequence();
  // the new allocation may return 0x5a5a5a5a, which means poison memory (Bug 860254).
}

(In reply to Boris Chiou [:boris] from comment #20)
> I can also reproduce this bug.
> 
> This crash happened in RasterImage::Discard()
> {
> ...
>   // now, mScaleResult.frame.get() == (0x0)
>   mFrameBlender.Discard();            
> 
>   // now, mScaleResult.frame.get() == (0x5a5a5a5a)  (When the gallary
> crashed, it was always 0x5a5a5a5a.)
>   mScaleResult.status = SCALE_INVALID;
>   mScaleResult.frame = nullptr;  // the crash happened (try to free
> 0x5a5a5a5a.)
> ...
> }
> 
> Here is my log:
> 05-27 22:40:40.359  4434  4434 I Gecko   : RasterImage::Discard ->
> mFrameBlender.Discard() (mScaleResult.frame is 0x0)
> 05-27 22:40:40.359  4434  4434 I Gecko   : ~imgFrame() is called (this ==
> 0xb08e5e00)
> 05-27 22:40:40.369  4434  4434 I Gecko   : RasterImage::Discard ->
> mScaleResult.frame = nullptr (mScaleResult.frame == 0x5a5a5a5a5a)
> 05-27 22:40:40.369  4434  4434 I Gecko   : ~imgFrame() is called (this ==
> 0x5a5a5a5a)
> // crash here
> 
> Looks like mFrameBlender.Discard() has some memory issues, so
> mScaleResult.frame is assigned to (0x5a5a5a5a). (Actually, it should be
> (0x0) in most cases.)
> 
> PS:
> I flashed flame code into openC to try to reproduce this bug.
> Target:    PVT.master.flame (eng)
> BuildID:   20140430040206
> Version    32.0a1
> gecko:     d6d67da827d0180b95db2592b725321f6102d141
(In reply to Boris Chiou [:boris] from comment #20)
> I can also reproduce this bug.
> 
> This crash happened in RasterImage::Discard()
> {
> ...
>   // now, mScaleResult.frame.get() == (0x0)
>   mFrameBlender.Discard();            
> 
>   // now, mScaleResult.frame.get() == (0x5a5a5a5a)  (When the gallary
> crashed, it was always 0x5a5a5a5a.)
Why "mFrameBlender".Discard() change value of "mScaleResult".frame?
Flags: needinfo?(boris.chiou)
(In reply to Boris Chiou [:boris] from comment #21)
> Looks like that the memory allocation has problem
I think we need to check the memory usage inside the DiscardTracker.
Then we can have more clues to find why jemalloc allocation/free[1] has problem.
[1]http://dxr.mozilla.org/mozilla-central/source/memory/mozjemalloc/jemalloc.c#4450
> 
> FrameBlender::ClearFrames()
> {
>   mFrames = new FrameSequence();
>   // the new allocation may return 0x5a5a5a5a, which means poison memory
> (Bug 860254).
> }
Whiteboard: [b2g-crash] → [b2g-crash][sprd314460][partner-blocker]
Duplicate of this bug: 1015863
(In reply to Boris Chiou [:boris] from comment #20)
> I can also reproduce this bug.
> 
> This crash happened in RasterImage::Discard()
> {
> ...
>   // now, mScaleResult.frame.get() == (0x0)
>   mFrameBlender.Discard();            
> 
>   // now, mScaleResult.frame.get() == (0x5a5a5a5a)  (When the gallary
> crashed, it was always 0x5a5a5a5a.)
>   mScaleResult.status = SCALE_INVALID;
>   mScaleResult.frame = nullptr;  // the crash happened (try to free
> 0x5a5a5a5a.)
> ...
> }
> 
> Here is my log:
> 05-27 22:40:40.359  4434  4434 I Gecko   : RasterImage::Discard ->
> mFrameBlender.Discard() (mScaleResult.frame is 0x0)
> 05-27 22:40:40.359  4434  4434 I Gecko   : ~imgFrame() is called (this ==
> 0xb08e5e00)
> 05-27 22:40:40.369  4434  4434 I Gecko   : RasterImage::Discard ->
> mScaleResult.frame = nullptr (mScaleResult.frame == 0x5a5a5a5a5a)
> 05-27 22:40:40.369  4434  4434 I Gecko   : ~imgFrame() is called (this ==
> 0x5a5a5a5a)
> // crash here
> 
> Looks like mFrameBlender.Discard() has some memory issues, so
> mScaleResult.frame is assigned to (0x5a5a5a5a). (Actually, it should be
> (0x0) in most cases.)
> 

As you described, it more looks like a double free.
I'm not familiar with this code, but here is my guess:

mFrameBlender.Discard() -> FrameBlender::ClearFrames() clears the mFrame. (It's a FrameSequence that contains many frames)
And then 
mScaleResult.frame = nullptr; Crash because it points to a frame inside the mFrame. (see FrameSequence::GetFrame)

Before you jump into jemelloc, I would prefer check the double free issue first.
You can dump the |mFrame| and |mScaleResult.frame| 's memory addresses or gdb at ~imgFrame().
Hi, CJ and Benjamin,

Thanks for your comments. I am trying to figure out what happens to this RasterImage. Now, I only know the RasterImage might be accessed by the main thread and the decoder thread simultaneously. When the crash happened, the main thread tried to call RasterImage::Discard() which cleared up all the data members, and the decoder thread was trying to call the destructor of this RasterImage at the same time. Therefore, after the destructor was called, this RasterImage and its data members became a freed object (0x5a5a5a5a), and then the main thread tried to free the freed object, which causes a segmentation fault.
Assignee: nobody → boris.chiou
Flags: needinfo?(boris.chiou)
Status: NEW → ASSIGNED
imgFrame is not reference counted. AnsAutoPtr manages the imgFrame's lifetime, but it is not thread safe and RasterImage seems not cared about the thread-safety.
I'm not convinced this is the same bug as bug 1015863 because the STR only happen on 2.0, not 1.4.
Whiteboard: [b2g-crash][sprd314460][partner-blocker] → [b2g-crash]
Duplicate of this bug: 1015863
blocking-b2g: 2.0+ → 1.4+
Whiteboard: [b2g-crash] → [sprd314460][partner-blocker][b2g-crash]
Comment on attachment 8436670 [details] [diff] [review]
Use WeakPtr to protect RasterImage

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

::: image/src/DiscardTracker.cpp
@@ +55,5 @@
>    MutexAutoLock lock(*sNodeListMutex);
>    MOZ_ASSERT(sInitialized);
> +
> +  nsRefPtr<RasterImage> img = node->img.get();
> +  MOZ_ASSERT(img);

CJ and Boris,
We should not get the raw pointer("img.get").

Here is a example:
nsRefPtr<RasterImage> img = node->img.get();

step 1:
node->img.get(); //return address 0x12345

step 2:
delete note from other thread. After deleted, the address 0x12345 is invalid.

step 3:
nsRefPtr<RasterImage> img = address 0x12345 //error

Do we have a member function to generate a refptr from weakptr?
Flags: needinfo?(slee)
Flags: needinfo?(slee) → needinfo?
Flags: needinfo?
Attachment #8436840 - Flags: feedback?(slee)
Per discussion:
In DecodeJob::Run(), forget DecodeJob::mImage after DecodeDoneWorker hold reference of RasterImage. 
DecodeDoneWorker is a task executed in main thread, it's safe to release RasterImage in ~DecodeDoneWorker
(In reply to Jerry Shih[:jerry] from comment #33)
> CJ and Boris,
> We should not get the raw pointer("img.get").
Jerry, you are right. Thank you for detailed review
Comment on attachment 8438124 [details] [diff] [review]
Force to Dispatch RasterImage to the main thread

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

::: image/src/RasterImage.h
@@ +486,5 @@
>        {}
>  
>        NS_IMETHOD Run();
>  
> +      virtual ~DecodeJob();

move to protected section.

@@ +509,5 @@
> +    /**
> +     * Called by DecodeJob::~DecodeJob(). We should dispatch the nsRefPtr<RasterImage>
> +     * to the main thread to keep the thread safety of RasterImage.
> +     */
> +    static void DispatchImageToMain(already_AddRefed<RasterImage> aImg);

aImage
Attachment #8438124 - Flags: feedback?(cku) → feedback+
Comment on attachment 8438124 [details] [diff] [review]
Force to Dispatch RasterImage to the main thread

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

::: image/src/RasterImage.h
@@ +487,5 @@
>  
>        NS_IMETHOD Run();
>  
> +      virtual ~DecodeJob();
> +

virtual ~DecodeJob() MOZ_OVERRIDE;

@@ +512,5 @@
> +     */
> +    static void DispatchImageToMain(already_AddRefed<RasterImage> aImg);
> +
> +    NS_IMETHOD Run();
> +

MOZ_OVERRIDE
Attachment #8438144 - Flags: review?(joe)
Comment on attachment 8438144 [details] [diff] [review]
Force to Dispatch RasterImage to the main thread

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

::: image/src/RasterImage.h
@@ +503,5 @@
>      mozilla::Mutex            mThreadPoolMutex;
>      nsCOMPtr<nsIThreadPool>   mThreadPool;
>    };
>  
> +  class ImageHolder : public nsRunnable

Should move the class to RasterImage.cpp?
Comment on attachment 8438144 [details] [diff] [review]
Force to Dispatch RasterImage to the main thread

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

::: image/src/RasterImage.h
@@ +484,5 @@
>          : mRequest(aRequest)
>          , mImage(aImg)
>        {}
>  
> +      NS_IMETHOD Run() MOZ_OVERRIDE;

You should not use override on non-virtual function.

@@ +512,5 @@
> +     * to the main thread to keep the thread safety of RasterImage.
> +     */
> +    static void DispatchImageToMain(already_AddRefed<RasterImage> aImage);
> +
> +    NS_IMETHOD Run() MOZ_OVERRIDE;

As above.
Attachment #8438144 - Flags: feedback?(slee) → feedback+
Comment on attachment 8438144 [details] [diff] [review]
Force to Dispatch RasterImage to the main thread

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

::: image/src/RasterImage.h
@@ +484,5 @@
>          : mRequest(aRequest)
>          , mImage(aImg)
>        {}
>  
> +      NS_IMETHOD Run() MOZ_OVERRIDE;

OK. Thanks.

@@ +503,5 @@
>      mozilla::Mutex            mThreadPoolMutex;
>      nsCOMPtr<nsIThreadPool>   mThreadPool;
>    };
>  
> +  class ImageHolder : public nsRunnable

OK, I should move it to RasterImage.cpp
Attachment #8438144 - Flags: review?(joe)
Comment on attachment 8438144 [details] [diff] [review]
Force to Dispatch RasterImage to the main thread

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

::: image/src/RasterImage.cpp
@@ +3408,5 @@
>  }
>  
> +RasterImage::DecodePool::DecodeJob::~DecodeJob()
> +{
> +  // Dispatch mImage to main thread to prevent mImage from being destructed by decode thread.

It is possible that multithread decoding is disabled and you don't need to dispatch to mainthread if current thread is already main thread.
http://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#3378


By the way, do we need to call dispatch for every decoding status?
http://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.h#381
(In reply to peter chang[:pchang][:peter] from comment #46)
> >  
> > +RasterImage::DecodePool::DecodeJob::~DecodeJob()
> > +{
> > +  // Dispatch mImage to main thread to prevent mImage from being destructed by decode thread.
> 
> It is possible that multithread decoding is disabled and you don't need to
> dispatch to mainthread if current thread is already main thread.
> http://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#3378
I agree. I should add a "if (NS_IsMainThread())" to check it.

> 
> 
> By the way, do we need to call dispatch for every decoding status?
> http://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.h#381
Any kind of DecodeJob holds a nsRefPtr<RasterImage>, and it may call the destructor of RasterImage, so I think we should call dispatch for every decoding status.
Attachment #8438251 - Flags: review?(joe)
(In reply to peter chang[:pchang][:peter] from comment #46)
> Comment on attachment 8438144 [details] [diff] [review]
> Force to Dispatch RasterImage to the main thread
> 
> Review of attachment 8438144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/RasterImage.cpp
> @@ +3408,5 @@
> >  }
> >  
> > +RasterImage::DecodePool::DecodeJob::~DecodeJob()
> > +{
> > +  // Dispatch mImage to main thread to prevent mImage from being destructed by decode thread.
> 
> It is possible that multithread decoding is disabled and you don't need to
> dispatch to mainthread if current thread is already main thread.
> http://dxr.mozilla.org/mozilla-central/source/image/src/RasterImage.cpp#3378
Nice catch!
I think we can just call NS_ProxyRelease(mainthread, mImage.forget()).
Flags: needinfo?(slee)
Attachment #8438251 - Flags: review?(joe) → review?(seth)
Flags: needinfo?(slee)
Sorry, I made some mistakes. The ambiguous base is from ImageResource or nsIProperties, and we can resolve it by static_cast or NS_ISUPPORT_CAST. I will upload a new patch to use NS_ProxyRelease(), instead of implementing a new runnable. Thanks.

(In reply to Boris Chiou [:boris] from comment #52)
> (In reply to Benjamin Chen [:bechen] from comment #51)
> > I think we can just call NS_ProxyRelease(mainthread, mImage.forget()).
> 
> We can not use NS_ProxyRelease() because RasterImage also supports
> mfbt/WeakPtr and will get a compilation error: "'nsISupports' is an
> ambiguous base of 'mozilla::image::RasterImage"
> 
>  ../../dist/include/nsProxyRelease.h:47:51: error: 'nsISupports' is an
> ambiguous base of 'mozilla::image::RasterImage'
> 
> Therefore, I implemented our own runnable object to dispatch RasterImage to
> the main thread.
Attachment #8439054 - Flags: review?(seth)
Comment on attachment 8439054 [details] [diff] [review]
Ensure that the delete of RasterImage occurs on the main thread

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

::: image/src/RasterImage.cpp
@@ +3418,5 @@
> +      RasterImage* rawImg = nullptr;
> +      mImage.swap(rawImg);
> +      nsresult rv = NS_ProxyRelease(mainThread, NS_ISUPPORTS_CAST(ImageResource*, rawImg));
> +      if (NS_FAILED(rv)) {
> +        NS_WARNING("Failed to proxy release to main thread!");

have an assertion here as well
Attachment #8439726 - Flags: review?(seth)
(In reply to StevenLee[:slee] from comment #44)
> Comment on attachment 8438144 [details] [diff] [review]
> Force to Dispatch RasterImage to the main thread
> 
> Review of attachment 8438144 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: image/src/RasterImage.h
> @@ +484,5 @@
> >          : mRequest(aRequest)
> >          , mImage(aImg)
> >        {}
> >  
> > +      NS_IMETHOD Run() MOZ_OVERRIDE;
> 
> You should not use override on non-virtual function.
> 
> @@ +512,5 @@
> > +     * to the main thread to keep the thread safety of RasterImage.
> > +     */
> > +    static void DispatchImageToMain(already_AddRefed<RasterImage> aImage);
> > +
> > +    NS_IMETHOD Run() MOZ_OVERRIDE;
> 
> As above.

NS_IMETHOD expands to include "virtual". MOZ_OVERRIDE is pretty much always appropriate where you see NS_IMETHOD.
Comment on attachment 8439726 [details] [diff] [review]
Ensure that the delete of RasterImage occurs on the main thread

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

Looks good.
Attachment #8439726 - Flags: review?(seth) → review+
Fix try server build failed:
1. Use DebugOnly<nsresult> to fix "error: unused variable 'rv'" on Opt builds.
2. Remove MOZ_OVERRIDE to fix "error C3665:
'mozilla:image:RasterImage::DecodePool:DecodeJob::~DecodeJob': override
specifier ''override not allowed on a destructor" on Windows builds.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=44cf23b06642
Attachment #8439726 - Attachment is obsolete: true
Attachment #8441146 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/3060e6862f15

This will get Aurora uplift by way of its 1.4+ blocking status, but I'm wondering if we should land this on beta for Fx31 as well? Is there any reason this would be B2G-only or could this affect Fx as well?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/3060e6862f15
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
https://hg.mozilla.org/releases/mozilla-aurora/rev/5a5f30915141
https://hg.mozilla.org/releases/mozilla-b2g30_v1_4/rev/3722afcbbc51

ni? Brogan about whether this should land on mozilla-beta for Fx31 as well. Seth, feel free to chime in too :)
Passing needinfo to jsmith as I don't believe I have the authority to advise here. :D
Flags: needinfo?(bzumwalt) → needinfo?(jsmith)
For B2G at least, we don't need this on Beta.
Flags: needinfo?(jsmith)
(In reply to Boris Chiou [:boris] from comment #60)
> Created attachment 8441146 [details] [diff] [review]
> Ensure that the delete of RasterImage occurs on the main thread. r=seth
> (carry seth's r+)
> 
> Fix try server build failed:
> 1. Use DebugOnly<nsresult> to fix "error: unused variable 'rv'" on Opt
> builds.
> 2. Remove MOZ_OVERRIDE to fix "error C3665:
> 'mozilla:image:RasterImage::DecodePool:DecodeJob::~DecodeJob': override
> specifier ''override not allowed on a destructor" on Windows builds.
Does this means we already turn on C++11 support on vc?
Flags: needinfo?(boris.chiou)
(In reply to C.J. Ku[:cjku] from comment #66)
> (In reply to Boris Chiou [:boris] from comment #60)
> > Created attachment 8441146 [details] [diff] [review]
> > Ensure that the delete of RasterImage occurs on the main thread. r=seth
> > (carry seth's r+)
> > 
> > Fix try server build failed:
> > 1. Use DebugOnly<nsresult> to fix "error: unused variable 'rv'" on Opt
> > builds.
> > 2. Remove MOZ_OVERRIDE to fix "error C3665:
> > 'mozilla:image:RasterImage::DecodePool:DecodeJob::~DecodeJob': override
> > specifier ''override not allowed on a destructor" on Windows builds.
> Does this means we already turn on C++11 support on vc?

According to the full log of the environment on Windows, we use Visual Studio 2010 to build our code, and it already partially supports override/final keyword (please check http://msdn.microsoft.com/en-us/library/hh567368.aspx).

BTW, if we define _MSC_VER, we will also define MOZ_HAVE_CXX11_OVERRIDE (http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#87), and then MOZ_OVERRIDE is defined as override. Therefore, I think we may already turn on "override" on msvc.

Besides, C3665 means this error on Visual Studio 2010: http://msdn.microsoft.com/en-us/library/vstudio/xfzxhe2a%28v=vs.100%29.aspx
Flags: needinfo?(boris.chiou)
(In reply to Boris Chiou [:boris] from comment #67)
> (In reply to C.J. Ku[:cjku] from comment #66)
> > (In reply to Boris Chiou [:boris] from comment #60)
> > > Created attachment 8441146 [details] [diff] [review]
> > > Ensure that the delete of RasterImage occurs on the main thread. r=seth
> > > (carry seth's r+)
> > > 
> > > Fix try server build failed:
> > > 1. Use DebugOnly<nsresult> to fix "error: unused variable 'rv'" on Opt
> > > builds.
> > > 2. Remove MOZ_OVERRIDE to fix "error C3665:
> > > 'mozilla:image:RasterImage::DecodePool:DecodeJob::~DecodeJob': override
> > > specifier ''override not allowed on a destructor" on Windows builds.
> > Does this means we already turn on C++11 support on vc?
> 
> According to the full log of the environment on Windows, we use Visual
> Studio 2010 to build our code, and it already partially supports
> override/final keyword (please check
> http://msdn.microsoft.com/en-us/library/hh567368.aspx).
> 
> BTW, if we define _MSC_VER, we will also define MOZ_HAVE_CXX11_OVERRIDE
> (http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#87), and
Sorry, the link should be http://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h#65
> then MOZ_OVERRIDE is defined as override. Therefore, I think we may already
> turn on "override" on msvc.
> 
> Besides, C3665 means this error on Visual Studio 2010:
> http://msdn.microsoft.com/en-us/library/vstudio/xfzxhe2a%28v=vs.100%29.aspx
(In reply to Jason Smith [:jsmith] from comment #65)
> For B2G at least, we don't need this on Beta.

*sigh* My question was specifically about *Firefox* 31.
Flags: needinfo?(seth)
(In reply to C.J. Ku[:cjku] from comment #66)
> Does this means we already turn on C++11 support on vc?

This is just a VS2010 bug. It's fixed in the newer versions of Visual Studio. (I can't wait til we can drop some of this old compilers!)
Flags: needinfo?(seth)
(In reply to Ryan VanderMeulen [:RyanVM UTC-4][PTO June 19-22] from comment #69)
> (In reply to Jason Smith [:jsmith] from comment #65)
> > For B2G at least, we don't need this on Beta.
> 
> *sigh* My question was specifically about *Firefox* 31.

There's nothing B2G-specific about this. In my opinion it makes sense to uplift this. I'll request it.
Comment on attachment 8441146 [details] [diff] [review]
Ensure that the delete of RasterImage occurs on the main thread. r=seth (carry seth's r+)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 716140 (multithreaded image decoding)
User impact if declined: Potential crashes.
Testing completed (on m-c, etc.): Landed on m-c 24 hours ago; no reported issues.
Risk to taking this patch (and alternatives if risky): Fairly low risk. This code is run for every single image that is displayed in the browser, so we'd expect problems to show up reasonably quickly. If there's some sort of rare, hard-to-trigger problem still lurking, well, we KNOW that's the case in the existing code! Although this has only baked for 24 hours, given that reasoning, I think that this version of the code is at a minimum just as reliable, and very likely much more reliable, than the code it's replacing.
String or IDL/UUID changes made by this patch: None.
Attachment #8441146 - Flags: approval-mozilla-beta?
Attachment #8441146 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified fixed on Flame device running b2g v2.1

Test Environment
Device: Flame
Base image: v188
Gaia:  a0174f7166745256aaca1cb3aa9f894033fbffa6
Gecko: 43bda3541f6b
Build-ID: 20141028001203
Platform Version: 34.0
OS Version: 2.1.0.0-prerelease
Update Channel: nightly-b2g34
Git Commit Info: 2014-10-28 02:52:05

Steps to reproduce:
1. Load lots of images on the SD Card (photos taken with the phone and / or images copied from your computer).
2. On the phone, launch the Gallery app.
3. Tap on a photo to view it.
4. Swipe your finger left and right on the screen in order to navigate to the next / previous photo on the SD Card.
5. Repeat Step 4 for a few minutes.

Expected Results:
The phone remains stable and does not crash.

Actual Results:
The phone remains stable and does not crash.
Issue verified fixed on Flame 1.4, Flame 2.0, and Flame 2.2

Actual results: No crash observed 

Device: Flame 1.4 (319mb) (Jellybean Bean) (Shallow Flash)
BuildID: 20141027000203
Gaia: bb76c81f83e1e4acc2d2972a451db2bce78c8f34
Gecko: 1bde54b2e7b0
Gonk: 
Version: 30.0 (1.4)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:30.0) Gecko/30.0 Firefox/30.0

Device: Flame 2.0 (319mb)(Kitkat Base)(Full Flash)
BuildID: 20141028000202
Gaia: 5e532a84e762b1bb6231756182cf1465671a061e
Gecko: 124f0bed1700
Gonk: 6e51d9216901d39d192d9e6dd86a5e15b0641a89
Version: 32.0 (2.0)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Device: Flame Master (319mb)(Kitkat Base)(Full Flash)
Build ID: 20141028040202
Gaia: 6a7fb482a03c5083ef79b41e7b0dfab27527cd04
Gecko: a255a234946e
Version: 36.0a1 (Master)
Firmware Version: v188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.