757025 - crash in mozilla::hal::NotifyScreenConfigurationChange

Status: VERIFIED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Scoobidiver (away)]

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
google/yakju/maguro:4.0.4/IMM76I/330937:user/release-keys
EMCheckCompatibility	True

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

More reports at:
https://crash-stats.mozilla.com/report/list?signature=mozilla%3A%3Ahal%3A%3ANotifyScreenConfigurationChange

Comment 1

[Martijn Wargers (dead)]

Yes, I was crashing here.

Comment 2

[Martijn Wargers (dead)]

I was hoping I could get a similar testcase as bug 735114, but am unable to, thus far.

Comment 4

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

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

Comment 5

[Martijn Wargers (dead)]

I could sort of reproduce it with this:
http://people.mozilla.org/~mwargers/tests/dom/overflow_orientationchange_parent.html
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.

Comment 6

[Scoobidiver (away)]

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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

It looks like the crash here is happening on:
http://hg.mozilla.org/mozilla-central/annotate/1158503601be/xpcom/glue/nsTArray.h#l224
http://hg.mozilla.org/mozilla-central/annotate/1158503601be/xpcom/glue/Observer.h#l97
http://hg.mozilla.org/mozilla-central/annotate/1158503601be/hal/Hal.cpp#l211
http://hg.mozilla.org/mozilla-central/annotate/1158503601be/hal/Hal.cpp#l580

It looks like the observers array is bad and when we call Length() on it sadness.

Comment 8

[Chris Peterson [:cpeterson]]

This crash has jumped to #1 for 15.0a1.

Comment 9

[Scoobidiver (away)]

(In reply to Chris Peterson (:cpeterson) from comment #8)
> This crash has jumped to #1 for 15.0a1.
Because one user found STR.

Comment 10

[Martijn Wargers (dead)]

Yeah, that user would be me. I guess topcrash data is easy to influence.

Comment 11

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

(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 :)

Comment 12

[Robert Kaiser]

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

Comment 13

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

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

Comment 14

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

Attachment: Patch to force the error

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.

[1] http://hg.mozilla.org/mozilla-central/annotate/1158503601be/hal/Hal.cpp#l181
[2] http://hg.mozilla.org/mozilla-central/annotate/1158503601be/hal/Hal.cpp#l209

Comment 15

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

Attachment: Patch

Comment 16

[Chris Peterson [:cpeterson]]

Comment on attachment 626781 [details] [diff] [review]
Patch

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 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 626781 [details] [diff] [review]
Patch

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.

Comment 18

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

Landed with the assert removed and a comment added.

https://hg.mozilla.org/integration/mozilla-inbound/rev/56ad2fee6962

Comment 19

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/56ad2fee6962

Comment 20

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

Comment on attachment 626781 [details] [diff] [review]
Patch

[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

Comment 21

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b330f11e780

Comment 22

[Catalin Suciu [:csuciu]]

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)