1294821 - Snackbars are broken (potentially leading to a crash) after a custom tab activity is started

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P2)

Description

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

Snackbars no longer appear and can potentially cause an uncaught exception crash after a custom tab is launched and Fennec is reopened.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

Is this related to having multiple event listeners registered?

Comment 2

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

Attachment: Desingletonize EventDispatcher

Yeah, having multiple listeners registered for many events was causing some problems. This patch de-singletonizes EventDispatcher and stores an instance of EventDispatcher in GeckoApp, but also leaves a "global" EventDispatcher in the EventDispatcher class (still accessible via EventDispatcher.getInstance) for cases where the event is not application specific (eg GeckoNetworkManager).

There may be some things using an app-specific EventDispatcher when they should be using the global one or vice-versa, I've just used my best judgement to take a first stab at determining which is which.

Comment 3

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

Comment on attachment 8787390 [details] [diff] [review]
Desingletonize EventDispatcher

This seems ok to me, but I think I would like Sebastian to review it. Does it pass Try?

Comment 4

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

(In reply to James Willcox (:snorp) ([e-mail]) from comment #3)
> Comment on attachment 8787390 [details] [diff] [review]
> Desingletonize EventDispatcher
> 
> This seems ok to me, but I think I would like Sebastian to review it. Does
> it pass Try?

I haven't done a try run yet, I've discovered a couple of problems this morning with this in conjunction with the patch for bug 1299925; mostly, finishing a CustomTabsActivity will call destroy on some singletons and result in their listeners being prematurely unregistered. I'll put up an updated patch when it can handle that properly.

Comment 5

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8787390 [details] [diff] [review]
Desingletonize EventDispatcher

Ok, reflag me for the updated patch (and maybe jchen, he definitely knows more about eventdispatcher and possible side effects).

Comment 6

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

Attachment: Desingletonize EventDispatcher

Not passing Try yet, getting some weird timeouts.

Comment 7

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

Comment on attachment 8790799 [details] [diff] [review]
Desingletonize EventDispatcher

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

The idea looks fine to me, but I think `GeckoAppShell.getGeckoInterface().getEventDispatcher()` is too verbose, and I'd rather have `GeckoApp.getEventDispatcher()` instead. You won't be able to use GeckoApp in geckoview code, but those changes you made in geckoview look like they should use the global EventDispatcher anyways.

Comment 8

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

Attachment: Desingletonize EventDispatcher

This is now passing Try and generally seems to be in good shape.

Jim, I went with adding a getEventDispatcher() function to GeckoAppShell and calling that in lieu of GeckoAppShell.getGeckoInterface().getEventDispatcher(); let me know if that's alright with you or if there's some reason aside from brevity that you'd like to call it on GeckoApp.

Comment 9

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

(In reply to Dylan Roeh (:droeh) from comment #8)
> Created attachment 8793463 [details] [diff] [review]
> Desingletonize EventDispatcher
> 
> This is now passing Try and generally seems to be in good shape.
> 
> Jim, I went with adding a getEventDispatcher() function to GeckoAppShell and
> calling that in lieu of
> GeckoAppShell.getGeckoInterface().getEventDispatcher(); let me know if
> that's alright with you or if there's some reason aside from brevity that
> you'd like to call it on GeckoApp.

Use GeckoApp, because the EventDispatcher is specific to GeckoApp, not GeckoAppShell.

Comment 10

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

To clarify, Fennec code should be using GeckoApp.getEventDispatcher(). GeckoView code can use getGeckoInterface().getEventDispatcher(), but it may be an indication that we are doing something we shouldn't be doing, because GeckoView code should not depend on the GeckoApp lifecycle.

Comment 11

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

Attachment: Desingletonize EventDispatcher

Alright, that makes sense. I have GeckoView using the global EventDispatcher now as well, too.

Comment 12

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

Comment on attachment 8793492 [details] [diff] [review]
Desingletonize EventDispatcher

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

::: mobile/android/base/java/org/mozilla/gecko/AccountsHelper.java
@@ +52,5 @@
>          mContext = context;
>          mProfile = profile;
>  
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        EventDispatcher dispatcher = geckoApp.getEventDispatcher();

You should be making `GeckoApp.getEventDispatcher()` static so you don't end up with a bunch of these two-liners. Rename `GeckoInterface.getEventDispatcher` to `getAppEventDispatcher` to accommodate that.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +1152,5 @@
>          // We need to set the notification client before launching Gecko, since Gecko could start
>          // sending notifications immediately after startup, which we don't want to lose/crash on.
>          GeckoAppShell.setNotificationClient(makeNotificationClient());
>  
> +        ((GeckoApplication) getApplication()).prepareLightweightTheme();

Why is this change necessary? `prepareLightweightTheme` call should stay in BrowserApp if possible.

::: mobile/android/base/java/org/mozilla/gecko/home/HomePanelsManager.java
@@ +83,5 @@
>          return sInstance;
>      }
>  
>      public void init(Context context) {
> +        if (!mInitialized) {

if (mInitialized) {
    return;
}

::: mobile/android/base/java/org/mozilla/gecko/javaaddons/JavaAddonManager.java
@@ +13,5 @@
>  import dalvik.system.DexClassLoader;
>  import org.json.JSONException;
>  import org.json.JSONObject;
>  import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.GeckoAppShell;

Unused?

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
@@ +376,2 @@
>  
> +        geckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Snackbar:Show");

Combine these two `registerGeckoThreadListener` calls.

@@ +511,5 @@
>  
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        geckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Sanitize:Finished");
> +
> +        geckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Snackbar:Show");

Combine these two `unregisterGeckoThreadListener` calls.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/SiteIdentityPopup.java
@@ +99,5 @@
>          mContentButtonClickListener = new ContentNotificationButtonListener();
> +
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        geckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Doorhanger:Logins");
> +        geckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Permissions:CheckResult");

Combine these two `registerGeckoThreadListener` calls.

@@ +550,5 @@
>  
>      void destroy() {
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        geckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Doorhanger:Logins");
> +        geckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Permissions:CheckResult");

Combine these two `unregisterGeckoThreadListener` calls.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/BaseGeckoInterface.java
@@ +25,5 @@
>      // Bug 908760: Implement GeckoEventResponder
>  
>      private final Context mContext;
>      private GeckoProfile mProfile;
> +    private EventDispatcher eventDispatcher;

final

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
@@ +219,5 @@
> +        if (this != INSTANCE) {
> +            final List<NativeEventListener> globalListeners = INSTANCE.getNativeListeners(type);
> +            if (listeners != null) {
> +                if (globalListeners != null) {
> +                    listeners.addAll(globalListeners);

Why are you modifying the list of app listeners??? `listeners` is _not_ a copy; you cannot modify it.

@@ +423,5 @@
> +            if (this != INSTANCE) {
> +                final List<GeckoEventListener> globalListeners = INSTANCE.getGeckoListeners(type);
> +                if (listeners != null) {
> +                    if (globalListeners != null) {
> +                        listeners.addAll(globalListeners);

Same thing. You cannot modify `listeners`.

I suggest you call `dispatchEvent` twice, once on the global object and once on the app object. Change `dispatchEvent` to allow you to check whether an event was handled or not.

@@ +440,5 @@
>                  }
>                  return;
>              }
> +
> +            if (listeners != null) {

Unnecessary if.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/notifications/NotificationHelper.java
@@ +81,5 @@
>          mContext = context;
>      }
>  
>      public void init() {
> +        if (!mInitialized) {

if (mInitialized) {
    return;
}

Also, NotificationHelper.java just moved out of geckoview so this needs rebasing.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testEventDispatcher.java
@@ +52,5 @@
>      public void setUp() throws Exception {
>          super.setUp();
>  
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        geckoApp.getEventDispatcher().registerGeckoThreadListener(

Does the global event dispatcher not work for the test? Why not?

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testTrackingProtection.java
@@ +50,5 @@
>          super.setUp();
>  
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        geckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Content:SecurityChange");
> +        geckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Test:Expected");

Combined these two `registerGeckoThreadListener` calls.

@@ +59,5 @@
>          super.tearDown();
>  
> +        final GeckoApp geckoApp = (GeckoApp) GeckoAppShell.getGeckoInterface();
> +        geckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Content:SecurityChange");
> +        geckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Test:Expected");

Combine these two `unregisterGeckoThreadListener` calls

Comment 13

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

Attachment: Desingletonize EventDispatcher

I think this covers everything you brought up.

Comment 14

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

Comment on attachment 8794990 [details] [diff] [review]
Desingletonize EventDispatcher

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

r+ with nits addressed. Thanks!

::: mobile/android/base/java/org/mozilla/gecko/BrowserApp.java
@@ -575,5 @@
>          final boolean isInAutomation = getIsInAutomationFromEnvironment(intent);
> -
> -        // This has to be prepared prior to calling GeckoApp.onCreate, because
> -        // widget code and BrowserToolbar need it, and they're created by the
> -        // layout, which GeckoApp takes care of.

You should put back this comment that you deleted, and remove the extra spaces you added in the blank line.

::: mobile/android/base/java/org/mozilla/gecko/GeckoApp.java
@@ +15,5 @@
>  import org.mozilla.gecko.health.HealthRecorder;
>  import org.mozilla.gecko.health.SessionInformation;
>  import org.mozilla.gecko.health.StubbedHealthRecorder;
>  import org.mozilla.gecko.home.HomeConfig.PanelType;
> +import org.mozilla.gecko.home.HomePanelsManager;

Unused import.

@@ +26,5 @@
>  import org.mozilla.gecko.notifications.AppNotificationClient;
>  import org.mozilla.gecko.notifications.NotificationClient;
>  import org.mozilla.gecko.notifications.NotificationHelper;
>  import org.mozilla.gecko.util.IntentUtils;
> +import org.mozilla.gecko.mdns.MulticastDNSManager;

Unused import.

@@ +1209,5 @@
>  
>          // GeckoThread has to register for "Gecko:Ready" first, so GeckoApp registers
>          // for events after initializing GeckoThread but before launching it.
>  
> +        getEventDispatcher().registerGeckoThreadListener((GeckoEventListener)this,

getAppEventDispatcher()

@@ +1214,5 @@
>              "Gecko:Ready",
>              "Gecko:Exited",
>              "Accessibility:Event");
>  
> +        getEventDispatcher().registerGeckoThreadListener((NativeEventListener)this,

getAppEventDispatcher()

@@ +1388,5 @@
>                  // The lifecycle of mHealthRecorder is "shortly after onCreate"
>                  // through "onDestroy" -- essentially the same as the lifecycle
>                  // of the activity itself.
>                  final String profilePath = getProfile().getDir().getAbsolutePath();
> +                final EventDispatcher dispatcher = getEventDispatcher();

getAppEventDispatcher()

@@ +1697,5 @@
>              @Override
>              public void run() {
>                  if (TabQueueHelper.TAB_QUEUE_ENABLED && TabQueueHelper.shouldOpenTabQueueUrls(GeckoApp.this)) {
>  
> +                    getEventDispatcher().registerGeckoThreadListener(new NativeEventListener() {

getAppEventDispatcher()

@@ +1702,4 @@
>                          @Override
>                          public void handleMessage(String event, NativeJSObject message, EventCallback callback) {
>                              if ("Tabs:TabsOpened".equals(event)) {
> +                                getEventDispatcher().unregisterGeckoThreadListener(this, "Tabs:TabsOpened");

getAppEventDispatcher()

@@ +2276,5 @@
>              super.onDestroy();
>              return;
>          }
>  
> +        getEventDispatcher().unregisterGeckoThreadListener((GeckoEventListener)this,

getAppEventDispatcher()

@@ +2281,5 @@
>              "Gecko:Ready",
>              "Gecko:Exited",
>              "Accessibility:Event");
>  
> +        getEventDispatcher().unregisterGeckoThreadListener((NativeEventListener)this,

getAppEventDispatcher()

::: mobile/android/base/java/org/mozilla/gecko/MediaPlayerManager.java
@@ +88,1 @@
>                                                                      "MediaPlayer:Load",

Nit: fix indent here.

::: mobile/android/base/java/org/mozilla/gecko/home/HomePanelsManager.java
@@ +17,5 @@
>  import org.json.JSONException;
>  import org.json.JSONObject;
>  import org.mozilla.gecko.db.HomeProvider;
>  import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.GeckoAppShell;

Unused import.

::: mobile/android/base/java/org/mozilla/gecko/lwt/LightweightTheme.java
@@ +13,5 @@
>  import org.json.JSONObject;
>  import org.mozilla.gecko.AppConstants.Versions;
>  import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.GeckoApp;
> +import org.mozilla.gecko.GeckoAppShell;

Unused imports.

::: mobile/android/base/java/org/mozilla/gecko/notifications/NotificationHelper.java
@@ +84,5 @@
>      public void init() {
> +        if (mInitialized) {
> +            return;
> +        }
> +        

Nit: extra space.

::: mobile/android/base/java/org/mozilla/gecko/preferences/GeckoPreferences.java
@@ +370,5 @@
>  
>          // Use setResourceToOpen to specify these extras.
>          Bundle intentExtras = getIntent().getExtras();
>  
> +        GeckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Sanitize:Finished", "Snackbar:Show");

Nit: put each event type string on its own line.

@@ +508,2 @@
>  
> +        GeckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Sanitize:Finished", "Snackbar:Show");

Nit: put each event type string on its own line.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java
@@ +20,5 @@
>  import android.content.res.Resources;
>  import android.graphics.drawable.Drawable;
>  import android.os.Bundle;
>  import android.util.AttributeSet;
> +import android.util.Log;

Unused import.

::: mobile/android/base/java/org/mozilla/gecko/toolbar/SiteIdentityPopup.java
@@ +97,5 @@
>          mResources = mContext.getResources();
>  
>          mContentButtonClickListener = new ContentNotificationButtonListener();
> +
> +        GeckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Doorhanger:Logins", "Permissions:CheckResult");

Nit: put each event type string on its own line.

@@ +546,5 @@
>          }
>      }
>  
>      void destroy() {
> +        GeckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Doorhanger:Logins", "Permissions:CheckResult");

Nit: put each event type string on its own line.

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/EventDispatcher.java
@@ +325,3 @@
>          }
>  
>          if (dispatchToThread(type, jsMessage, bundleMessage, callback,

We also call `callback.sendError` in dispatchToThread. Did you look into what we should do in that case?

::: mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java
@@ +1949,5 @@
>      }
>  
>      @WrapForJNI(calledFrom = "gecko")
>      private static void handleGeckoMessage(final NativeJSContainer message) {
> +        boolean success = EventDispatcher.getInstance().dispatchEvent(message);

final boolean success = EventDispatcher.getInstance().dispatchEvent(message) ||
    getGeckoInterface().getAppEventDispatcher().dispatchEvent(message);

@@ +1951,5 @@
>      @WrapForJNI(calledFrom = "gecko")
>      private static void handleGeckoMessage(final NativeJSContainer message) {
> +        boolean success = EventDispatcher.getInstance().dispatchEvent(message);
> +        success = success || getGeckoInterface().getAppEventDispatcher().dispatchEvent(message);
> +        if (!success) {

if (success) {
    message.disposeNative();
    return;
}

@@ +1955,5 @@
> +        if (!success) {
> +            final String type = message.optString("type", null);
> +            final String guid = message.optString(EventDispatcher.GUID, null);
> +            EventCallback callback = null;
> +            if (guid != null) {

if (type != null && guid != null) {
    (new EventDispatcher.GeckoEventCallback(guid, type)).sendError("No listeners for request");
}

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testEventDispatcher.java
@@ +7,5 @@
>  import static org.mozilla.gecko.tests.helpers.AssertionHelper.*;
>  
>  import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.GeckoApp;
> +import org.mozilla.gecko.GeckoAppShell;

Unused imports.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testFindInPage.java
@@ +8,5 @@
>  
>  import org.mozilla.gecko.Actions;
>  import org.mozilla.gecko.Element;
> +import org.mozilla.gecko.GeckoApp;
> +import org.mozilla.gecko.GeckoAppShell;

Unused import.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testRuntimePermissionsAPI.java
@@ +4,5 @@
>  
>  package org.mozilla.gecko.tests;
>  
> +import org.mozilla.gecko.GeckoApp;
> +import org.mozilla.gecko.GeckoAppShell;

Unused import.

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testSnackbarAPI.java
@@ +7,5 @@
>  import static org.mozilla.gecko.tests.helpers.AssertionHelper.fFail;
>  
>  import org.mozilla.gecko.EventDispatcher;
> +import org.mozilla.gecko.GeckoApp;
> +import org.mozilla.gecko.GeckoAppShell;

Unused import

::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testTrackingProtection.java
@@ +48,5 @@
>      @Override
>      public void setUp() throws Exception {
>          super.setUp();
>  
> +        GeckoApp.getEventDispatcher().registerGeckoThreadListener(this, "Content:SecurityChange", "Test:Expected");

Nit: put each event type string on its own line.

@@ +55,5 @@
>      @Override
>      public void tearDown() throws Exception {
>          super.tearDown();
>  
> +        GeckoApp.getEventDispatcher().unregisterGeckoThreadListener(this, "Content:SecurityChange", "Test:Expected");

Nit: put each event type string on its own line.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/893cd9c72641
De-singletonize EventDispatcher and make it application-specific. r=jchen

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/893cd9c72641