Closed Bug 860879 Opened 7 years ago Closed 7 years ago

ANR @ org.mozilla.gecko.PromptService.finishDialog(PromptService.java:470)

Categories

(Firefox for Android :: General, defect)

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 23

People

(Reporter: aaronmt, Assigned: jchen)

References

Details

(Keywords: hang, Whiteboard: [ANR])

Attachments

(9 files, 1 obsolete file)

159.30 KB, text/plain
Details
WIP
4.28 KB, patch
Details | Diff | Splinter Review
2.18 KB, patch
kats
: review+
Details | Diff | Splinter Review
1.19 KB, patch
kats
: review+
Details | Diff | Splinter Review
6.76 KB, patch
wesj
: review+
Details | Diff | Splinter Review
5.61 KB, patch
blassey
: review+
Details | Diff | Splinter Review
4.51 KB, patch
wesj
: review+
Details | Diff | Splinter Review
10.48 KB, patch
wesj
: review+
Details | Diff | Splinter Review
3.33 KB, patch
wesj
: review+
Details | Diff | Splinter Review
Attached file traces.txt
main" prio=5 tid=1 WAIT
  | group="main" sCount=1 dsCount=0 obj=0x4105d9a0 self=0x4009b010
  | sysTid=2718 nice=0 sched=0/0 cgrp=apps handle=1075106780
  | state=S schedstat=( 0 0 0 ) utm=336 stm=58 core=0
  at java.lang.Object.wait(Native Method)
  - waiting on <0x4105dda0> (a java.lang.VMThread) held by tid=1 (main)
  at java.lang.Thread.parkFor(Thread.java:1231)
  at sun.misc.Unsafe.park(Unsafe.java:323)
  at java.util.concurrent.locks.LockSupport.park(LockSupport.java:159)
  at java.util.concurrent.SynchronousQueue$TransferStack.awaitFulfill(SynchronousQueue.java:427)
  at java.util.concurrent.SynchronousQueue$TransferStack.transfer(SynchronousQueue.java:329)
  at java.util.concurrent.SynchronousQueue.put(SynchronousQueue.java:847)
  at org.mozilla.gecko.PromptService.finishDialog(PromptService.java:470)
  at org.mozilla.gecko.PromptService.onCancel(PromptService.java:451)

I hit this a few days ago, just attaching traces.txt now

--
LG Nexus 4 (Android 4.2.2)
Nightly (04/09)
This is one of the top ANRs
We need to know what the gecko thread is doing in native code. What I would like is for some of these ANRs to actually crash using MOZ_CRASH so that we get some stack info on what the gecko thread is doing. Maybe crash all ANRs for a couple of builds on Nightly and see what kind of data we get.
Attached patch WIPSplinter Review
This is the patch I used to determine that disk cache on the gecko thread (bug 852467) was the problem in bug 834243. I think if we hook this up the ANR handling code it might give us some useful info.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #2)
> We need to know what the gecko thread is doing in native code. What I would
> like is for some of these ANRs to actually crash using MOZ_CRASH so that we
> get some stack info on what the gecko thread is doing. Maybe crash all ANRs
> for a couple of builds on Nightly and see what kind of data we get.

Can we print out the profiler stack? That seems better than having to crash.
We can print the pseudo-stack, and that might be good enough to start with, but I think eventually we're going to need a full stack to actually fix the issues.
Depends on: 863777
Whiteboard: [ANR]
Since we've needed something like this a couple of times already, I'm wondering if we should just keep these around. But we can decide that when it's time to back out.
Attachment #744203 - Flags: review?(bugmail.mozilla)
For data-gathering only and to be backed out later
Attachment #744204 - Flags: review?(bugmail.mozilla)
Comment on attachment 744203 [details] [diff] [review]
Add UiThreadBlockedException and utility method to dump all stack traces (v1)

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

LGTM
Attachment #744203 - Flags: review?(bugmail.mozilla) → review+
Attachment #744204 - Flags: review?(bugmail.mozilla) → review+
Whiteboard: [ANR] → [ANR][leave open]
Depends on: 868196
Getting rid of a bit of redundant code in file picker as part of refactoring.
Attachment #746532 - Flags: review?(wjohnston)
This is just a simple refactoring before the major changes.
Attachment #746535 - Flags: review?(wjohnston)
The next two patches will switch from using SynchronousQueue to ConcurrentLinkedQueue, and as part of that change we also need to change the processNextNativeEvent calls from not-wait to wait.
Attachment #746541 - Flags: review?(wjohnston)
We were prone to locking up the UI thread when using SynchronousQueue, so we switch to using ConcurrentLinkedQueue here. This way, the UI thread never blocks. On the Gecko thread, we wait and process any Gecko events, and after every event check for any pending response from the queue. In order to wake up the Gecko thread in case it's waiting for an event, we also send a no-op event to Gecko when we put reponses in the queue.
Attachment #746544 - Flags: review?(wjohnston)
Although the file picker SynchronousQueue usage doesn't show up on ANR reports, I think it's still worth it to make it consistent with the PromptService implementation
Attachment #746545 - Flags: review?(wjohnston)
Attachment #746535 - Flags: review?(wjohnston) → review+
Comment on attachment 746541 [details] [diff] [review]
Make GeckoAppShell.processNextNativeEvent waitable (v1)

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

Brad of kats should probably look at this (and kats is PTO).
Attachment #746541 - Flags: review?(wjohnston) → review?(blassey.bugs)
Comment on attachment 746544 [details] [diff] [review]
Make PromptService not block when handing result from UI to Gecko thread (v1)

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

::: mobile/android/base/PromptService.java
@@ +457,5 @@
>          mDialog = null;
>          mSelected = null;
> +        mPromptQueue.offer(aReturn);
> +        // poke the Gecko thread in case it's waiting for new events
> +        GeckoAppShell.sendEventToGecko(GeckoEvent.createNoOpEvent());

Ahh. I wondered what was up with this (it busted my tests because I didn't care about the return value there...)
Attachment #746544 - Flags: review?(wjohnston) → review+
Attachment #746545 - Flags: review?(wjohnston) → review+
Comment on attachment 746532 [details] [diff] [review]
Make file picker use public PromptService.getResponse method (v1)

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

I assume this fails as is, or maybe I'm missing something? Just want to make sure its tested.

::: mobile/android/base/ActivityHandlerHelper.java
@@ +151,5 @@
>              return intents.get(0);
>          }
>  
> +        final PromptService.PromptListItem[] items =
> +            getItemsAndIntentsForFilePicker(context, aMimeType, intents);

I don't think you can move this... i.e. intents.size() above will always be zero.

@@ +158,5 @@
> +        // Runnable has to be called to show an intent-like
> +        // context menu UI using the PromptService.
> +        ThreadUtils.postToUiThread(new Runnable() {
> +            @Override public void run() {
> +                GeckoApp.mAppContext.getPromptService().show(title, "", items, false);

We should probably just store a single reference to the promptService here.

final PromptService ps = GeckoApp....
Attachment #746532 - Flags: review?(wjohnston) → review-
Comment on attachment 746541 [details] [diff] [review]
Make GeckoAppShell.processNextNativeEvent waitable (v1)

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

::: widget/android/AndroidJNI.cpp
@@ +72,4 @@
>  {
>      // poke the appshell
>      if (nsAppShell::gAppShell)
> +        nsAppShell::gAppShell->ProcessNextNativeEvent(mayWait != JNI_FALSE);

can't you just cast this to bool?
Attachment #746541 - Flags: review?(blassey.bugs) → review+
(In reply to Wesley Johnston (:wesj) from comment #18)
> Comment on attachment 746532 [details] [diff] [review]
> Make file picker use public PromptService.getResponse method (v1)
> 
> Review of attachment 746532 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I assume this fails as is, or maybe I'm missing something? Just want to make
> sure its tested.
> 
> ::: mobile/android/base/ActivityHandlerHelper.java
> @@ +151,5 @@
> >              return intents.get(0);
> >          }
> >  
> > +        final PromptService.PromptListItem[] items =
> > +            getItemsAndIntentsForFilePicker(context, aMimeType, intents);
> 
> I don't think you can move this... i.e. intents.size() above will always be
> zero.

Good catch! Fixed and tested

> @@ +158,5 @@
> > +        // Runnable has to be called to show an intent-like
> > +        // context menu UI using the PromptService.
> > +        ThreadUtils.postToUiThread(new Runnable() {
> > +            @Override public void run() {
> > +                GeckoApp.mAppContext.getPromptService().show(title, "", items, false);
> 
> We should probably just store a single reference to the promptService here.
> 
> final PromptService ps = GeckoApp....

Done.
Attachment #746532 - Attachment is obsolete: true
Attachment #747491 - Flags: review?(wjohnston)
Attachment #747491 - Flags: review?(wjohnston) → review+
FWIW I also took a quick look at the overall shape of the changes and I see no problems with it. Nice work! :) I really like that we're not polling every 1ms now.
You need to log in before you can comment on or make changes to this bug.