Last Comment Bug 898028 - crash @ gfxUtils::ConvertBGRAtoRGBA()
: crash @ gfxUtils::ConvertBGRAtoRGBA()
Status: RESOLVED FIXED
: assertion
Product: Firefox for Android
Classification: Client Software
Component: Graphics, Panning and Zooming (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 25
Assigned To: Chris Kitching [:ckitching]
:
: Kartikaya Gupta (email:kats@mozilla.com)
Mentors:
: 899472 (view as bug list)
Depends on:
Blocks: 896822
  Show dependency treegraph
 
Reported: 2013-07-25 09:56 PDT by Scott Johnson (:jwir3)
Modified: 2016-07-29 14:34 PDT (History)
8 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Correcting the issue (4.35 KB, patch)
2013-07-26 11:54 PDT, Chris Kitching [:ckitching]
no flags Details | Diff | Splinter Review
Correct the assertation. (1.30 KB, patch)
2013-07-26 11:58 PDT, Chris Kitching [:ckitching]
jgilbert: review+
Details | Diff | Splinter Review

Description Scott Johnson (:jwir3) 2013-07-25 09:56:49 PDT
Could be a regression from bug 896822.

STR: 
1) On a Samsung Galaxy S2, open Fennec.
2) Tap on one of the about:home thumbnails to navigate to a commonly viewed site. This site should load properly.
3) Tap on the awesome bar and navigate to bugzilla.mozilla.org.
4) Crash with the following BT:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 25850]
0x69178f50 in gfxUtils::ConvertBGRAtoRGBA (aSourceSurface=0x74ba3b20, aDestSurface=0x74ba3b20)
    at /home/sjohnson/Source/mozilla-android/mozilla/gfx/thebes/gfxUtils.cpp:156
156	    MOZ_ASSERT(aSourceSurface->Format() == gfxASurface::ImageFormatARGB32,
(gdb) bt
#0  0x69178f50 in gfxUtils::ConvertBGRAtoRGBA (aSourceSurface=0x74ba3b20, aDestSurface=0x74ba3b20)
    at /home/sjohnson/Source/mozilla-android/mozilla/gfx/thebes/gfxUtils.cpp:156
#1  0x690f023a in mozilla::AndroidBridge::CaptureThumbnail (this=0x5be50400, window=0x5bf84760, bufW=240, 
    bufH=171, tabId=0, buffer=0x1d700252)
    at /home/sjohnson/Source/mozilla-android/mozilla/widget/android/AndroidBridge.cpp:2733
#2  0x690fb78c in ThumbnailRunnable::Run (this=0x6eafb6c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/widget/android/nsAppShell.cpp:97
#3  0x678ba632 in DispatchToMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)()> (
    obj=0x6eafb6c0, method=&virtual table offset 12, arg=...)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/tuple.h:383
#4  0x678ba3f4 in RunnableMethod<mozilla::dom::ContentParent, void (mozilla::dom::ContentParent::*)(), Tuple0>::Run (this=0x6eafb6e0) at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/task.h:307
#5  0x67ef1262 in MessageLoop::RunTask (this=0x5be620c0, task=0x6eafb6e0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/message_loop.cc:338
#6  0x67ef0f30 in MessageLoop::ProcessNextDelayedNonNestableTask (this=0x5be620c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/message_loop.cc:236
#7  0x67ef1702 in MessageLoop::DoIdleWork (this=0x5be620c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/message_loop.cc:477
#8  0x6790a72e in mozilla::ipc::MessagePump::Run (this=0x5be01be0, aDelegate=0x5be620c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/glue/MessagePump.cpp:108
#9  0x67ef0ed0 in MessageLoop::RunInternal (this=0x5be620c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/message_loop.cc:220
#10 0x67ef0e72 in MessageLoop::RunHandler (this=0x5be620c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/message_loop.cc:213
#11 0x67ef0e52 in MessageLoop::Run (this=0x5be620c0)
    at /home/sjohnson/Source/mozilla-android/mozilla/ipc/chromium/src/base/message_loop.cc:187
#12 0x6911317c in nsBaseAppShell::Run (this=0x5be10f20)
    at /home/sjohnson/Source/mozilla-android/mozilla/widget/xpwidgets/nsBaseAppShell.cpp:163
#13 0x68f4296e in nsAppStartup::Run (this=0x6ea54100)
    at /home/sjohnson/Source/mozilla-android/mozilla/toolkit/components/startup/nsAppStartup.cpp:269
#14 0x677db99a in XREMain::XRE_mainRun (this=0x5ce6aa80)
    at /home/sjohnson/Source/mozilla-android/mozilla/toolkit/xre/nsAppRunner.cpp:3858
#15 0x677dbbb0 in XREMain::XRE_main (this=0x5ce6aa80, argc=7, argv=0x5be36148, aAppData=0x5b72cdd0 <sAppData>)
    at /home/sjohnson/Source/mozilla-android/mozilla/toolkit/xre/nsAppRunner.cpp:3926
#16 0x677dbd46 in XRE_main (argc=7, argv=0x5be36148, aAppData=0x5b72cdd0 <sAppData>, aFlags=0)
    at /home/sjohnson/Source/mozilla-android/mozilla/toolkit/xre/nsAppRunner.cpp:4128
#17 0x677d08d6 in GeckoStart (data=0x5be44280, appData=0x5b72cdd0 <sAppData>)
    at /home/sjohnson/Source/mozilla-android/mozilla/toolkit/xre/nsAndroidStartup.cpp:73
#18 0x5b6c45fc in Java_org_mozilla_gecko_mozglue_GeckoLoader_nativeRun (jenv=0x13089c0, jc=0x21800001, 
    jargs=0x1fc00005) at /home/sjohnson/Source/mozilla-android/mozilla/mozglue/android/APKOpen.cpp:379
#19 0x40891d34 in dvmPlatformInvoke () from /home/sjohnson/Downloads/jimdb-arm/lib/69e60196/system/lib/libdvm.so
#20 0x408cb488 in dvmCallJNIMethod(unsigned int const*, JValue*, Method const*, Thread*) ()
   from /home/sjohnson/Downloads/jimdb-arm/lib/69e60196/system/lib/libdvm.so
#21 0x408ccd52 in dvmResolveNativeMethod(unsigned int const*, JValue*, Method const*, Thread*) ()
   from /home/sjohnson/Downloads/jimdb-arm/lib/69e60196/system/lib/libdvm.so
#22 0x408a3b90 in dvmJitToInterpNoChain ()
   from /home/sjohnson/Downloads/jimdb-arm/lib/69e60196/system/lib/libdvm.so
#23 0x408a3b90 in dvmJitToInterpNoChain ()
   from /home/sjohnson/Downloads/jimdb-arm/lib/69e60196/system/lib/libdvm.so
Comment 1 Chris Kitching [:ckitching] 2013-07-25 13:23:19 PDT
It seems that this crash only occurs when assertions are enabled. When disabled, it both does not crash and does the proper conversion - so I guess the assertation here: https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#156 
Is not correct?

It seems strange that a convert-to-RGBA-from-BGRA function has a check for "Is ARGB"..?
Comment 2 Jeff Gilbert [:jgilbert] 2013-07-25 18:42:39 PDT
(In reply to Chris Kitching [:ckitching] from comment #1)
> It seems that this crash only occurs when assertions are enabled. When
> disabled, it both does not crash and does the proper conversion - so I guess
> the assertation here:
> https://mxr.mozilla.org/mozilla-central/source/gfx/thebes/gfxUtils.cpp#156 
> Is not correct?
> 
> It seems strange that a convert-to-RGBA-from-BGRA function has a check for
> "Is ARGB"..?

Yes, but no, this is correct. ImageFormatARGB32 is 0xAARRGGBB, or {BB, GG, RR, AA}, BGRA. Really though, this is just asserting that we have an alpha channel. What the regressing bug was trying to do was use this to switch from BGRX to RGBX. We should probably just assert that src and dest formats are the same, and that they're either RGBA or RGBX. (We are probably fine leaving the name of the function alone, since RGBX is fine to pretend to be RGBA in this case)

We probably don't want to allow conversion here between BGRX and RGBA, though.
Comment 3 Chris Kitching [:ckitching] 2013-07-26 11:54:22 PDT
Created attachment 781847 [details] [diff] [review]
Correcting the issue
Comment 4 Chris Kitching [:ckitching] 2013-07-26 11:58:39 PDT
Created attachment 781852 [details] [diff] [review]
Correct the assertation.

Tiny patch tweaking the assertation so this problem no longer exists. Should probably try and land this fairly quickly...
Comment 5 Chris Kitching [:ckitching] 2013-07-26 12:17:04 PDT
https://tbpl.mozilla.org/?tree=Try&rev=b4fb1ca28c7a
Just in case...
Comment 6 Jeff Gilbert [:jgilbert] 2013-07-26 14:27:41 PDT
Comment on attachment 781852 [details] [diff] [review]
Correct the assertation.

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

Fix the formatting and it's good.

::: gfx/thebes/gfxUtils.cpp
@@ +152,5 @@
>  
>      MOZ_ASSERT(aSourceSurface->Stride() == aSourceSurface->Width() * 4,
>                 "Source surface stride isn't tightly packed");
>  
> +    MOZ_ASSERT(aSourceSurface->Format() == gfxASurface::ImageFormatARGB32 || aSourceSurface->Format() == gfxASurface::ImageFormatRGB24,

This line is way too long. Newline and reindent the part after the ||.
Comment 7 Jeff Gilbert [:jgilbert] 2013-07-26 14:29:24 PDT
This isn't a crash, it's an assertion hit, thus DEBUG build only.
Comment 8 Chris Kitching [:ckitching] 2013-07-26 19:57:03 PDT
(In reply to Chris Kitching [:ckitching] from comment #5)
> https://tbpl.mozilla.org/?tree=Try&rev=b4fb1ca28c7a
> Just in case...

Still no good - creates oranges with seemingly the same signature as was reported here. There's a third case we haven't thought about, it would seem..
Comment 9 Chris Kitching [:ckitching] 2013-07-28 19:08:34 PDT
(In reply to Chris Kitching [:ckitching] from comment #8)
> (In reply to Chris Kitching [:ckitching] from comment #5)
> > https://tbpl.mozilla.org/?tree=Try&rev=b4fb1ca28c7a
> > Just in case...
> 
> Still no good - creates oranges with seemingly the same signature as was
> reported here. There's a third case we haven't thought about, it would seem..

And upon retry they went green... Yay for intermittents.

Are those anything to worry about, or are we done here?
Comment 10 :Margaret Leibovic 2013-07-29 08:49:40 PDT
(In reply to Chris Kitching [:ckitching] from comment #9)
> (In reply to Chris Kitching [:ckitching] from comment #8)
> > (In reply to Chris Kitching [:ckitching] from comment #5)
> > > https://tbpl.mozilla.org/?tree=Try&rev=b4fb1ca28c7a
> > > Just in case...
> > 
> > Still no good - creates oranges with seemingly the same signature as was
> > reported here. There's a third case we haven't thought about, it would seem..
> 
> And upon retry they went green... Yay for intermittents.
> 
> Are those anything to worry about, or are we done here?

Is it a new intermittent orange? Or is there an existing bug on file for it?
Comment 11 Aaron Train [:aaronmt] 2013-07-29 11:10:44 PDT
Patch works for me on a debug build
Comment 12 Chris Kitching [:ckitching] 2013-07-29 11:33:23 PDT
(In reply to :Margaret Leibovic from comment #10)
>
> Is it a new intermittent orange? Or is there an existing bug on file for it?

Looks like they're existing ones. Okay, we're done here.
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-07-29 13:25:51 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6b62fd4235
Comment 14 Kartikaya Gupta (email:kats@mozilla.com) 2013-07-30 05:16:54 PDT
*** Bug 899472 has been marked as a duplicate of this bug. ***
Comment 15 Ryan VanderMeulen [:RyanVM] 2013-07-30 10:29:38 PDT
https://hg.mozilla.org/mozilla-central/rev/ab6b62fd4235
Comment 16 Chris Kitching [:ckitching] 2013-09-17 18:50:23 PDT
Why the tracking flag? Didn't this already land way back in 25?
Comment 17 Aaron Train [:aaronmt] 2013-09-18 08:23:18 PDT
Nomination removed as this was fixed before a decision was made, that's all.

Note You need to log in before you can comment on or make changes to this bug.