Android crash in nsXPCWrappedJS::AddRef

RESOLVED FIXED in Firefox 24

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: scoobidiver, Assigned: blassey)

Tracking

({crash, topcrash})

24 Branch
mozilla24
All
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox23 unaffected, firefox24+ fixed)

Details

(Whiteboard: [native-crash], crash signature)

Attachments

(2 attachments)

It has already been hit by three users in 24.0a1/20130612. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=86413e921d5d&tochange=0414d6d0f60d
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@1.04.01.01.06.041 (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
SEMC/LT26i_1257-8080/LT26i:4.1.2/6.2.B.0.211/LL__zg:user/release-keys
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 	libxul.so 	nsXPCWrappedJS::AddRef 	js/xpconnect/src/XPCWrappedJS.cpp:158
1 	libxul.so 	libxul.so@0x875577 	
2 	libxul.so 	nsXPTCStubBase::AddRef 	xpcom/reflect/xptcall/src/xptcall.cpp:30
3 	libxul.so 	mozilla::MapsMemoryReporter::Init 	xpcom/base/MapsMemoryReporter.cpp:572
4 	libxul.so 	nsAppShell::CallObserver 	obj-firefox/dist/include/nsISupportsUtils.h:59
5 	libxul.so 	nsAString_internal::ReplacePrep 	obj-firefox/dist/include/nsTSubstring.h:738
6 	libdvm.so 	libdvm.so@0xb193e 	
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsXPCWrappedJS%3A%3AAddRef%28%29
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: http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#735

The MapsMemoryReporter stuff is a fakeout as far as I can tell. Presumably there's a JS observer in play here.
Also, NotifyObserversCaller (http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.cpp#768) 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:
> http://mxr.mozilla.org/mozilla-central/source/widget/android/nsAppShell.
> 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 
https://pvtbuilds.mozilla.org/pub/mozilla.org/b2g/nightly/mozilla-central-unagi-eng/latest/ build from 13.06.2.12


It's reproducible when running the camera tests:
https://github.com/mozilla/gaia-ui-tests/tree/master/gaiatest/tests/camera
tracking-fennec: 24+ → ?
(In reply to Florin Strugariu [:Bebe] from comment #8)
> It's reproducible when running the camera tests:
> https://github.com/mozilla/gaia-ui-tests/tree/master/gaiatest/tests/camera
It's bug 882328, not this one.
Hardware: ARM → All
Posted 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/MemoryMonitor.java
@@ +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 GeckoEvent.java

s/GeckoEvent.java/MemoryMonitor.java/. Also add a comment in MemoryMonitor.java 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+
https://hg.mozilla.org/mozilla-central/rev/f7628d639dd1
https://hg.mozilla.org/mozilla-central/rev/35b943a379dd
Status: NEW → RESOLVED
Closed: 6 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.