Marcia Knous [:marcia] Reporter
	Description • 6 years ago

This bug was filed from the Socorro interface and is
report bp-8f7aae82-8063-4131-9ebe-f0ae30180809.
=============================================================

Seen while looking at Android crash stats on release: https://bit.ly/2MumaYo. Not sure where to bucket it, but it appears many of the signatures are potential UAFs so marking security sensitive. Only one crash seen in b62 so far.

Unfortunately comments are not useful.  

Top 10 frames of crashing thread:

0 libxul.so nsTArray_Impl<BroadcastListener*, nsTArrayInfallibleAllocator>::Clear 
1 libxul.so wcsrtombs 
2 libxul.so mozilla::jni::detail::ProxyNativeCall<mozilla::GeckoScreenOrientation, mozilla::java::GeckoScreenOrientation, true, false, short, short>::operator widget/android/jni/Natives.h:418
3 libxul.so mozilla::detail::RunnableFunction<mozilla::jni::detail::ProxyNativeCall<mozilla::GeckoScreenOrientation, mozilla::java::GeckoScreenOrientation, true, false, short, short> >::Run xpcom/threads/nsThreadUtils.h:552
4 libxul.so nsThread::ProcessNextEvent xpcom/threads/nsThread.cpp:1090
5 libc.so libc.so@0x1956b 
6 libxul.so wcsrtombs 
7 libxul.so mozilla::detail::VariantImplementation<unsigned char, 0, const int, const char*, void > > mfbt/Variant.h:219
8 libxul.so mozilla::PerformanceCounter::GetDispatchCount android-ndk/sources/cxx-stl/llvm-libc++/include/atomic:885
9 libxul.so wcsrtombs 

=============================================================

	Daniel Veditz [:dveditz]
	Comment 1 • 6 years ago

Who should look at this explosive new crash?

	James Willcox (:snorp) (jwillcox@mozilla.com) (he/him)
	Comment 2 • 6 years ago

It seems like this may be a variant of bug 1476250. What do you think, :gsvelto?

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 3 • 6 years ago

It could be, most of the stack seems garbled except for the part about calling OnOrientationChange(). The crashes look like UAFs alright since >80% have an e5e5e5e5 value as the crashing address and this would be consistent with bug 1476250 which is basically the same problem: the last screen orientation observer is unregistered, the observer array is deleted and then something tries to touch. There's something important here I hadn't noticed in the other bug: the crashing thread is _not_ the main thread and that's just wrong because the HAL shouldn't be accessed from other threads. I'll first have to check where the JNI code is calling OnOrientationChange() because if it's not from the main thread then that should be sorted out first.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 4 • 6 years ago

Scratch what I wrote before, the notification is sent on the correct thread. I have a hard time figuring out how the race can possibly happen. The patch that I'm writing for bug 1476250 should address this issue too though by never destroying the list of observers as long as the service is in use thus entirely preventing the race.

	David Bolter [:davidb] (NeedInfo me for attention)
	Comment 5 • 6 years ago

Updating assignee based on comment 4. Thanks!

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 6 • 6 years ago

The fix for this issue is undergoing review in bug 1476250 where I had already started working on it before this bug was filed.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 7 • 6 years ago

I'm about to land the patch for bug 1476250, it should fix this too.

	Gabriele Svelto [:gsvelto] (PTO) Assignee
	Comment 8 • 6 years ago

There have been no instances of this crash for versions including the fix in bug 1476250, closing as fixed.

	Sorina Florean [:sflorean]
	Comment 9 • 6 years ago

Tested with the following devices on 63.0b13:
- Motorola Nexus 6(Android 7.1.1)
- Nexus 9(Android 6.0.1)
- Sony Xperia Z5 Premium(Android 6.0.1)
- Xiaomi Mi4i(Android 5.0.2)
We didn't see the crash while performing the steps from bug 1476250.