Closed Bug 757025 Opened 11 years ago Closed 11 years ago

crash in mozilla::hal::NotifyScreenConfigurationChange


(Core Graveyard :: Widget: Android, defect)

Not set


(firefox14 verified, firefox15 verified, blocking-fennec1.0 +)

Tracking Status
firefox14 --- verified
firefox15 --- verified
blocking-fennec1.0 --- +


(Reporter: scoobidiver, Assigned: kats)



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

Crash Data


(2 files)

It's likely related to or a new form of bug 740329.

Signature 	mozilla::hal::NotifyScreenConfigurationChange More Reports Search
UUID	0f35cd89-5871-408f-a6ab-c02e02120521
Date Processed	2012-05-21 11:43:05
Uptime	2552
Last Crash	3.1 hours before submission
Install Age	18.2 hours since version was first installed.
Install Time	2012-05-20 17:33:26
Product	FennecAndroid
Version	15.0a1
Build ID	20120520030530
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 3.0.8-gda6252b #1 SMP PREEMPT Fri Apr 13 11:35:09 PDT 2012 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
AdapterVendorID: tuna, AdapterDeviceID: Galaxy Nexus.
AdapterDescription: 'Model: 'Galaxy Nexus', Product: 'yakju', Manufacturer: 'samsung', Hardware: 'tuna''.
samsung Galaxy Nexus
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	mozilla::hal::NotifyScreenConfigurationChange 	
1 	nsAppShell::ProcessNextNativeEvent 	widget/android/nsAppShell.cpp:586
2 	nsBaseAppShell::DoProcessNextNativeEvent 	widget/xpwidgets/nsBaseAppShell.cpp:172
3 	nsBaseAppShell::OnProcessNextEvent 	widget/xpwidgets/nsBaseAppShell.cpp:313
4 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:618
5 	NS_ProcessNextEvent_P 	obj-firefox/xpcom/build/nsThreadUtils.cpp:245
6 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:114
7 	MessageLoop::RunInternal 	ipc/chromium/src/base/
8 	MessageLoop::Run 	ipc/chromium/src/base/
9 	nsBaseAppShell::Run 	widget/xpwidgets/nsBaseAppShell.cpp:196
10 	nsAppStartup::Run 	toolkit/components/startup/nsAppStartup.cpp:296
11 	XREMain::XRE_mainRun 	toolkit/xre/nsAppRunner.cpp:3792
12 	XREMain::XRE_main 	toolkit/xre/nsAppRunner.cpp:3869
13 	XRE_main 	toolkit/xre/nsAppRunner.cpp:3945

More reports at:
Yes, I was crashing here.
I was hoping I could get a similar testcase as bug 735114, but am unable to, thus far.
(In reply to Scoobidiver from comment #0)
> It's likely related to or a new form of bug 740329.

Yeah, bug 745145 changed NotifyScreenOrientationChange to NotifyScreenConfigurationChange so they're the same bug. I duped that one to here since this reflects the latest code in m-c. Also cc'ing Chris Jones who seems to have touched this code a fair bit.
I could sort of reproduce it with this:
You need to allow popups.
And in order to have a chance of reproducing, you need to switch orientation portrait/landscape while the testcase is running.

I managed to reproduce it a couple of times on my Galaxy Nexus, but it is very difficult.
I am adding the signature from 14.0 in order to track it for the first native version.
It's #28 top crasher in 14.0b2, and #16 if you remove fixed crashes.
Crash Signature: [@ mozilla::hal::NotifyScreenConfigurationChange] → [@ mozilla::hal::NotifyScreenConfigurationChange] [@ mozilla::hal::NotifyScreenOrientationChange]
Depends on: 757821
This crash has jumped to #1 for 15.0a1.
blocking-fennec1.0: --- → ?
Keywords: topcrash
(In reply to Chris Peterson (:cpeterson) from comment #8)
> This crash has jumped to #1 for 15.0a1.
Because one user found STR.
Keywords: topcrash
Yeah, that user would be me. I guess topcrash data is easy to influence.
Assignee: nobody → bugmail.mozilla
blocking-fennec1.0: ? → +
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #10)
> I guess topcrash data is easy to influence.

I suppose that's a really good problem to have :)
(In reply to Kartikaya Gupta (:kats) from comment #11)
> I suppose that's a really good problem to have :)

Well, in this case it's because we have so few ADUs - ~800 for native Nightly.
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #5)
> I managed to reproduce it a couple of times on my Galaxy Nexus, but it is
> very difficult.

Any chance you still have the logcat from when you reproduced it?
I think I figured out why this is happening. The code in RemoveObserver [1] deletes the mObservers array when all observers have been removed, and also disables notifications. In a single-threaded world this is fine, as no further notifications will come through. On Android (maybe B2G as well?), however, the screen orientation notification is asynchronous (sent as a message from java->gecko) and so there can be a notification in-flight when this happens. The notification gets processed when mObservers is null at [2] and causes the sadness.

I was able to artificially induce the scenario by applying the attached patch, which forces a screen orientation notification at exactly the wrong time, and loading :mw22's test page. The MOZ_ASSERT(mObservers) trips and SIGSEGV's in my debug build, but on a release build I assume it would continue after spitting out a warning or something. The fix should be to return there in the non-debug case.

Attached patch PatchSplinter Review
Attachment #626781 - Flags: review?(jones.chris.g)
Comment on attachment 626781 [details] [diff] [review]

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

::: hal/Hal.cpp
@@ +206,5 @@
>      }
>    }
>    void BroadcastInformation(const InfoType& aInfo) {
>      MOZ_ASSERT(mObservers);

kats, if NULL mObservers is a valid case, should we remove the assert?
Comment on attachment 626781 [details] [diff] [review]

Thanks for debugging this.

>diff --git a/hal/Hal.cpp b/hal/Hal.cpp

>   void BroadcastInformation(const InfoType& aInfo) {
>     MOZ_ASSERT(mObservers);

Please remove this assert and add a comment about why we're null-checking.

r=me with that.
Attachment #626781 - Flags: review?(jones.chris.g) → review+
Landed with the assert removed and a comment added.
Target Milestone: --- → mozilla15
Closed: 11 years ago
Resolution: --- → FIXED
Comment on attachment 626781 [details] [diff] [review]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: crashes
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): mobile blocker
String or UUID changes made by this patch: none
Attachment #626781 - Flags: approval-mozilla-aurora?
Attachment #626781 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
I was not able to reproduce this crash using Martijn's testcase (see comment #5)

Verified fixed on:
Nightly 15.0a1 (2012-05-29) 
Aurora 14.0a2 (2012-05-29)

Device: Galaxy Nexus (Android 4.0.2)
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.