882196 - Android crash in nsXPCWrappedJS::AddRef

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Scoobidiver (away)]

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

Comment 1

[Bobby Holley (:bholley)]

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?

Comment 2

[Josh Matthews [:jdm]]

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.

Comment 3

[Josh Matthews [:jdm]]

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.

Comment 4

[Josh Matthews [:jdm]]

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.

Comment 5

[Scoobidiver (away)]

A comment says: "during download"

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

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.

Comment 7

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 8

[Florin Strugariu [:Bebe]]

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

Comment 9

[Scoobidiver (away)]

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

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 11

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch to remove nsAppShell::NotifyObservers

either snorp or kats, whoever gets here first

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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

Comment 13

[Kartikaya Gupta (email:[e-mail])]

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++.

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f7628d639dd1
https://hg.mozilla.org/mozilla-central/rev/35b943a379dd