Robocop blockForEventData may drop events

RESOLVED FIXED in Firefox 25

Status

()

defect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: gbrown, Assigned: gbrown)

Tracking

unspecified
Firefox 25
x86
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Blocks: 876874
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
Attachment #779915 - Flags: review?(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.
Attachment #779915 - Flags: review?(margaret.leibovic) → review+
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
Attachment #779915 - Attachment is obsolete: true
Attachment #781943 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/2ee2dc1d2bcc
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Duplicate of this bug: 876874
You need to log in before you can comment on or make changes to this bug.