997679 - marionette can be called before session is started

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::UI Tests, defect)

Description

[Zac C (:zac)]

If a particular combination of workflow and crash occured, marionette could be called before a session is created.

A quick rejig of GaiaDevice class would avoid this.

Comment 1

[Zac C (:zac)]

Attachment: github pr

This partially unwinds some of the interdependencies in GaiaDevice.

No groundbreaking changes though!

Comment 2

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8408182 [details] [review]
github pr

This will regress Eideticker and cause the update notifications to return. Do you have an example where GaiaDevice is instantiated without a Marionette session? Perhaps we should make this a prerequisite and throw an exception if a session does not exist (though I suspect this is already what happens).

Comment 3

[Zac C (:zac)]

It occurs when we're recovering from a crash (where Gecko has restarted and the session lost). We would hit a marionette exception but we don't want to because then we'll fail to recover from the crash.

GaiaDevice, setUp and marionette have this horrible interdependency. I'm going to try and unravel it, somehow, but I will try do it in several small steps like this over time to not bust up every other project.

How will this regress Eideticker? Does Eideticker use its own setUp?  I thought it a good practice to split the Update Checker out of the GaiaDevice for various reasons (maybe someone needs GaiaDevice but not the checker) so perhaps Eideticker could modify its setUp with the next gaiatest?

Comment 4

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #3)
> It occurs when we're recovering from a crash (where Gecko has restarted and
> the session lost). We would hit a marionette exception but we don't want to
> because then we'll fail to recover from the crash.

I'm not sure I follow here. When we encounter a crash the Marionette session will always be lost. What's preventing us from recovering from the crash? It sounds like the real issue is somewhere else.

> GaiaDevice, setUp and marionette have this horrible interdependency. I'm
> going to try and unravel it, somehow, but I will try do it in several small
> steps like this over time to not bust up every other project.

Making these cleaner sounds good to me, though I suspect some dependencies are unavoidable.

> How will this regress Eideticker? Does Eideticker use its own setUp?  I
> thought it a good practice to split the Update Checker out of the GaiaDevice
> for various reasons (maybe someone needs GaiaDevice but not the checker) so
> perhaps Eideticker could modify its setUp with the next gaiatest?

As mentioned in comment 2, Eideticker does not use GaiaTestCase and therefore does not use GaiaTest.setUp. It does use GaiaDevice and start_b2g though, so benefits from the fake update checker. The fake update checker should probably be set as close to starting b2g as possible, and so perhaps we could move the import into there. It would mean it's not available if you haven't called start_b2g, but then I'm not sure there are other cases where you'd need to use it.

Comment 5

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #4)
> (In reply to Zac C (:zac) from comment #3)
> > It occurs when we're recovering from a crash (where Gecko has restarted and
> > the session lost). We would hit a marionette exception but we don't want to
> > because then we'll fail to recover from the crash.
> 
> I'm not sure I follow here. When we encounter a crash the Marionette session
> will always be lost. What's preventing us from recovering from the crash? It
> sounds like the real issue is somewhere else.
> 

GaiaDevice's __init__ will try and import the lockscreen atom before stop_b2g/start_b2g (which starts the marionette session) has been called. If Gecko has actually restarted quick enough and the marionette server is up, then we'll get the exception from the marionette server before we've had the chance to execute start_session.

This patch doesn't change the timing of the FakeUpdateChecker at all, it is immediately after start_b2g whereas before it was the last step of that. If you move it into start_b2g then you're just bringing it back into GaiaDevice.

Comment 6

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #5)
> (In reply to Dave Hunt (:davehunt) from comment #4)
> > (In reply to Zac C (:zac) from comment #3)
> > > It occurs when we're recovering from a crash (where Gecko has restarted and
> > > the session lost). We would hit a marionette exception but we don't want to
> > > because then we'll fail to recover from the crash.
> > 
> > I'm not sure I follow here. When we encounter a crash the Marionette session
> > will always be lost. What's preventing us from recovering from the crash? It
> > sounds like the real issue is somewhere else.
> > 
> 
> GaiaDevice's __init__ will try and import the lockscreen atom before
> stop_b2g/start_b2g (which starts the marionette session) has been called. If
> Gecko has actually restarted quick enough and the marionette server is up,
> then we'll get the exception from the marionette server before we've had the
> chance to execute start_session.

Okay, thank you for the more detailed explanation. By moving the import of the fake update checker into start_b2g you ensure you have a new session, so that should solve that problem.

> This patch doesn't change the timing of the FakeUpdateChecker at all, it is
> immediately after start_b2g whereas before it was the last step of that. If
> you move it into start_b2g then you're just bringing it back into GaiaDevice.

It completely changes the timing of FakeUpdateChecker - it's moved from GaiaDevice.start_b2g to GaiaTestCase.setUp.

Comment 7

[Zac C (:zac)]

Comment on attachment 8408182 [details] [review]
github pr

re r? now that you understand the full context.

Comment 9

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8408182 [details] [review]
github pr

This will still adversely affect Eideticker.

Comment 10

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #9)
> Comment on attachment 8408182 [details] [review]
> github pr
> 
> This will still adversely affect Eideticker.

I will make the pull request to change the setUp in Eideticker.

Comment 11

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #10)
> (In reply to Dave Hunt (:davehunt) from comment #9)
> > Comment on attachment 8408182 [details] [review]
> > github pr
> > 
> > This will still adversely affect Eideticker.
> 
> I will make the pull request to change the setUp in Eideticker.

That's not necessary, you can just put the import and check updates call in start_b2g after a new Marionette session is created.

Comment 12

[Zac C (:zac)]

I would rather not make it implied like that. I'd rather give the flexibility to keep updates, for example in a test suite that does actually checks updates (which has been asked of us), can create their own setUp method, use start_b2g and omit the update checker override.

Some parts of start_b2g implied are valid like establishing the new marionette session, but even as Bob is finding in the other bug it could even be better to abstract out the wait method too, one day.

Comment 13

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #12)
> I would rather not make it implied like that. I'd rather give the
> flexibility to keep updates, for example in a test suite that does actually
> checks updates (which has been asked of us), can create their own setUp
> method, use start_b2g and omit the update checker override.

Well that's a change of the current behaviour. I was previously going to suggest an optional argument on start_b2g for disabling updates, but really that can also be taken care of separately.

> Some parts of start_b2g implied are valid like establishing the new
> marionette session, but even as Bob is finding in the other bug it could
> even be better to abstract out the wait method too, one day.

Which 'other bug'? please provide a bug number when referencing other bugs. If we don't establish a new Marionette session after a restart, it leaves the instance in a fairly unusable state.

Comment 14

[Zac C (:zac)]

(In reply to Dave Hunt (:davehunt) from comment #13)

> Which 'other bug'? please provide a bug number when referencing other bugs.

https://bugzilla.mozilla.org/show_bug.cgi?id=1000334

Comment 15

[Zac C (:zac)]

Dave, today when I was working using b2gpopulate I noticed that it uses start_b2g and hence does the fake update check. I thought this was a case where the user might not necessarily want the update disabled but b2gpopulate is doing it silently.

On the balance of things I think this change is sensible to give the option to run the update checker or not to the consumer of gaiatest and not mess with the device in a way that's undesired, even if it's a small change for eideticker.

Comment 16

[Zac C (:zac)]

The ni? is what can I do to convince you that this is a good step?

Comment 17

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #15)
> Dave, today when I was working using b2gpopulate I noticed that it uses
> start_b2g and hence does the fake update check. I thought this was a case
> where the user might not necessarily want the update disabled but
> b2gpopulate is doing it silently.

Yes, and b2gperf does the same.

> On the balance of things I think this change is sensible to give the option
> to run the update checker or not to the consumer of gaiatest and not mess
> with the device in a way that's undesired, even if it's a small change for
> eideticker.

I don't disagree with this, however it's really out of scope for this bug, which is about a marionette session being used before it is created. If you want to compound the two items then providing a method to disable the update checker would make sense, but we would need to make sure that it does not adversely affect Eideticker, which is now reliant on the notifications being disabled. We would most likely need a release with the new method, where it's called from start_b2g, followed by an update to Eideticker, followed by the removal of the implicit disabling of the update checker. For the record, I have never had any objections to this, I would just prefer it to be handled in a separate bug.

Comment 18

[Zac C (:zac)]

Well Marionette is used in the __init__ of the FakeUpdateChecker class, which is instantiated in the __init__ of GaiaDevice so this is sitll in the scope of the bug. However I can change this to be a little bit less aggressive. That will make it less risky to make the change to Eideticker.

Comment 19

[Zac C (:zac)]

OK I re-jigged the pull so the fake update check is still implicit in start_b2g, but it will only be run after start_b2g and hence a session will definitely exist.

I'll open another bug for splitting the start_b2g / FakeUpdateChecker tangle.

Comment 20

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #18)
> Well Marionette is used in the __init__ of the FakeUpdateChecker class,
> which is instantiated in the __init__ of GaiaDevice so this is sitll in the
> scope of the bug. However I can change this to be a little bit less
> aggressive. That will make it less risky to make the change to Eideticker.

Yep, that's what I suggested in comment 11.

Comment 21

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 8408182 [details] [review]
github pr

It's good practice to not squash commits until the review process is complete, as it makes reviewing just the latest changes much easier.

Comment 22

[Zac C (:zac)]

Thanks Dave.

Merged:
https://github.com/mozilla-b2g/gaia/commit/7c4f227c2794f5fb20db738e4ceb12d68be94d77

Comment 23

[Dave Hunt [:davehunt] [he/him] ⌚BST]

(In reply to Zac C (:zac) from comment #19)
> OK I re-jigged the pull so the fake update check is still implicit in
> start_b2g, but it will only be run after start_b2g and hence a session will
> definitely exist.
> 
> I'll open another bug for splitting the start_b2g / FakeUpdateChecker tangle.

Has this bug been filed? I was working on Eideticker yesterday and noticed that start_b2g is not used [1], so this change will in fact still cause the update notifications to appear once a new gaiatest is released. We should update Eideticker to use start_b2g or to explicitly use the fake update checker (or both, potentially). Either way, we'll need to have the patch ready to land shortly after a new gaiatest is released. The new bug should block bug 1002541.

[1] https://github.com/mozilla/eideticker/blob/2e6ce7b526d634463d16f4194ea81a4fc3681473/src/eideticker/eideticker/device.py#L453

Comment 24

[Zac C (:zac)]

Dave, I've filed it: https://bugzilla.mozilla.org/show_bug.cgi?id=1003784