896199 - Robocop blockForEventData may drop events

Status: RESOLVED FIXED

(Firefox for Android :: General, defect)

Description

[Geoff Brown [:gbrown]]

When a Robocop test listens on an EventExpecter with blockForEventData, it is possible for the EventExpecter to receive 2 or more messages before blockForEventData returns; in this case, only the most recently received message is returned to the test -- the others are silently discarded (overwritten). 

https://hg.mozilla.org/mozilla-central/file/9af6265e9884/build/mobile/robocop/FennecNativeActions.java.in#l241

This happens intermittently in testDistribution, when several Preferences:Data messages are generated at about the same time.

Comment 1

[Geoff Brown [:gbrown]]

Attachment: use a queue in Robocop's GeckoEventExpecter

The problem I am solving here is that in our current code, it is possible for notifyOfEvent() to be called twice (or more) before blockForEventData() has a chance to return the received message; in that case, mEventData is overwritten and the event is never seen by the test.

I remove the wait/notifyAll and use a LinkedBlockingQueue instead:

http://developer.android.com/reference/java/util/concurrent/LinkedBlockingQueue.html

That also allows for removal of most of the synchronized sections of GeckoEventExpecter (I am a little nervous about that, but I cannot find a good reason to keep them, and keeping them all with the queue causes a deadlock).

Try runs:
https://tbpl.mozilla.org/?tree=Try&rev=c5132f48467f
https://tbpl.mozilla.org/?tree=Try&rev=c4b05de4ce75

Comment 2

[:Margaret Leibovic]

Comment on attachment 779915 [details] [diff] [review]
use a queue in Robocop's GeckoEventExpecter

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

This looks good to me, especially since those try runs look really green. Nice find on LinkedBlockingQueue, it makes this code look more robust.

I'm still not completely comfortable with synchronized code, so you may want someone else to take a look at this if you're not sure about removing those. And I think there are some more things you can remove, see comments below.

::: build/mobile/robocop/FennecNativeActions.java.in
@@ +140,5 @@
> +            }
> +            if (mEventData == null) {
> +                if (failOnTimeout) {
> +                    FennecNativeDriver.logAllStackTraces(FennecNativeDriver.LogLevel.ERROR);
> +                    mAsserter.ok(false, "GeckoEventExpecter", 

Nit: trailing whitespace.

@@ +167,5 @@
> +                mEventData = mEventDataQueue.poll(MAX_WAIT_MS, TimeUnit.MILLISECONDS);
> +            } catch (InterruptedException ie) {
> +                FennecNativeDriver.log(LogLevel.ERROR, ie);
> +            }
> +            endTime = SystemClock.uptimeMillis();

Time isn't used anymore, so we can get rid of it. In fact, I think we can get rid of the endTime/startTime variables altogether.

@@ -189,3 @@
>                  } catch (InterruptedException ie) {
> -                    FennecNativeDriver.log(LogLevel.ERROR, ie);
> -                    break;

Should we keep this break?

@@ +184,3 @@
>                  }
>                  endTime = SystemClock.uptimeMillis();
> +                if ((mEventData == null) && (endTime - startTime >= millis)) {

Do we still need to do this endTime/startTime check? Shouldn't poll take care of that for us?

@@ +231,5 @@
> +            }
> +            try {
> +                mEventDataQueue.put(args[1].toString());
> +            } catch (InterruptedException e) {
> +                FennecNativeDriver.log(LogLevel.ERROR, 

Nit: trailing whitespace.

@@ +238,4 @@
>              }
>          }
>      }
>      

There's also some whitespace here you could kill.

Comment 3

[Geoff Brown [:gbrown]]

Attachment: use a queue in Robocop's GeckoEventExpecter

Updated for review comments. I removed the remaining startTime/endTime references. I reconsidered the break and decided to remove it -- I think it's better to retry in this case. r+ carried.

New try: https://tbpl.mozilla.org/?tree=Try&rev=a91441f80486

Comment 4

[Geoff Brown [:gbrown]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee2dc1d2bcc

Comment 5

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2ee2dc1d2bcc