859563 - GeckoEventExpecter instances are reused incorrectly

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chris Peterson [:cpeterson]]

Attachment: add-EventExpecter-reuse-checks.patch

GeckoEventExpecter.blockForEvent() and blockUntilClear() call mUnregisterEventListener:

  https://hg.mozilla.org/mozilla-central/file/b0d842380959/build/mobile/robocop/FennecNativeActions.java.in#l143

This means GeckoEventExpecter instances should not be reused because subsequent calls to blockForEvent() will wait on an event that has been unregistered!

However, some tests do reuse GeckoEventExpecter instances and they seem to work, e.g.:

  https://hg.mozilla.org/mozilla-central/file/b0d842380959/mobile/android/base/tests/testDistribution.java.in#l186

The problem is that GeckoEventListener's call to mUnregisterEventListener.invoke() fails to unregister the event listener! I _think_ EventDispatcher.unregisterEventListener() fails to find the GeckoEventListener to be unregistered because the .invoke() proxy is breaking the object identity of the GeckoEventListener instances that are registered.

Please see the attached patch for some test code that identifies reused GeckoEventExpecter instances. We should probably allow GeckoEventExpecter instances to be reused (so not unregister in blockForEvent()), but also fix the problem with unregistering GeckoEventExpecter instances.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: fix unregister; move unregisterListener into its own function

A simple change to the equals() method allows the listener to be unregistered successfully; I like using strings here so that it's easy to debug.

Listener unregistration is moved into its own unregisterListener function, so that an EventExpecter can be re-used when needed (when waiting for a message with certain data contents, for instance). Callers are responsible for calling unregisterListener explicitly now, but if they don't, it likely won't cause any problems - this is just about tidying up resources in a timely manner.

I have also rolled in your re-use checks, modified slightly, so that an exception is thrown if an attempt is made to wait on an expecter that is not listening for an event.

Comment 2

[Geoff Brown [:gbrown]]

Attachment: change all the tests to call unregisterListener

...also found and corrected a mis-use of an EventExpecter in testFlingCorrectness.

Comment 3

[Chris Peterson [:cpeterson]]

Comment on attachment 739106 [details] [diff] [review]
fix unregister; move unregisterListener into its own function

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

r+ with some minor changes

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +89,5 @@
>              if(methodName.equals("equals")) {
> +                return
> +                    this == null ? args[0] == null :
> +                    (args[0] == null ? false :
> +                     this.toString().equals(args[0].toString()));

Can `this` ever be null? Wouldn't the caller throw an NullPointerException? If so, then this equals expression could be simplified to something like:

  return args[0] != null && args[0].toString().equals(this.toString());

@@ +117,5 @@
> +            if (TextUtils.isEmpty(geckoEvent)) {
> +                throw new IllegalArgumentException("geckoEvent must not be empty");
> +            }
> +            if (registrationParams == null || registrationParams.length == 0) {
> +                throw new IllegalArgumentException("registrationParams must not be empty"); 

Trailing whitespace.

@@ +315,5 @@
>              }
>          }
>  
>          private synchronized void blockForEvent(long millis, boolean failOnTimeout) {
> +            if (mListening == false) {

I would just: if (!mListening) {

@@ +357,5 @@
>              return mPaintDone;
>          }
>  
>          public synchronized void blockUntilClear(long millis) {
> +            if (mListening == false) {

I would just: if (!mListening) {

@@ +404,4 @@
>              try {
>                  mSetDrawListener.invoke(mRobocopApi, (Object)null);
> +                mListening = false;
> +                FennecNativeDriver.log(LogLevel.INFO, "PaintExpecter: no longer listening for events");

I think we should move `mListening = false` and the log message before mSetDrawListener(). Even if invoke() throws an exception because it failed to unregister the listener, we still want GeckoEventExpecter to believe it is not reusable.

Comment 4

[Chris Peterson [:cpeterson]]

Comment on attachment 739108 [details] [diff] [review]
change all the tests to call unregisterListener

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

LGTM

Comment 5

[Chris Peterson [:cpeterson]]

Comment on attachment 739106 [details] [diff] [review]
fix unregister; move unregisterListener into its own function

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

More comments:

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +213,5 @@
>              blockForEvent(millis, false);
>              return mEventData;
>          }
>  
> +        public synchronized void unregisterListener() {

1. Should we throw an exception if mRegistrationParams == null because a test called unregisterListener() twice?

2. Should we add a finalize() method that throws an exception (or just logs an error) if the GeckoEventListener was not unregistered (i.e. mRegistrationParams != null)?

@@ +399,5 @@
>                  startTime = endTime;
>              }
> +        }
> +
> +        public synchronized void unregisterListener() {

1. Should we throw an exception if !mListening because a test called unregisterListener() twice?

2. Should we add a finalize() method that throws an exception (or just logs an error) if the PaintExpecter was not unregistered (i.e. mListening is still true)?

Comment 6

[Geoff Brown [:gbrown]]

Attachment: fix unregister; move unregisterListener into its own function

Updated for review comments 3 and 5. r+ carried.

Thanks for the suggestions.

> 2. Should we add a finalize() method that throws an exception (or just logs an
> error) if the GeckoEventListener was not unregistered (i.e. 
> mRegistrationParams != null)?

I did not make this change. I want to encourage better use of EventExpecter, but I don't want to be overly-restrictive or risk logging warning messages where they are not necessary.

Comment 7

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6a2592dba813
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec53926a1488

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/ec53926a1488
https://hg.mozilla.org/mozilla-central/rev/6a2592dba813