857335 - Log an Event Dispatcher warning when an event has no registered event listeners

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: add-EventDispather-logging.patch

These log messages would have helped me diagnose a test bug related to the order in which events where registered and dispatched.

I don't expect many warnings to be logged. During startup and typical browsing, I only saw to warnings:

  D/GeckoEventDispatcher(32139): dispatchEvent: no listeners registered
  for event "FormHistory:Init:Return"
  D/GeckoEventDispatcher(32139): dispatchEvent: no listeners registered
  for event "Passwords:Init:Return"


* wesj: Do those warnings point to real bugs? You add those sendMessageToJava() calls in bug 704682 and bug 725881.

Comment 1

[Wesley Johnston (:wesj)]

I don't think we need those messages anymore. I think I was using them at one point to know, "Hey, you can start using the db now!". But at this point we just fail and wait for Sync (the only person using this stuff) to try again in an hour or so. We should remove 'em.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 732564 [details] [diff] [review]
add-EventDispather-logging.patch

Seems useful, but...

... it is possible that we will send messages that Firefox itself does not use, especially if Java-based add-ons become more prevalent. Will this output become too noisy if that situation ever happens?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

I think if that happens we can take the logging out at that point, or whitelist some messages that are intended for add-ons. Note that if we do add messages for Java add-ons that we're not using ourselves, then a default install of Firefox (without addons) will have the most amount of these logs, so we should be able to catch such spurious logging before we ship.

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 732564 [details] [diff] [review]
add-EventDispather-logging.patch

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

In general I'm not a fan of dumping the stacktrace when it provides no useful information. I guess it makes it more noticeable in the logcat which is probably why you did it, but still, it feels unnecessarily clutter-y to me. I'll leave that up to you guys though.

::: mobile/android/base/util/EventDispatcher.java
@@ +44,5 @@
> +                return;
> +            }
> +            if (listeners.size() == 0) {
> +                Log.e(LOGTAG, "BUG", new IllegalStateException("Found empty listener list for " +
> +                                                               "event \"" + event + "\""));

Instead of checking the size here, I would prefer checking the return value of the remove() call below. That way we can catch a broader range of errors (which includes the case that we're unregistering a listener that wasn't in the list).

@@ +72,1 @@
>              String type = json.getString("type");

Can we rearrange this a bit to avoid calling json.getString("type") twice? Maybe define "String type;" before the if condition, and move this assignment into an else block.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 732564 [details] [diff] [review]
add-EventDispather-logging.patch

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

Whoops, looks like i clobbered mark's review. Putting it back.

Comment 6

[Chris Peterson [:cpeterson]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> In general I'm not a fan of dumping the stacktrace when it provides no
> useful information. I guess it makes it more noticeable in the logcat which
> is probably why you did it, but still, it feels unnecessarily clutter-y to
> me. I'll leave that up to you guys though.

I can remove the stack trace dumps. I only logged the stack traces for cases that were "real bugs", not just warnings or diagnostic info.


> Instead of checking the size here, I would prefer checking the return value
> of the remove() call below. That way we can catch a broader range of errors
> (which includes the case that we're unregistering a listener that wasn't in
> the list).

OK.


> Can we rearrange this a bit to avoid calling json.getString("type") twice?
> Maybe define "String type;" before the if condition, and move this
> assignment into an else block.

OK. I had originally written the code to avoid calling getString() twice, but the code was a little more confusing to handle a deprecated code path.

Comment 7

[Chris Peterson [:cpeterson]]

Attachment: add-EventDispatcher-warnings-v2.patch

Patch v2 changes:
* Remove stack traces from log warnings
* Avoid extra call to json.getString("type")
* Add log warnings about remove() return value

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 733097 [details] [diff] [review]
add-EventDispatcher-warnings-v2.patch

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

LGTM. I would probably use single-quote characters in the error messages to avoid having to escape them with backslashes, but I don't really care either way. Your call.

Comment 9

[Chris Peterson [:cpeterson]]

Landed with single quotes:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4f46d70fa1c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

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