Closed Bug 839675 Opened 12 years ago Closed 11 years ago

Prevent out-of-sync problems

Categories

(Remote Protocol :: Marionette, defect)

defect
Not set
normal

Tracking

(firefox19 wontfix, firefox20 wontfix, firefox21 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 fixed)

RESOLVED FIXED
mozilla21
Tracking Status
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
Attached patch Prevent out-of-sync problems, (obsolete) — Splinter Review
This prevents out of sync problems completely.  It doesn't solve any socket.timeout problems.
Attachment #712019 - Flags: review?(mdas)
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?
Attachment #712019 - Flags: review?(mdas) → review+
(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.
This patch fails try with a bunch of errors of the sort:

MarionetteException: Emulator callback still pending when finish() called
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
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
Updated patch that fixes oranges shown by try
Attachment #712019 - Attachment is obsolete: true
Comment on attachment 712977 [details] [diff] [review]
Prevent out-of-sync problems,

Carry r+ forward.
Attachment #712977 - Flags: review+
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
Attachment #712977 - Flags: approval-mozilla-b2g18?
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.
Attachment #712977 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
(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:)
Dammit, wires got crossed, I thought this was a different bug. Ignore my comment please.
https://hg.mozilla.org/mozilla-central/rev/ede7e0cd808b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
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.
Oops, I inadvertently left in a little extra logging I'd added.  This removes it.
Attachment #713508 - Flags: review?(mdas)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #713508 - Flags: review?(mdas) → review+
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:
Attachment #713508 - Flags: approval-mozilla-b2g18?
https://hg.mozilla.org/mozilla-central/rev/e02c8045c228
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
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.
Attachment #713508 - Flags: approval-mozilla-b2g18? → approval-mozilla-b2g18+
Product: Testing → Remote Protocol
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: