Closed Bug 763166 Opened 12 years ago Closed 12 years ago

crash in mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport

Categories

(Core Graveyard :: Widget: Android, defect)

Product:
Core Graveyard
Component:
Widget: Android
Version:
15 Branch
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

(firefox15+ fixed, firefox16+ verified, firefox17 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla17
Tracking Flags:
Tracking Status
firefox15 + fixed
firefox16 + verified
firefox17 --- fixed

People

(Reporter: scoobidiver, Assigned: kats)

References

Details

(4 keywords, Whiteboard: [native-crash][startupcrash])

Crash Data

Attachments

(2 files, 1 obsolete file)

patch to actually use tls
1.73 KB, patch
kats
: review+
kats
: checkin+
Details | Diff | Splinter Review
Remove race condition from AndroidBridge initialization
2.54 KB, patch
snorp
: review+
kats
: checkin+
Details | Diff | Splinter Review 
There are only two crashes in 14.0b5 but there's a spike in startup crashes beginning from 16.0a1/20120606. The regression range for the spike is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a7a905fd70d5&tochange=6338a8988917
It's likely a regression from bug 758361 which was uplifted to Aurora.

Signature 	_JNIEnv::CallVoidMethod | mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport More Reports Search
UUID	4a54c3c6-09ec-4376-b96b-87aa02120608
Date Processed	2012-06-08 21:40:28
Uptime	28
Last Crash	3.7 weeks before submission
Install Age	9.2 minutes since version was first installed.
Install Time	2012-06-08 21:31:13
Product	FennecAndroid
Version	16.0a1
Build ID	20120608030525
Release Channel	nightly
OS	Linux
OS Version	0.0.0 Linux 3.0.8+ #9 SMP PREEMPT Wed May 2 18:19:29 CEST 2012 armv7l
Build Architecture	arm
Build Architecture Info	
Crash Reason	SIGSEGV
Crash Address	0x0
App Notes 	
AdapterVendorID: archos, AdapterDeviceID: ARCHOS 80G9.
AdapterDescription: 'Model: 'ARCHOS 80G9', Product: 'G9A80', Manufacturer: 'archos', Hardware: 'archos''.
archos ARCHOS 80G9
archos/G9A80/A80:4.0.3/MR1/20120420.190002:user/release-keys
EMCheckCompatibility	True

Frame 	Module 	Signature 	Source
0 	libdvm.so 	libdvm.so@0x57a70 	
1 	libdvm.so 	libdvm.so@0x57a4f 	
2 	libxul.so 	_JNIEnv::CallVoidMethod 	jni.h:631
3 	libxul.so 	mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport 	widget/android/AndroidJavaWrappers.cpp:664
4 	libxul.so 	mozilla::AndroidBridge::SetFirstPaintViewport 	widget/android/AndroidBridge.cpp:2047
5 	libxul.so 	mozilla::layers::CompositorParent::SetFirstPaintViewport 	gfx/layers/ipc/CompositorParent.cpp:429
6 	libxul.so 	mozilla::layers::CompositorParent::TransformShadowTree 	gfx/layers/ipc/CompositorParent.cpp:374
7 	libxul.so 	mozilla::layers::CompositorParent::Composite 	gfx/layers/ipc/CompositorParent.cpp:260
8 	libxul.so 	RunnableMethod<mozilla::layers::CompositorParent, void , Tuple0>::Run 	ipc/chromium/src/base/tuple.h:383
9 	libxul.so 	MessageLoop::RunTask 	ipc/chromium/src/base/message_loop.cc:318
10 	libxul.so 	MessageLoop::DeferOrRunPendingTask 	ipc/chromium/src/base/message_loop.cc:326
11 	libxul.so 	MessageLoop::DoWork 	ipc/chromium/src/base/message_loop.cc:426
12 	libxul.so 	base::MessagePumpDefault::Run 	ipc/chromium/src/base/message_pump_default.cc:23
13 	libxul.so 	MessageLoop::RunInternal 	ipc/chromium/src/base/message_loop.cc:208
14 	libxul.so 	MessageLoop::Run 	ipc/chromium/src/base/message_loop.cc:201
15 	libxul.so 	base::Thread::ThreadMain 	ipc/chromium/src/base/thread.cc:156
16 	libxul.so 	ThreadFunc 	ipc/chromium/src/base/platform_thread_posix.cc:31
17 	libc.so 	libc.so@0x12c4e 	
18 	libc.so 	libc.so@0x127a2 	
19 	libEGL.so 	libEGL.so@0x23e82 

More reports at:
https://crash-stats.mozilla.com/report/list?signature=_JNIEnv%3A%3ACallVoidMethod+|+mozilla%3A%3AAndroidGeckoLayerClient%3A%3ASetFirstPaintViewport
https://crash-stats.mozilla.com/report/list?signature=_JNIEnv%3A%3ACallVoidMethod
https://crash-stats.mozilla.com/report/list?signature=JNI_CreateJavaVM+|+_JNIEnv%3A%3ACallVoidMethod+|+mozilla%3A%3AAndroidGeckoLayerClient%3A%3ASetFirstPaintViewport
As 15.0a2 is unaffected, it's not caused by bug 758361.
No longer blocks: 758361
Version: 15 Branch → 16 Branch
Given that this crash seems like a race condition on startup, I would guess that this change was the one that caused the spike in 16.0:

d945ae198516	Mike Hommey — Bug 760152 - Start library decompression earlier. r=blassey

Just a guess, though.
Setting as top crash : 6th on nightly, does not seem to appear in aurora, or beta
status-firefox14: --- → unaffected
status-firefox15: --- → unaffected
status-firefox16: --- → affected
Keywords: topcrash
CC'ing :glandium. Does bug 760152 only do library decompression earlier? Does it do any other gecko startup earlier?
(In reply to Kartikaya Gupta (:kats) from comment #4)
> CC'ing :glandium. Does bug 760152 only do library decompression earlier?
> Does it do any other gecko startup earlier?

Gecko's actual initialization has not moved. Only decompression did.
https://tbpl.mozilla.org/php/getParsedLog.php?id=12818266&tree=Mozilla-Inbound
Whiteboard: [native-crash][startupcrash] → [native-crash][startupcrash][orange]
Blocks: 438871
Crash Signature: [@ _JNIEnv::CallVoidMethod | mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport] [@ _JNIEnv::CallVoidMethod] [@ JNI_CreateJavaVM | _JNIEnv::CallVoidMethod | mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport] → [@ _JNIEnv::CallVoidMethod | mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport ] [@ _JNIEnv::CallVoidMethod ] [@ JNI_CreateJavaVM | _JNIEnv::CallVoidMethod | mozilla::AndroidGeckoLayerClient::SetFirstPaintViewport ]
tracking-fennec: --- → ?
Assignee: nobody → bugmail.mozilla
This should be sufficient to fix this crash, assuming the problem is in fact the GetJNIForThread call. I can kill the GetJNIForThread() more in a separate bug.
Attachment #641598 - Flags: review?(blassey.bugs)
Comment on attachment 641598 [details] [diff] [review]
Add a GetJNIForCompositorThread and make the compositor JNI invocations use it

Review of attachment 641598 [details] [diff] [review]:
-----------------------------------------------------------------

::: widget/android/AndroidBridge.h
@@ +132,5 @@
> +    static JNIEnv* GetJNIForCompositorThread() {
> +        if (NS_LIKELY(sBridge)) {
> +            if (sBridge->mCompositorThread) {
> +                if ((void*)pthread_self() != sBridge->mCompositorThread) {
> +                    __android_log_print(ANDROID_LOG_ERROR, "AndroidBridge", "Non-compositor thread calling GetJNIForCompositorThread!");

this should probably be a NS_ABORT_IF_FALSE()

@@ +145,5 @@
> +            MutexAutoLock lock(sBridge->mCompositorJNICreationMutex);
> +
> +            if (sBridge->mCompositorThread) {
> +                // this means that another thread executed this function between the time we started executing
> +                // it and the time we acquired the mutex. fail.

this also means we have two threads that claim to be the compositor thread, which sounds like a disaster. Let's abort here too
Attachment #641598 - Flags: review?(blassey.bugs) → review+
Backed out on suspicion of making all the android talos tests go red. Nothing useful in the log though.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3215d2dc286
(In reply to comment #27)
> Backed out on suspicion of making all the android talos tests go red. Nothing
> useful in the log though.
> 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/f3215d2dc286

The backout seems to have fixed it.
Assignee: bugmail.mozilla → blassey.bugs

        Attachment #641598 -
        Attachment is obsolete: true

        Attachment #642940 -
        Flags: review?(bugmail.mozilla)

    
    

    
  
Comment on attachment 642940 [details] [diff] [review]
patch to actually use tls

Review of attachment 642940 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with changing the if (status < 0) to if (status) as well.

        Attachment #642940 -
        Flags: review?(bugmail.mozilla) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/2ee313f65ccf
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED

    
    

    
  
14.0.1 and 15.0 Beta are affected.

          status-firefox14:
          unaffected → affected

          status-firefox15:
          unaffected → affected

          tracking-firefox15:
          --- → ?

          tracking-firefox16:
          --- → ?
Version: 16 Branch → 15 Branch

    
    

    
  
Let's restrict this bug to top crashers.
It's #3 top crasher in 15.0b2. The Beta regression range is:
http://hg.mozilla.org/releases/mozilla-beta/pushloghtml?fromchange=695042299ec9&tochange=166ba24e4239
Bug 760152 is the only bug that belongs to the two regression windows.
Blocks: 760152

          status-firefox14:
          affected → ---

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #5)
> (In reply to Kartikaya Gupta (:kats) from comment #4)
> > CC'ing :glandium. Does bug 760152 only do library decompression earlier?
> > Does it do any other gecko startup earlier?
> 
> Gecko's actual initialization has not moved. Only decompression did.

actually, one thing moved: static initialization. Is there any static initialization that calls back into java?

    
  
Blocks: 778954

    
    

    
  
(In reply to Mike Hommey [:glandium] from comment #37)
> actually, one thing moved: static initialization. Is there any static
> initialization that calls back into java?

I'm not aware of any offhand, but it's quite possible that there is. Looking at the code again, I found something that might be the problem here. Patch coming in a sec.

    
    

    
  
I think I'd rather we just backed out bug 760152 on branches at this point, since it didn't have any obvious perf gains. Is that riskier?

    
    

    
  
Doing the backout is probably the better option. I don't know if my patch will actually fix this or not yet. If it does we can re-land it with the fix.

    
    

    
  
(In reply to Kartikaya Gupta (:kats) from comment #43)
> Doing the backout is probably the better option. I don't know if my patch
> will actually fix this or not yet. If it does we can re-land it with the fix.

Thanks, sending over to glandium in that case.
Assignee: blassey.bugs → mh+mozilla

    
    

    
  
Comment on attachment 647566 [details] [diff] [review]
Remove race condition from AndroidBridge initialization

New try run is at https://tbpl.mozilla.org/?tree=Try&rev=e3e0e2f7ec4f (old one failed to build because i used a busted inbound changeset as a base). It looks about as green as android usually does, so requesting review.

        Attachment #647566 -
        Flags: review?(snorp)

        Attachment #647566 -
        Flags: review?(snorp) → review+

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/87736e458d15
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED

    
    

    
  
There are still crashes in the trunk.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
(In reply to Scoobidiver from comment #49)
> There are still crashes in the trunk.

Do you have evidence of that? There are no crashes I can see with a build id from a build after the landing:
https://crash-stats.mozilla.com/report/list?range_value=7&range_unit=days&date=2012-08-02&signature=_JNIEnv%3A%3ACallVoidMethod%20|%20mozilla%3A%3AAndroidGeckoLayerClient%3A%3ASetFirstPaintViewport&version=FennecAndroid%3A17.0a1

    
    

    
  
There are no crashes after 16.0a2/20120801, i.e. the backout of bug 760152.

          status-firefox16:
          fixed → verified

    
    

    
  
https://crash-stats.mozilla.com/report/index/1fd05d7b-0f24-4063-9a84-ee7782120806
https://crash-stats.mozilla.com/report/index/e4673839-1a8f-4f07-ab22-956902120806

Note to clarify.. that's only 3 report that shows on/after 8/1 out of 2,838+634+1 Crash Reports.  Only 1 report after 8/1.

Should we close this out?  Or keep this and mark as a lower priority?  People just need to update it seems.

    
    

    
  
(In reply to Naoki Hirata :nhirata from comment #54)
> Should we close this out?  Or keep this and mark as a lower priority? 
No. When 17 Branch will go to Aurora, it will be #3 top crasher in 17.0a2 like it was in 16.0a2.

    
    

    
  
Bug 760152 was backed out of Beta 15 yesterday.

    
    

    
  
(In reply to Naoki Hirata :nhirata from comment #54)
> Should we close this out?  Or keep this and mark as a lower priority? 
> People just need to update it seems.

No. This is still occurring on trunk.

    
    

    
  
Assigning to kats after speaking to glandium.
Assignee: mh+mozilla → bugmail.mozilla

    
    

    
  
It looks like all of the recent logs are from mochitest runs -- why would that be?

    
    

    
  
Kats, do you have any ideas for this?

    
    

    
  
Unfortunately, no. I've looked through the code multiple times and don't see any other problems that could cause this.

    
    

    
  
(In reply to Ed Morley [:edmorley] from comment #101)
> Kats, do you have any ideas for this?

Actually, we should back out bug 760152 on 17 as well. Backing that out seems to have fixed the error completely on 15 and 16, so it should help significantly on 17. Note that this error was happening even before bug 760152 landed (albeit at a very low crash volume) so I don't expect it to disappear completely, unless one of the other patches on this bug did actually fix something. (i.e. bug 760152 might have masked the original problem, which was fixed by one of the patches on this bug).

I posted a comment on bug 760152 requesting it be backed out on 17 as well.

    
    

    
  
Thank you :-)

    
    

    
  
I pushed the backout of bug 760152 to inbound so hopefully this should stop happening as that propagates to the various trees. *fingers crossed*

    
    

    
  
This has stopped after the build from the 16th, so the backout worked. Should we mark this bug fixed?

    
    

    
  
Sounds good to me.
Status: REOPENED → RESOLVED
Closed: 12 years ago → 12 years ago
Resolution: --- → FIXED
Whiteboard: [native-crash][startupcrash][orange] → [native-crash][startupcrash]
tracking-fennec: ? → ---

    
  
Product: Core → Core Graveyard

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: