Closed Bug 882196 Opened 11 years ago Closed 11 years ago

Android crash in nsXPCWrappedJS::AddRef


(Core :: XPConnect, defect)

24 Branch
Not set



Tracking Status
firefox23 --- unaffected
firefox24 + fixed


(Reporter: scoobidiver, Assigned: blassey)



(Keywords: crash, topcrash, Whiteboard: [native-crash])

Crash Data


(2 files)

It has already been hit by three users in 24.0a1/20130612. The regression range is:
It's likely a regression from bug 770840.

Signature 	nsXPCWrappedJS::AddRef() More Reports Search
UUID	42c24efe-4075-414d-bd33-fe4252130612
Date Processed	2013-06-12 16:18:35
Uptime	511
Last Crash	4.5 days before submission
Install Age	8.5 minutes since version was first installed.
Install Time	2013-06-12 16:09:48
Product	FennecAndroid
Version	24.0a1
Build ID	20130612031138
Release Channel	nightly
OS	Android
OS Version	0.0.0 Linux 3.4.0+1.0.21100-313065-g1ccebb5-00166-g58b3171 #1 SMP PREEMPT Wed May 22 17:36:36 2013 armv7l SEMC/LT26i_1257-8080/LT26i:4.1.2/6.2.B.0.211/LL__zg:user/release-keys
Build Architecture	arm
Build Architecture Info	ARMv0
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
AdapterDescription: 'Qualcomm -- Adreno (TM) 220 -- OpenGL ES 2.0 V@6.0 AU@ (CL@) -- Model: LT26i, Product: LT26i_1257-8080, Manufacturer: Sony Ericsson, Hardware: semc'
GL Layers! EGL? EGL+ GL Context? GL Context+ GL Layers+ 
Sony Ericsson LT26i
Processor Notes 	sp-processor09_phx1_mozilla_com_2330:2012; MDSW emitted too many frames, triggering truncation; exploitability tool: ERROR: unable to analyze dump
EMCheckCompatibility	True
Adapter Vendor ID	Qualcomm
Adapter Device ID	Adreno (TM) 220
Device	Sony Ericsson LT26i
Android API Version	16 (REL)
Android CPU ABI	armeabi-v7a

Frame 	Module 	Signature 	Source
0 	nsXPCWrappedJS::AddRef 	js/xpconnect/src/XPCWrappedJS.cpp:158
2 	nsXPTCStubBase::AddRef 	xpcom/reflect/xptcall/src/xptcall.cpp:30
3 	mozilla::MapsMemoryReporter::Init 	xpcom/base/MapsMemoryReporter.cpp:572
4 	nsAppShell::CallObserver 	obj-firefox/dist/include/nsISupportsUtils.h:59
5 	nsAString_internal::ReplacePrep 	obj-firefox/dist/include/nsTSubstring.h:738

More reports at:
mfinkle -

looks like Android is occasionally addref/releaseing XPCWrappedJS callbacks off-main-thread. This is now forbidden, enforced with runtime aborts. Can you find an owner for this?
Flags: needinfo?(mark.finkle)
Pretty sure this is the problem:

The MapsMemoryReporter stuff is a fakeout as far as I can tell. Presumably there's a JS observer in play here.
Also, NotifyObserversCaller ( could be dangerous - it's instantiated off the main thread and and addrefs a random nsISupports pointers.
NotifyObserversCaller may be safe, since it appears from a cursory glance that the users of NotifyObservers only ever pass null. With respect to CallObserver, if AddObserver is only called on the main thread, mObserversHash could store nsMainThreadPtrHandle objects in a hashtable instead of the current addreffing interface hashtable.
A comment says: "during download"
Blocks: 773610
This part of the code is the domain of the integration team. Assigning to Brad (even though I dislike it when it happens to me) so he can pass it on to someone else.
Assignee: nobody → blassey.bugs
Flags: needinfo?(mark.finkle)
tracking-fennec: ? → 24+
(In reply to Josh Matthews [:jdm] from comment #2)
> Pretty sure this is the problem:
> cpp#735
> The MapsMemoryReporter stuff is a fakeout as far as I can tell. Presumably
> there's a JS observer in play here.

I'm thinking that GeckoAppShell.callObserver() should just be an event that gets processed on the main thread.
we can reproduce this crash in our gaia-UI automation build using build from

It's reproducible when running the camera tests:
tracking-fennec: 24+ → ?
(In reply to Florin Strugariu [:Bebe] from comment #8)
> It's reproducible when running the camera tests:
It's bug 882328, not this one.
Hardware: ARM → All
Attached patch patchSplinter Review
Attachment #763154 - Flags: review?(snorp)
Attachment #763154 - Flags: review?(snorp) → review+
either snorp or kats, whoever gets here first
Attachment #763776 - Flags: review?(snorp)
Attachment #763776 - Flags: review?(bugmail.mozilla)
Comment on attachment 763776 [details] [diff] [review]
patch to remove nsAppShell::NotifyObservers

Review of attachment 763776 [details] [diff] [review]:

Generally looks ok but unless you have a good reason to move the level check into c++ I would rather not do that.

::: mobile/android/base/
@@ +147,5 @@
>          }
> +        if (GeckoThread.checkLaunchState(GeckoThread.LaunchState.GeckoRunning)) {
> +            GeckoAppShell.sendEventToGecko(GeckoEvent.createLowMemoryEvent(level));
> +        }

Why move the level check into c++? This means we will be sending messages to c++ that we know will be ignored, which is inefficient.

::: widget/android/AndroidJavaWrappers.cpp
@@ +693,5 @@
> +        case LOW_MEMORY: {
> +            mMetaState = jenv->GetIntField(jobj, jMetaStateField);
> +            break;
> +        }
> +            

trailing whitespace on the blank line

::: widget/android/AndroidJavaWrappers.h
@@ +712,5 @@
>          dummy_java_enum_list_end
>      };
>      enum {
> +        // Memory pressue levels, keep in sync with those in

s/ Also add a comment in that says it should be kept in sync with AndroidJavaWrappers.h

::: widget/android/nsAppShell.cpp
@@ +527,5 @@
> +        // TODO hook in memory-reduction stuff for different levels here
> +        if (curEvent->MetaState() >= AndroidGeckoEvent::MEMORY_PRESSURE_MEDIUM) {
> +            nsCOMPtr<nsIObserverService> os = mozilla::services::GetObserverService();
> +            if (os)
> +            os->NotifyObservers(nullptr,

indent (and braces, but you probably won't add those anyway...)

@@ +542,5 @@
> +                                NS_NETWORK_LINK_TOPIC,
> +                                nsString(curEvent->Characters()).get());
> +        break;
> +    }
> +  

trailing whitespace on blank line
Attachment #763776 - Flags: review?(snorp)
Comment on attachment 763776 [details] [diff] [review]
patch to remove nsAppShell::NotifyObservers

r+'ing after discussion on IRC; blassey agreed to keep the level-check in Java as well so as to not send useless events to c++.
Attachment #763776 - Flags: review?(bugmail.mozilla) → review+
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
tracking-fennec: ? → ---
You need to log in before you can comment on or make changes to this bug.