1063120 - HelperApps.jsm can get stuck in the event loop

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

HelperApps.jsm includes a synchronous call [1] that sends a message to Java and waits for the response by spinning the event loop. However, spinning the event loop this way is not very reliable, as we can end up waiting for more events even after our event has run. The result is that the call ends up taking much longer than needed, up to several hundred milliseconds.

[1] http://mxr.mozilla.org/mozilla-central/source/mobile/android/modules/HelperApps.jsm#207

Comment 1

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

Attachment: Add synchronous method to notify Gecko observer (v1)

Add a JNI method that notifys Gecko observers on the main thread, outside of the event loop.

Comment 2

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

Attachment: Synchronously respond to Gecko requests if running on Gecko thread (v1)

If we are on the Gecko thread already and we are responding to JS, we should repond synchronously. This avoids the need to spinning the event loop in some cases, which doesn't work reliably.

Comment 3

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 8486640 [details] [diff] [review]
Add synchronous method to notify Gecko observer (v1)

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

looks good with nits

::: mobile/android/base/GeckoAppShell.java
@@ +448,5 @@
>      // Tell the Gecko event loop that an event is available.
>      public static native void notifyGeckoOfEvent(GeckoEvent event);
>  
> +    // Synchronously notify a Gecko observer; must be called from Gecko thread.
> +    public static native void notifyGeckoObserver(String subject, String data);

Can we call it notifyObservers? Seems redundant to put Gecko in there. And the nsIObserver method is pluralized.

Comment 4

[Brian Nicholson (:bnicholson)]

Comment on attachment 8486645 [details] [diff] [review]
Synchronously respond to Gecko requests if running on Gecko thread (v1)

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

I guess we need to be careful to avoid situations where
1) Gecko issues a request,
2) the Java-side listener fires a broadcast message inside the request handler,
3) the Gecko-side request callback assumes the message from #2 has been handled.

Comment 5

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/19598e50a751
https://hg.mozilla.org/integration/mozilla-inbound/rev/986e8a9f6195

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> Comment on attachment 8486640 [details] [diff] [review]
> Add synchronous method to notify Gecko observer (v1)
> 
> Can we call it notifyObservers? Seems redundant to put Gecko in there. And
> the nsIObserver method is pluralized.

I kept "Gecko" to remind people that it's for the Gecko thread only.

(In reply to Brian Nicholson (:bnicholson) from comment #4)
> Comment on attachment 8486645 [details] [diff] [review]
> Synchronously respond to Gecko requests if running on Gecko thread (v1)
> 
> Review of attachment 8486645 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I guess we need to be careful to avoid situations where
> 1) Gecko issues a request,
> 2) the Java-side listener fires a broadcast message inside the request
> handler,
> 3) the Gecko-side request callback assumes the message from #2 has been
> handled.

True. I don't think we want this behavior in any case though; we should combine the two broadcasts in one.

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/19598e50a751
https://hg.mozilla.org/mozilla-central/rev/986e8a9f6195