898028 - crash @ gfxUtils::ConvertBGRAtoRGBA()

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Toolbar, defect)

Description

[Scott Johnson (:jwir3)]

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]]

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

[Kelsey Gilbert [:jgilbert]]

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

Attachment: Correcting the issue

Comment 4

[Chris Kitching [:ckitching]]

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

https://tbpl.mozilla.org/?tree=Try&rev=b4fb1ca28c7a
Just in case...

Comment 6

[Kelsey Gilbert [:jgilbert]]

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

[Kelsey Gilbert [:jgilbert]]

This isn't a crash, it's an assertion hit, thus DEBUG build only.

Comment 8

[Chris Kitching [:ckitching]]

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

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

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

Patch works for me on a debug build

Comment 12

[Chris Kitching [:ckitching]]

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6b62fd4235

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ab6b62fd4235

Comment 16

[Chris Kitching [:ckitching]]

Why the tracking flag? Didn't this already land way back in 25?

Comment 17

[Aaron Train [:aaronmt]]

Nomination removed as this was fixed before a decision was made, that's all.