crash @ gfxUtils::ConvertBGRAtoRGBA()

RESOLVED FIXED in Firefox 25

Status

()

Firefox for Android
Graphics, Panning and Zooming
RESOLVED FIXED
4 years ago
10 months ago

People

(Reporter: jwir3, Assigned: ckitching)

Tracking

({assertion})

unspecified
Firefox 25
ARM
Android
assertion
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(crash signature)

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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
(Reporter)

Updated

4 years ago
OS: Linux → Android
Hardware: x86_64 → ARM
(Assignee)

Comment 1

4 years ago
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"..?
(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.

Updated

4 years ago
Severity: normal → critical
Crash Signature: gfxUtils::ConvertBGRAtoRGBA (aSourceSurface=0x74ba3b20, aDestSurface=0x74ba3b20) → [@ gfxUtils::ConvertBGRAtoRGBA(gfxImageSurface*, gfxImageSurface*)]
Keywords: crash
Whiteboard: [native-crash]

Updated

4 years ago
tracking-fennec: --- → ?
(Assignee)

Updated

4 years ago
Assignee: nobody → ckitching
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 3

4 years ago
Created attachment 781847 [details] [diff] [review]
Correcting the issue
(Assignee)

Updated

4 years ago
Attachment #781847 - Attachment description: thumbnailVictory.patch → Correcting the issue
Attachment #781847 - Attachment is obsolete: true
(Assignee)

Comment 4

4 years ago
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...
Attachment #781852 - Flags: review?(jgilbert)
(Assignee)

Comment 5

4 years ago
https://tbpl.mozilla.org/?tree=Try&rev=b4fb1ca28c7a
Just in case...
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 ||.
Attachment #781852 - Flags: review?(jgilbert) → review+
This isn't a crash, it's an assertion hit, thus DEBUG build only.
Severity: critical → normal
Keywords: crash → assertion
Whiteboard: [native-crash]
(Assignee)

Comment 8

4 years ago
(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..
(Assignee)

Comment 9

4 years ago
(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

4 years ago
(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?
Patch works for me on a debug build
(Assignee)

Comment 12

4 years ago
(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.
Keywords: assertion → checkin-needed
(Assignee)

Updated

4 years ago
Keywords: assertion
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab6b62fd4235
Keywords: checkin-needed
Duplicate of this bug: 899472
https://hg.mozilla.org/mozilla-central/rev/ab6b62fd4235
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
tracking-fennec: ? → ---
(Assignee)

Comment 16

4 years ago
Why the tracking flag? Didn't this already land way back in 25?
Nomination removed as this was fixed before a decision was made, that's all.
You need to log in before you can comment on or make changes to this bug.