Closed
Bug 896199
Opened 10 years ago
Closed 10 years ago
Robocop blockForEventData may drop events
Categories
(Firefox for Android Graveyard :: General, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 25
People
(Reporter: gbrown, Assigned: gbrown)
References
Details
Attachments
(1 file, 1 obsolete file)
11.09 KB,
patch
|
gbrown
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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+
![]() |
Assignee | |
Comment 3•10 years ago
|
||
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+
![]() |
Assignee | |
Comment 4•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/2ee2dc1d2bcc
OS: Mac OS X → Android
Comment 5•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2ee2dc1d2bcc
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 25
Updated•2 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•