Closed
Bug 882196
Opened 10 years ago
Closed 10 years ago
Android crash in nsXPCWrappedJS::AddRef
Categories
(Core :: XPConnect, defect)
Tracking
()
RESOLVED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox23 | --- | unaffected |
firefox24 | + | fixed |
People
(Reporter: scoobidiver, Assigned: blassey)
References
Details
(Keywords: crash, topcrash, Whiteboard: [native-crash])
Crash Data
Attachments
(2 files)
9.86 KB,
patch
|
snorp
:
review+
|
Details | Diff | Splinter Review |
12.09 KB,
patch
|
kats
:
review+
|
Details | Diff | Splinter Review |
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•10 years ago
|
||
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)
Comment 2•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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 6•10 years ago
|
||
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)
Updated•10 years ago
|
tracking-fennec: ? → 24+
Assignee | ||
Comment 7•10 years ago
|
||
(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•10 years ago
|
||
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+ → ?
Reporter | ||
Comment 9•10 years ago
|
||
(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.
Reporter | ||
Updated•10 years ago
|
Hardware: ARM → All
Assignee | ||
Comment 10•10 years ago
|
||
Attachment #763154 -
Flags: review?(snorp)
Updated•10 years ago
|
Attachment #763154 -
Flags: review?(snorp) → review+
Assignee | ||
Comment 11•10 years ago
|
||
either snorp or kats, whoever gets here first
Attachment #763776 -
Flags: review?(snorp)
Attachment #763776 -
Flags: review?(bugmail.mozilla)
Comment 12•10 years ago
|
||
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 13•10 years ago
|
||
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+
Updated•10 years ago
|
Comment 14•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/f7628d639dd1 https://hg.mozilla.org/mozilla-central/rev/35b943a379dd
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Updated•10 years ago
|
tracking-fennec: ? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•