1365599 - Make Tabs use the window event dispatcher rather than global when dispatching messages

Status: RESOLVED FIXED

(GeckoView :: General, enhancement)

Description

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

We run into a race condition when trying to use GeckoView for things other than Fennec: if we run (eg) a GV-based custom tab before Fennec is launched, and then launch Fennec, we see "dead tabs" in Fennec because Tabs sends "Tab:Load" and "Tab:Selected" messages before Fennec has registered listeners for them in browser.js. Under the assumption that there is only one GeckoApp, we can fix this by sending those messages on the window event dispatcher.

Comment 1

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

Attachment: tabs-dispatcher.patch

As mentioned in the summary, this switches Tabs over to using the window event dispatcher for some messages. This will likely break GeckoApp-based custom tabs and PWAs, so I'm looking to land simultaneously with GV-based custom tabs and PWAs.

Comment 2

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

Comment on attachment 8868557 [details] [diff] [review]
tabs-dispatcher.patch

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

I see an existing bug that this patch should fix. When `Tabs.attachToContext()` is called again, we actually return early because the application context is the same. We should stop doing that, so that we can set a new EventDispatcher/LayerView.

Also, we should add something like `detachFromContext` to free the `mLayerView` instance that's kept around even after GeckoApp is destroyed.

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
@@ +107,5 @@
>      private static final AtomicInteger sTabId = new AtomicInteger(0);
>      private volatile boolean mInitialTabsAdded;
>  
>      private Context mAppContext;
> +    private GeckoApp mGeckoApp;

Only keep the EventDispatcher instance instead of the whole GeckoApp

@@ +184,5 @@
>              Log.w(LOGTAG, "The application context has changed!");
>          }
>  
>          mAppContext = appContext;
> +        mGeckoApp = (GeckoApp) context;

Pass in EventDispatcher as an argument

Comment 3

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

Attachment: tabs-dispatcher.patch

Updated to address feedback.

Comment 4

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

Comment on attachment 8869032 [details] [diff] [review]
tabs-dispatcher.patch

Forgot to flag for review.

Comment 5

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

Comment on attachment 8869032 [details] [diff] [review]
tabs-dispatcher.patch

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

::: mobile/android/base/java/org/mozilla/gecko/Tabs.java
@@ +178,2 @@
>  
> +        if (mAppContext != null && mAppContext != appContext) {

Just remove this block.

@@ +205,5 @@
>      }
>  
> +    public void detachFromContext() {
> +        mLayerView = null;
> +        mEventDispatcher = null;

We shouldn't clear `mEventDispatcher` here because I think it's still possible for the tab events to be dispatched after detaching.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94150e0a299
Make Tabs use the window event dispatcher rather than global in some instances. r=jchen

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b94150e0a299

Comment 8

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

Reopening; we backed this out in bug 1369393 and will re-land in 56.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1994e5846c
Make Tabs use the window event dispatcher rather than global in some instances. r=jchen

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/7d1994e5846c