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

RESOLVED FIXED in Firefox 56

Status

enhancement
RESOLVED FIXED
2 years ago
7 months ago

People

(Reporter: droeh, Assigned: droeh)

Tracking

53 Branch
mozilla56
All
Android

Firefox Tracking Flags

(firefox56 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch tabs-dispatcher.patch (obsolete) — Splinter Review
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.
Attachment #8868557 - Flags: review?(nchen)
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
Attachment #8868557 - Flags: review?(nchen) → feedback+
Updated to address feedback.
Attachment #8868557 - Attachment is obsolete: true
Comment on attachment 8869032 [details] [diff] [review]
tabs-dispatcher.patch

Forgot to flag for review.
Attachment #8869032 - Flags: review?(nchen)
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.
Attachment #8869032 - Flags: review?(nchen) → review+
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b94150e0a299
Make Tabs use the window event dispatcher rather than global in some instances. r=jchen
https://hg.mozilla.org/mozilla-central/rev/b94150e0a299
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Reopening; we backed this out in bug 1369393 and will re-land in 56.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Pushed by droeh@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d1994e5846c
Make Tabs use the window event dispatcher rather than global in some instances. r=jchen
https://hg.mozilla.org/mozilla-central/rev/7d1994e5846c
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: Firefox 55 → Firefox 56
Product: Firefox for Android → GeckoView
Target Milestone: Firefox 56 → mozilla56
You need to log in before you can comment on or make changes to this bug.