Closed Bug 815807 Opened 7 years ago Closed 7 years ago

Make Marionette load later in B2G startup cycle, except when --load-early is passed

Categories

(Testing :: Marionette, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-basecamp:+, firefox18 fixed, firefox19 fixed, firefox20 fixed)

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed
firefox20 --- fixed

People

(Reporter: jgriffin, Assigned: jgriffin)

References

Details

Attachments

(6 files, 3 obsolete files)

953 bytes, patch
ahal
: review+
Details | Diff | Splinter Review
8.80 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
990 bytes, patch
Details | Diff | Splinter Review
875 bytes, patch
Details | Diff | Splinter Review
1.43 KB, patch
past
: review+
Details | Diff | Splinter Review
5.17 KB, patch
jgriffin
: review+
Details | Diff | Splinter Review
In order to allow us to land bug 800138, which allows Marionette to work remotely on pandas, we need to load Marionette later in the startup process.  However, this breaks WebAPI tests on emulators.

Therefore, we need to optionally allow Marionette to preserve it's current early-loading behavior on B2G.  We can add a --load-early argument to facilitate this.
Target Milestone: --- → B2G C2 (20nov-10dec)
In order to implement this without breaking anything on TBPL, I'll have to land three patches in sequence:

1 - gecko patch which adds --load-early to the Marionette Python client
2 - a mozharness script change to add --load-early to the webapi tests
3 - gecko patch which causes the Marionette server to act differently when --load-early is passed
Assignee: nobody → jgriffin
This will be slightly bitrotted by bug 790817, but I'll unbitrot it before landing if needed.  This patch has no effect until parts 2 and 3 land (not yet submitted).
Attachment #686225 - Flags: review?(ahalberstadt)
This is the mozharness script change; it can't land until part 1 has propagated to all the trees, in order to avoid breaking things in TPBL
Attachment #686228 - Flags: review?(ahalberstadt)
Attachment #686225 - Attachment is obsolete: true
Attachment #686225 - Flags: review?(ahalberstadt)
Attachment #686225 - Attachment is obsolete: false
Comment on attachment 686225 [details] [diff] [review]
Part 1: add --load-early arg to Marionette,

Blargh, bzexport unset the review flag.
Attachment #686225 - Flags: review?(ahalberstadt)
Comment on attachment 686225 [details] [diff] [review]
Part 1: add --load-early arg to Marionette,

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

::: testing/marionette/client/marionette/emulator.py
@@ +401,5 @@
> +            self.wait_for_system_message(marionette)
> +
> +    def restart_b2g(self):
> +        print 'restarting B2G'
> +        self.dm.shellCheckOutput(['stop', 'b2g'])

We should probably start using mozb2g to get all of the b2g specific stuff out of here, but I won't block on that since this is time sensitive. I'm guilty of just adding the install_busybox method ;)
Attachment #686225 - Flags: review?(ahalberstadt) → review+
Attachment #686228 - Flags: review?(ahalberstadt) → review+
(In reply to Andrew Halberstadt [:ahal] from comment #5)
> Comment on attachment 686225 [details] [diff] [review]
> Part 1: add --load-early arg to Marionette,
> 
> Review of attachment 686225 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: testing/marionette/client/marionette/emulator.py
> @@ +401,5 @@
> > +            self.wait_for_system_message(marionette)
> > +
> > +    def restart_b2g(self):
> > +        print 'restarting B2G'
> > +        self.dm.shellCheckOutput(['stop', 'b2g'])
> 
> We should probably start using mozb2g to get all of the b2g specific stuff
> out of here, but I won't block on that since this is time sensitive. I'm
> guilty of just adding the install_busybox method ;)

I agree, after all the fires are out we should clean up a lot of things.  I'll file a bug for this if I can't find an existing one.
Bug 816236 filed for getting Marionette to use mozdevice/mozb2g.
Part 1: https://hg.mozilla.org/integration/mozilla-inbound/rev/60a1cb7e8b79
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta][leave open]
Unbitrotted; carry r+ forward.
Attachment #686225 - Attachment is obsolete: true
Attachment #686255 - Flags: review+
I realized I needed to tweak some code to prevent breakage until part 2 lands, so here it is.
And I realized when I merged Part 1 with inbound, on which bug 790817 landed, I made a typo which broke Marionette on TBPL.  Here's the fix.  :sigh:

https://hg.mozilla.org/integration/mozilla-inbound/rev/702ff40d1447
This patch takes some of past's work from bug 800138, along with patch 1 on this bug to load marionette early only if a pref is set.  Also does some cleanup of related code on the Python side.  Tested locally and it works fine.
Attachment #686343 - Flags: review?(ahalberstadt)
Comment on attachment 686343 [details] [diff] [review]
Part 3: Load marionette early in the B2G startup process if relevant pref is set, otherwise load it late,

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

Heh, I have no idea why any of this is needed. r+

Do you think in the future we could pass in the actual event to wait for via pref? That would make things a bit more flexible in case we find ourselves needing to wait for yet another event, or the events change.
Attachment #686343 - Flags: review?(ahalberstadt) → review+
https://hg.mozilla.org/releases/mozilla-aurora/rev/5b81398a1ea2
https://hg.mozilla.org/releases/mozilla-aurora/rev/7cbfbb0bcb89
https://hg.mozilla.org/releases/mozilla-aurora/rev/ba6628781ca1
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta][leave open] → [automation-needed-in-beta][leave open]
(In reply to Jonathan Griffin (:jgriffin) from comment #19)
> Pushed part 3 to try: https://tbpl.mozilla.org/?tree=Try&rev=c714fc81f334

This patch breaks mochitest.  The reason is that when we restart B2G after installing the test profile, we never get a 'system-message-listener-ready' event, so Marionette never gets initialized.  I'm not quite sure why this is.  The workaround is to add the loadearly pref to the test profile, for which I'll attach a patch.

We won't be able to run mochitests on pandas until we find a better way to deal with this, but I guess that's a fight for another day.
This revision fixes bustage with mochitest and reftest.
Attachment #686343 - Attachment is obsolete: true
Attachment #686930 - Flags: review?(ahalberstadt)
Pushed new version of Part 3 to try: https://tbpl.mozilla.org/?tree=Try&rev=bb5f83086e20
(In reply to Andrew Halberstadt [:ahal] from comment #15)
> Comment on attachment 686343 [details] [diff] [review]
> Part 3: Load marionette early in the B2G startup process if relevant pref is
> set, otherwise load it late,
> 
> Review of attachment 686343 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Heh, I have no idea why any of this is needed. r+
> 
> Do you think in the future we could pass in the actual event to wait for via
> pref? That would make things a bit more flexible in case we find ourselves
> needing to wait for yet another event, or the events change.

I wouldn't want to have to pass in a pref with an event name in it, since it would force the Python client to be more closely coupled with the JS bits, which would mean that version mistmaches between client and server (which are quite common in the wild and usually harmless) could end up being bad.  We'd also have to propagate a change like that to the JS client, so I think I'm in favor of leaving things as-is.
(In reply to Jonathan Griffin (:jgriffin) from comment #23)
> I wouldn't want to have to pass in a pref with an event name in it, since it
> would force the Python client to be more closely coupled with the JS bits,
> which would mean that version mistmaches between client and server (which
> are quite common in the wild and usually harmless) could end up being bad. 
> We'd also have to propagate a change like that to the JS client, so I think
> I'm in favor of leaving things as-is.

Oops, I shouldn't have said pref. I meant command line argument (i.e instead of --load-early, --load-event <event name>). Anyway, not important for now at least.
Comment on attachment 686930 [details] [diff] [review]
Part 3: Load marionette early in the B2G startup process if relevant pref is set, otherwise load it late

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

Lgtm
Attachment #686930 - Flags: review?(ahalberstadt) → review+
Part 3:  https://hg.mozilla.org/integration/mozilla-inbound/rev/9252522aab90

I'll also take what's left of bug 800138 and make a Part 4.
This contains the last gecko bits of bug 800138 that haven't been landed as parts of other patches.  The gaia PR for bug 800138 will have to land before we can close that bug.
Attachment #687187 - Flags: review?(past)
Carry r+ forward.  Fix desktop breakage by continuing to load Marionette early in non-B2G platforms.

Pushed to try:
https://tbpl.mozilla.org/?tree=Try&rev=b2a3ab386389
https://tbpl.mozilla.org/?tree=Try&rev=60b37c2de1b5
Attachment #686930 - Attachment is obsolete: true
Attachment #687284 - Flags: review+
Try run for b2a3ab386389 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=b2a3ab386389
Results (out of 4 total builds):
    success: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jgriffin@mozilla.com-b2a3ab386389
per aki's mtg w/jgrffin, this is blocking-basecamp, hence nominating.
blocking-basecamp: --- → ?
Attachment #687187 - Flags: review?(past) → review+
Try run for 60b37c2de1b5 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=60b37c2de1b5
Results (out of 12 total builds):
    success: 11
    warnings: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/jgriffin@mozilla.com-60b37c2de1b5
blocking-basecamp: ? → +
Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b70ac0124fe
Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/41be1ff63431
blocking-basecamp: + → ?
Whiteboard: [leave open] → [automation-needed-in-aurora][automation-needed-in-beta][leave open]
(In reply to Jonathan Griffin (:jgriffin) from comment #36)
> Part 3: https://hg.mozilla.org/integration/mozilla-inbound/rev/1b70ac0124fe
> Part 4: https://hg.mozilla.org/integration/mozilla-inbound/rev/41be1ff63431

I have no idea why this reset the blocking-basecamp flag.
blocking-basecamp: ? → +
All patches have landed.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [automation-needed-in-aurora][automation-needed-in-beta][leave open]
You need to log in before you can comment on or make changes to this bug.