839675 - Prevent out-of-sync problems

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

[Jonathan Griffin (:jgriffin)]

In bug 779011 I implemented some logic that could reduce out-of-sync problems, but it didn't go quite far enough.  I'll extend that logic and enforce it to make it impossible for Marionette to get out-of-sync.

Comment 1

[Jonathan Griffin (:jgriffin)]

Attachment: Prevent out-of-sync problems,

This prevents out of sync problems completely.  It doesn't solve any socket.timeout problems.

Comment 2

[Jonathan Griffin (:jgriffin)]

pushed to try:

b2g: https://tbpl.mozilla.org/?tree=Try&rev=8287bbacb883
desktop: https://tbpl.mozilla.org/?tree=Try&rev=f7f3a3042e0d

Comment 3

[Malini Das [:mdas] - Away, not checking bugmail]

Comment on attachment 712019 [details] [diff] [review]
Prevent out-of-sync problems,

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

hurray!

::: testing/marionette/marionette-actors.js
@@ +472,4 @@
>     */
>    newSession: function MDA_newSession() {
>      this.command_id = this.getCommandId();
> +    this.newSessionCommandId = this.command_id;

Why can't we use this.command_id instead of creating newSessionCommandId? I don't think we reassign it anywhere during newSession() or any functions it calls, do we?

Comment 4

[Jonathan Griffin (:jgriffin)]

(In reply to Malini Das [:mdas] from comment #3)
> ::: testing/marionette/marionette-actors.js
> @@ +472,4 @@
> >     */
> >    newSession: function MDA_newSession() {
> >      this.command_id = this.getCommandId();
> > +    this.newSessionCommandId = this.command_id;
> 
> Why can't we use this.command_id instead of creating newSessionCommandId? I
> don't think we reassign it anywhere during newSession() or any functions it
> calls, do we?

The call chain that results from newSession is rather convoluted.  We don't send a response back to the client until a frame script registers itself in the Marionette:register handler.  I use the the 'this.newSessionCommandId' variable there to assign the correct command_id to the response.  It would be nicer to pass the command_id along as we do for other commands, but this is hard because of the way we're hooking up the frame script in newSession.

Comment 5

[Jonathan Griffin (:jgriffin)]

This patch fails try with a bunch of errors of the sort:

MarionetteException: Emulator callback still pending when finish() called

Comment 6

[Jonathan Griffin (:jgriffin)]

This was a silly error on my part.  Fixed and pushed to try again:

https://tbpl.mozilla.org/?tree=Try&rev=d7297db0aed2
https://tbpl.mozilla.org/?tree=Try&rev=e094e7dc9974

Comment 7

[Jonathan Griffin (:jgriffin)]

Fixed all the oranges in the last try run and pushed to try again:

https://tbpl.mozilla.org/?tree=Try&rev=7ea2bf40784c
https://tbpl.mozilla.org/?tree=Try&rev=d4bd329c05df

Comment 8

[Jonathan Griffin (:jgriffin)]

Attachment: Prevent out-of-sync problems,

Updated patch that fixes oranges shown by try

Comment 9

[Jonathan Griffin (:jgriffin)]

Comment on attachment 712977 [details] [diff] [review]
Prevent out-of-sync problems,

Carry r+ forward.

Comment 10

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ede7e0cd808b

Comment 11

[Jonathan Griffin (:jgriffin)]

Comment on attachment 712977 [details] [diff] [review]
Prevent out-of-sync problems,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: None - test-only patch
Testing completed: Full try runs
Risk to taking this patch (and alternatives if risky): 
  This patch enhances the stability of Marionette-based tests, including Gaia UI tests.  Not taking it will prevent these stability enhancements from taking effect.
String or UUID changes made by this patch: None

Comment 12

[Alex Keybl [:akeybl]]

Comment on attachment 712977 [details] [diff] [review]
Prevent out-of-sync problems,

Test-only patch, if this sticks on m-i/m-c (I'll let you all decide on when you're confident with the landing) we can uplift to mozilla-b2g18.

Comment 13

[Malini Das [:mdas] - Away, not checking bugmail]

(In reply to Alex Keybl [:akeybl] from comment #12)
> Comment on attachment 712977 [details] [diff] [review]
> Prevent out-of-sync problems,
> 
> Test-only patch, if this sticks on m-i/m-c (I'll let you all decide on when
> you're confident with the landing) we can uplift to mozilla-b2g18.

This has already landed on m-c, so we're clear:)

Comment 14

[Malini Das [:mdas] - Away, not checking bugmail]

Dammit, wires got crossed, I thought this was a different bug. Ignore my comment please.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/3dbddbdc7a26

Comment 17

[Jonathan Griffin (:jgriffin)]

This actually made problems on Jenkins worse in a way, since a test that fails due to a frozen UI will cause all subsequent tests to fail out very quickly, before the UI unfreezes.  Before the out-of-sync problem was fixed, the tests could get to an execute call that would take some time to error out; a few of these would allow enough time to progress that that the UI would unfreeze and allow other tests to function.

Comment 18

[Jonathan Griffin (:jgriffin)]

Attachment: Remove extra logging,

Oops, I inadvertently left in a little extra logging I'd added.  This removes it.

Comment 19

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e02c8045c228

Comment 20

[Jonathan Griffin (:jgriffin)]

Comment on attachment 713508 [details] [diff] [review]
Remove extra logging,

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed: 
Risk to taking this patch (and alternatives if risky): 
This removes some extra logging accidentally left from a previous patch.  Test-only and no user impact.
String or UUID changes made by this patch:

Comment 21

[Ed Morley [:emorley]]

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

Comment 22

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 713508 [details] [diff] [review]
Remove extra logging,

Approving for v1-train uplift, fwiw in the future fixes that are only to tests can be a=test-only commits.

Comment 23

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18/rev/14bd266b0757

Comment 24

[Jonathan Griffin (:jgriffin)]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/eb5e87a42a25