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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aaron Train [:aaronmt]]

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

Comment 1

[(inactive) Jim Chen [:jchen] [:darchons]]

This is one of the top ANRs

Comment 2

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

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.

Comment 3

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

Attachment: WIP

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.

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 5

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

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.

Comment 6

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add UiThreadBlockedException and utility method to dump all stack traces (v1)

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.

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Throw an exception after 5 seconds timeout when returning PromptService result (v1)

For data-gathering only and to be backed out later

Comment 8

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

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

Comment 9

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b2771b6ab366
https://hg.mozilla.org/integration/mozilla-inbound/rev/78e3b52f963e

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/b2771b6ab366
https://hg.mozilla.org/mozilla-central/rev/78e3b52f963e

Comment 11

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Make file picker use public PromptService.getResponse method (v1)

Getting rid of a bit of redundant code in file picker as part of refactoring.

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Refactor PromptService (v1)

This is just a simple refactoring before the major changes.

Comment 13

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Make GeckoAppShell.processNextNativeEvent waitable (v1)

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.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Make PromptService not block when handing result from UI to Gecko thread (v1)

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.

Comment 15

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Make file picker not block when handing result from UI to Gecko thread (v1)

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

Comment 16

[Wesley Johnston (:wesj)]

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

Comment 17

[Wesley Johnston (:wesj)]

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

Comment 18

[Wesley Johnston (:wesj)]

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

Comment 19

[Brad Lassey [:blassey] (use needinfo?)]

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?

Comment 20

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Make file picker use public PromptService.getResponse method (v2)

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

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/397da5eb63b2
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c189f66a528
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b3b08aeba3d
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e1981be9749
https://hg.mozilla.org/integration/mozilla-inbound/rev/c66ae62f9cb1

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/397da5eb63b2
https://hg.mozilla.org/mozilla-central/rev/3c189f66a528
https://hg.mozilla.org/mozilla-central/rev/4b3b08aeba3d
https://hg.mozilla.org/mozilla-central/rev/2e1981be9749
https://hg.mozilla.org/mozilla-central/rev/c66ae62f9cb1

Comment 23

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

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.