1003893 - crash in imgFrame::~imgFrame()

Status: VERIFIED FIXED

(Core :: Graphics, defect)

Description

[Brogan Zumwalt [Inactive]]

Attachment: 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

Comment 1

[Natalya Kot [:nkot]]

'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)

Comment 2

[Jason Smith [:jsmith]]

I'd also like to know if this is consistently reproducible with the STR given above.

Comment 3

[Brogan Zumwalt [Inactive]]

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

Comment 4

[J Zimbrick]

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%

Comment 5

[Brogan Zumwalt [Inactive]]

Assigning self as QA Contact, finding regression window

Comment 6

[Brogan Zumwalt [Inactive]]

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

Comment 7

[Pi Wei Cheng [:piwei] (inactive)]

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.

Comment 8

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Hi Jet, could you find someone to take a look at this please?

Comment 9

[Jet Villegas (inactive)]

Crash in the imgFrame destructor indicates a double-free. Need logs from the crashing device please.

Comment 10

[Jason Smith [:jsmith]]

(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.

Comment 11

[Jet Villegas (inactive)]

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?

Comment 12

[Jason Smith [:jsmith]]

Ok - adding qawanted to get a longer logcat when this crash reproduces?

Comment 13

[Pi Wei Cheng [:piwei] (inactive)]

Attachment: logcat_completeRepro.txt

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.

Comment 14

[Michael Wu [:mwu]]

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.

Comment 15

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

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.

Comment 17

[Milan Sreckovic [:milan] (needinfo for best results)]

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.

Comment 18

[Peter Chang[:pchang]]

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?

Comment 19

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Peter, I sent you an email.

Comment 20

[Boris Chiou [:boris]]

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

Comment 21

[Boris Chiou [:boris]]

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

Comment 22

[u459114]

(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?

Comment 23

[Peter Chang[:pchang]]

(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).
> }

Comment 25

[Benjamin Chen [:bechen]]

(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().

Comment 26

[Boris Chiou [:boris]]

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.

Comment 27

[Sotaro Ikeda [:sotaro]]

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.

Comment 28

[Jason Smith [:jsmith]]

I'm not convinced this is the same bug as bug 1015863 because the STR only happen on 2.0, not 1.4.

Comment 30

[Boris Chiou [:boris]]

Attachment: Use WeakPtr to protect RasterImage

1. Root cause:
   When DiscardTracker called RasterImage::Discard() (main thread), DecodeJob
   also held the nsRefPtr and the mRefCnt == 1 (Decodejob thread).
   After DecodeJob was finished, it deleted this RasterImage, and then
   RasterImage::Discard() tried to access the invalid address (0x5a5a5a5a), ex.
   delete imgFrame object again. Therefore, we should avoid other threads
   delete RasterImage while DiscardTracker is discarding the RasterImage.

2. Idea:
   we use a WeakPtr to protect the RasterImage in DiscardTrack::Node.
   (Note: We shouldn't use nsRefPtr<RasterImage> which refs to itself.)

Comment 31

[u459114]

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);

You may not use assertion here. Instead you need validate img at run time.
if (img) {
  MOZ_ASSERT(img->CanDiscard());
   MOZ_ASSERT(!img->mAnim);
}

@@ +133,5 @@
>    // Be careful: Calling Discard() on an image might cause it to be removed
>    // from the list!
>    Node *n;
>    while ((n = sDiscardableImages.popFirst())) {
> +    nsRefPtr<RasterImage> img = n->img.get();

if (img) {
  img->Discard();
}

@@ +303,5 @@
>          sCurrentDecodedImageBytes > sMaxDecodedImageKB * 1024) {
>  
>        // Discarding the image should cause sCurrentDecodedImageBytes to
>        // decrease via a call to InformDeallocation().
> +      nsRefPtr<RasterImage> img = node->img.get();

if (img) {
  img->Discard();
}

Comment 32

[Boris Chiou [:boris]]

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);

Great! Thanks.

@@ +133,5 @@
>    // Be careful: Calling Discard() on an image might cause it to be removed
>    // from the list!
>    Node *n;
>    while ((n = sDiscardableImages.popFirst())) {
> +    nsRefPtr<RasterImage> img = n->img.get();

OK, I forgot to add this check while cleaning up and rebasing this patch. Thanks.

Comment 33

[Jerry Shih[:jerry] (UTC+8) (inactive)]

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?

Comment 34

[Boris Chiou [:boris]]

Attachment: Use WeakPtr to protect RasterImage

1. Root cause:
   When DiscardTracker called RasterImage::Discard() (main thread), DecodeJob
   also held the nsRefPtr and the mRefCnt == 1 (Decodejob thread).
   After DecodeJob was finished, it deleted this RasterImage, and then
   RasterImage::Discard() tried to access the invalid address (0x5a5a5a5a),
   ex. delete imgFrame object again. Therefore, we should avoid other threads
   delete RasterImage while DiscardTracker is discarding the RasterImage.

2. Idea:
   we use a WeakPtr to protect the RasterImage in DiscardTrack::Node.
   (Note: We shouldn't use nsRefPtr<RasterImage> which refs to itself.)

Comment 35

[Boris Chiou [:boris]]

(In reply to Jerry Shih[:jerry] from comment #33)
> 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?

Good question. I saw some people also called get() function to convert from WeakPtr. Do we have a better way to convert it?

Comment 36

[Boris Chiou [:boris]]

Steven, could you please give us any comment? Thank you.

(In reply to Boris Chiou [:boris] from comment #35)
> (In reply to Jerry Shih[:jerry] from comment #33)
> > 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?
> 
> Good question. I saw some people also called get() function to convert from
> WeakPtr. Do we have a better way to convert it?

Comment 37

[u459114]

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

Comment 38

[u459114]

(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 39

[Boris Chiou [:boris]]

Attachment: Force to Dispatch RasterImage to the main thread

Dispatch nsRefPtr<RasterImage> to the main thread to avoid
RasterImage from being destructed by the DecodeJob thread.
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Comment 40

[u459114]

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

Comment 41

[u459114]

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

Comment 42

[Boris Chiou [:boris]]

Attachment: Force to Dispatch RasterImage to the main thread

Dispatch nsRefPtr<RasterImage> to the main thread to avoid
RasterImage from being destructed by the DecodeJob thread.
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Comment 43

[Benjamin Chen [:bechen]]

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 44

[StevenLee[:slee]]

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.

Comment 45

[Boris Chiou [:boris]]

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

Comment 46

[Peter Chang[:pchang]]

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

Comment 47

[Boris Chiou [:boris]]

Attachment: Force to Dispatch RasterImage to the main thread

Dispatch nsRefPtr<RasterImage> to the main thread to avoid
RasterImage from being destructed by the DecodeJob thread.
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Comment 48

[Boris Chiou [:boris]]

(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.

Comment 49

[Boris Chiou [:boris]]

Attachment: Force to Dispatch RasterImage to the main thread

Dispatch nsRefPtr<RasterImage> to the main thread to avoid
RasterImage from being destructed by the DecodeJob thread.
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Comment 50

[u459114]

(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!

Comment 51

[Benjamin Chen [:bechen]]

I think we can just call NS_ProxyRelease(mainthread, mImage.forget()).

Comment 52

[Boris Chiou [:boris]]

(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.

Comment 53

[Boris Chiou [:boris]]

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.

Comment 54

[Boris Chiou [:boris]]

Attachment: Ensure that the delete of RasterImage occurs on the main thread

Dispatch nsRefPtr<RasterImage> to the main thread by NS_ProxyRelease
to avoid RasterImage from being destructed by the DecodeJob thread.
(If gMultithreadedDecoding is true.)
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Comment 55

[Boris Chiou [:boris]]

Attachment: Ensure that the delete of RasterImage occurs on the main thread

Dispatch nsRefPtr<RasterImage> to the main thread by NS_ProxyRelease
to avoid RasterImage from being destructed by the DecodeJob thread.
(If gMultithreadedDecoding is true.)
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Comment 56

[u459114]

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

Comment 57

[Boris Chiou [:boris]]

Attachment: Ensure that the delete of RasterImage occurs on the main thread

Dispatch nsRefPtr<RasterImage> to the main thread by NS_ProxyRelease
to avoid RasterImage from being destructed by the DecodeJob thread.
(If gMultithreadedDecoding is true.)
Therefore, DiscardTracker can safely access RasterImage::Discard()
by the raw pointer in DiscardTracker::Node.

Add an assertion in this patch.

Comment 58

[Seth Fowler [:seth] [:s2h]]

(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 59

[Seth Fowler [:seth] [:s2h]]

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.

Comment 60

[Boris Chiou [:boris]]

Attachment: 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.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=44cf23b06642

Comment 61

[Ryan VanderMeulen [:RyanVM]]

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?

Comment 62

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3060e6862f15

Comment 63

[Ryan VanderMeulen [:RyanVM]]

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 :)

Comment 64

[Brogan Zumwalt [Inactive]]

Passing needinfo to jsmith as I don't believe I have the authority to advise here. :D

Comment 65

[Jason Smith [:jsmith]]

For B2G at least, we don't need this on Beta.

Comment 66

[u459114]

(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?

Comment 67

[Boris Chiou [:boris]]

(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

Comment 68

[Boris Chiou [:boris]]

(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

Comment 69

[Ryan VanderMeulen [:RyanVM]]

(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.

Comment 70

[Seth Fowler [:seth] [:s2h]]

(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!)

Comment 71

[Seth Fowler [:seth] [:s2h]]

(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 72

[Seth Fowler [:seth] [:s2h]]

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.

Comment 73

[:Gijs (he/him)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/9bf59a51e04d

Comment 74

[Parul Mathur [:pragmatic] - out of office - please ni? [:bajaj] instead]

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.

Comment 75

[Brogan Zumwalt [Inactive]]

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