1474618 - Change GeckoResult dispatch behavior to allow for synchronous ops on UI thread

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

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

Right now Focus calls saveState() from a background thread. It then waits for that to complete on the UI thread with a CountDownLatch. With the change to GeckoResult that isn't possible anymore because GeckoResult always dispatches listeners on the UI thread -- which is blocked waiting on the CountDownLatch.

The easiest solution is to change GeckoResult to dispatch on the thread where the result was created. I think this also may be closer to what app developers expect.

Comment 1

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

Attachment: Bug 1474618 - Dispatch GeckoResult listeners on thread where we were created

Review commit: https://reviewboard.mozilla.org/r/256026/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/256026/

Comment 2

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

Comment on attachment 8991035 [details]
Bug 1474618 - Dispatch GeckoResult listeners on thread where we were created

https://reviewboard.mozilla.org/r/256026/#review262870

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:32
(Diff revision 1)
>      /**
>       * This constructs an incomplete GeckoResult. Call {@link #complete(Object)} or
>       * {@link #completeExceptionally(Throwable)} in order to fulfill the result.
>       */
>      public GeckoResult() {
> +        mHandler = new Handler();

I think we may want to cache these in a static map at some point, but we're not creating GeckoResults at a rate where it will matter yet.

Comment 3

[Dylan Roeh (:droeh) (he/him)]

Comment on attachment 8991035 [details]
Bug 1474618 - Dispatch GeckoResult listeners on thread where we were created

https://reviewboard.mozilla.org/r/256026/#review262924

LGTM

Comment 4

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

Comment on attachment 8991035 [details]
Bug 1474618 - Dispatch GeckoResult listeners on thread where we were created

https://reviewboard.mozilla.org/r/256026/#review263426

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:32
(Diff revision 1)
>      /**
>       * This constructs an incomplete GeckoResult. Call {@link #complete(Object)} or
>       * {@link #completeExceptionally(Throwable)} in order to fulfill the result.
>       */
>      public GeckoResult() {
> +        mHandler = new Handler();

I think we should at least check the most common case of the UI thread and use `ThreadUtils.sUiThread` in that case.

::: mobile/android/geckoview/src/main/java/org/mozilla/geckoview/GeckoResult.java:32
(Diff revision 1)
>      /**
>       * This constructs an incomplete GeckoResult. Call {@link #complete(Object)} or
>       * {@link #completeExceptionally(Throwable)} in order to fulfill the result.
>       */
>      public GeckoResult() {
> +        mHandler = new Handler();

Now that callbacks are no longer guaranteed on the UI thread, we should assert in `GeckoSession`'s `then` callbacks that we're on the UI thread because those callbacks still expect the UI thread.

Comment 5

[Chris Peterson [:cpeterson]]

[geckoview:klar:p1] because this is a Focus+GV blocker.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/58c3113b9020
Dispatch GeckoResult listeners on thread where we were created r=jchen,droeh

Comment 7

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/58c3113b9020

Comment 8

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

Comment on attachment 8991035 [details]
Bug 1474618 - Dispatch GeckoResult listeners on thread where we were created

Approval Request Comment
[Feature/Bug causing the regression]: GeckoView
[User impact if declined]: Unable to save state in Focus
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: Only affects GeckoView
[String changes made/needed]: None

Comment 9

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8991035 [details]
Bug 1474618 - Dispatch GeckoResult listeners on thread where we were created

Geckoview only change, let's uplift to beta.

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/25ffc90ec574