775811 - Separate out ConsoleService listener and other sensitive code into its own component

Status: RESOLVED FIXED

(Testing Graveyard :: Mozmill, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

See bug 742766 where I have seen that in some cases when Mozmill modules are broken due to syntax errors or a wrong usage of Cc/Ci we can't make a test to fail because code in those modules are not getting loaded and executed. The worst case is if frame.js is broken.

We should move at least the ConsoleService listener to a real component without any dependency on other modules. That way the chance that something is broken is minimal and we could ensure to really transmit all the information to the framework and mark tests as failed.

Also keep in mind that currently we do not shutdown the browser but letting it timeout. We should close it right away. Otherwise testruns with hundreds of tests will kill our time schedule.

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: WIP

Pointer to Github pull-request

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 644262 [details]
WIP

So this is just a WIP. It has the same behavior as before but now we ensure to always report framework failures. Even when driver/mozmill.js is broken.

Andrew, I don't have that much experience in sending events to Python. Is there a way we can do that without including frame.js and use its event method?

Comment 3

[Andrew Halberstadt [:ahal]]

This code adds a global listener to the frame.js events object.
https://github.com/mozautomation/mozmill/blob/d623b520b5c562209cafc599bea25d3d40ffccd3/mozmill/mozmill/extension/resource/modules/frame.js#L419

You probably just need this code in the component and you can do the same thing frame.js does (see events.fireEvent).

I'm not 100% sure what moving the ConsoleListener to a new component is solving though.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrew Halberstadt [:ahal] from comment #3)
> This code adds a global listener to the frame.js events object.
> https://github.com/mozautomation/mozmill/blob/
> d623b520b5c562209cafc599bea25d3d40ffccd3/mozmill/mozmill/extension/resource/
> modules/frame.js#L419
>
> You probably just need this code in the component and you can do the same
> thing frame.js does (see events.fireEvent).

Thanks I will check that.

> I'm not 100% sure what moving the ConsoleListener to a new component is
> solving though.

Have you thought about what happens if the mozmill.js module is broken due to one of those many dependent modules? It will not be executed and the same applies to the console listener. Having a separate module which gets registered as early as possible in the process, let you catch even those failures.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Patch

Pointer to Github pull-request

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 646073 [details]
Patch

With this patch we finally now report any framework failure to the Python side and include the failure in the report. That means no longer generic 'Application disconnect' messages in our report but meaningful descriptions what's wrong when running the test in the application.

I have tested this with nearly each module of the mozmill extension and it works great. One caveat we have through, if mozmill.js is broken we can't retrieve the application information. So I left it out of the report, which is even better as dying completely.

Further if a problem with the framework exists, we now log it to the console as critical so you can see immediately what happened! That was not the case before and caused a lot of headaches for me.

Asking for feedback/review from everyone involved in JSBridge, and Mozmill so we have a sane checkin.

Comment 7

[Jeff Hammel]

Comment on attachment 646073 [details]
Patch

+            # We don't have to call report_disconnect here because
+            # start_runner() already cares about

...already cares about..what?

I would probably also change the function to not define an app_info variable and just have a return in the try: and in the except: but that's just style.

+            self.logger.fatal('Framwork Failure | ' + string)

Spelling

To be honest, I am scared of this change.  The code looks fine, I just worry that it will introduce new and unforseen complexities a la restart tests.  I am in full support of the premise: it would be nice to detect harness failures.  That said, for users of mozmill they should never see a harness failure (unless they're also doing something strange), so it is really only for mozmill developers.

I realize you will say that this isn't risky and that it is needed to detect harness failures.  I don't see anything wrong here nor can think of any particular failure paths, it just makes me nervous.  r+

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Jeff Hammel [:jhammel] from comment #7)
> +            # We don't have to call report_disconnect here because
> +            # start_runner() already cares about
> 
> ...already cares about..what?

The JSBridgeDisconnectError. I have updated the comment.

> +            self.logger.fatal('Framwork Failure | ' + string)
> 
> Spelling

Fixed.

> To be honest, I am scared of this change.  The code looks fine, I just worry
> that it will introduce new and unforseen complexities a la restart tests.  I
> am in full support of the premise: it would be nice to detect harness

Those changes should have no impact on restart tests. At least all the tests we have in our mutt folder are passing, except the known broken ones.

> failures.  That said, for users of mozmill they should never see a harness
> failure (unless they're also doing something strange), so it is really only
> for mozmill developers.

They can see if we did a poor job and a specific version of the application doesn't support what Mozmill wants to have. But yeah, mostly it's for developers.

> I realize you will say that this isn't risky and that it is needed to detect
> harness failures.  I don't see anything wrong here nor can think of any
> particular failure paths, it just makes me nervous.  r+

Well, a risk is everywhere but this will help us a lot in continuing to work on mozmill and if something should be wrong, which I can't think of right now, we can always revert. But lets wait for the opinion of the others.

Comment 9

[Andrew Halberstadt [:ahal]]

Comment on attachment 646073 [details]
Patch

It looks good. My one comment, is does the file _have_ to be mozmill.js? We have about 2-4 files called mozmill.js already.. can we call it console_observer.js or console.js or something else instead?

Also for the comment that you fixed, I think you meant "handles the exception" rather than "cares about"

I think this patch is ok to land.. it might cause problems, but it might also save problems.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrew Halberstadt [:ahal] from comment #9)
> It looks good. My one comment, is does the file _have_ to be mozmill.js? We
> have about 2-4 files called mozmill.js already.. can we call it
> console_observer.js or console.js or something else instead?

I would like to keep the name because I have more than that in mind. Once I get to all the window handling code we need a central place for it. Therefore mozmill.js should become a real component other code can use via its interface nsIMozmill. So we should better rename the other ones or move code around to have it in the right modules. But I agree, finally we should end-up with a single mozmill.js. 

> Also for the comment that you fixed, I think you meant "handles the
> exception" rather than "cares about"

Yeah. Fixed it now.

I will wait for Clint's feedback until tomorrow. Given that I'm away the next week I would like to land it in either way. I hope that's ok then.

Comment 11

[cmtalbert]

Comment on attachment 646073 [details]
Patch

(In reply to Henrik Skupin (:whimboo) [away 07/27 - 08/05] from comment #0)
> See bug 742766 where I have seen that in some cases when Mozmill modules are
> broken due to syntax errors or a wrong usage of Cc/Ci we can't make a test
> to fail because code in those modules are not getting loaded and executed.
> The worst case is if frame.js is broken.
> 
> We should move at least the ConsoleService listener to a real component
> without any dependency on other modules. That way the chance that something
> is broken is minimal and we could ensure to really transmit all the
> information to the framework and mark tests as failed.
> 
> Also keep in mind that currently we do not shutdown the browser but letting
> it timeout. We should close it right away. Otherwise testruns with hundreds
> of tests will kill our time schedule.

If we have problems in the framework such that things won't run, then NOTHING will run. Regardless of what file this is in.  This really feels like a problem in search of a solution. 

I don't know why we are expending time and energy on this, especially when we have already identified what the actual need list is for mozmill 2.0 and this is not on it.

The only time you should have a problem in the framework is when you're developing for the framework.  And that is just a cost of being a framework developer. Testers don't hit these kinds of issues because testers shouldn't be using in development frameworks.

I really question why we are expending the energy to solve this problem here. There are actionable, real problems that are required to get mozmill tests unblocked, and this is not one of those.  So, what is this buying us in terms of either A) making it easier to write tests for end users or B) turning on the set of agreed on tests for Q3?

While on the surface there may be nothing wrong with this change, on the other hand, we're moving code *out* of the driver module which is written to be an entirely re-usable UI automation driver utility for code that wants to rely on this. And I think that this change invalidates that architecture. Also, who knows what side effects this change will cause and since this change has very little benefit, I simply think the cost is too high.

Feedback -.

If jhammel and ahal think this is something we need, I can be convinced otherwise, but as this patch stands, I have a hard time believing this is something we want.

Comment 12

[Andrew Halberstadt [:ahal]]

(In reply to Clint Talbert ( :ctalbert ) from comment #11)
> While on the surface there may be nothing wrong with this change, on the
> other hand, we're moving code *out* of the driver module which is written to
> be an entirely re-usable UI automation driver utility for code that wants to
> rely on this. And I think that this change invalidates that architecture.

I thought about this after I r+'ed the review. Then I remembered this changeset: https://github.com/mozilla/mozmill/commit/ee49208547b6eedefe7bd4645f68b430ef44f92e and realized that architecture has already been invalidated. Given this I would prefer if the console listener remained inside mozmill.js, though if I had to I could always write a new console listener for peptest.

If debugging is really that much of a pain we could always just put debug statements that get enabled with an environment variable throughout JSBridge.

Comment 13

[Jeff Hammel]

FWIW, I agree with Clint and think this is a bit overkill for mozmill.  There are ways of debugging if the harness is broken currently: I wouldn't say its fun, but its doable.  Trying to determine if a harness is broken within a harness seems mostly a bad pattern to me, at least in this instance.  If the new mozmill.js is also broken, what will detect that? Etc.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Now that I'm back from vacation I finally be able to respond to the last comments and give my feedback why I still think it's a good idea to move the code out into a real component. There are several reasons for more or less important for each of us. But everyone should decide on their own.

1. Currently the mozmill.js module is loaded at an unknown time which is somewhat after profile-after-change. There is no definitive definition for it. That means we could miss important notifications, especially when I would like to finish up all the window handling code. I really would like to register all observer notifications and listeners as soon as possible to make the code simpler.

2. I think that this low-level code should better live in a real component compared to its current location in a module. That would allow other code or even extensions to access it easier by querying for its service via Cc/Ci.

3. With this component we will have to make sure to only import the bare minimum amount of necessary modules to lower to risk of breakage. For possible situations see below. All other modules including resource/driver/mozmill.js has a huge amount of dependencies, which increases the risk of breakage the low-level code of Mozmill. That means the component should only be used for event handling code.

4. Breakage can not only happen due to failures of developers. There is more than that! Keep in mind that a change in Firefox makes Mozmill incompatible. Having a component which less dependencies would give us a better change to report regressions. Same if some failure only happen on special platforms which we do not have in our test matrix yet but are tested by our community. Additionally keep locales in mind. Those could also be a factor in introducing breakage.

5. With this change we will be able to include the failure in the report which gets send to our dashboard. It would make it a lot easier for us to already start debugging at the report level.

6. We would have a more specific error message as 'application disconnected' in situations when the extension is broken.

7. Right now we dump the message to the console but at least on OS X we don't shutdown Firefox right away but hard-kill it after 60s. This is annoying because Ctrl+C also doens't help here. It exists the CLI but leaves the Firefox process open, which then has to be force killed via the activity manager. With my change we directly quit Firefox.

8. While we are good in debugging or have the capability in learning stuff quite fast, we can't say that for everyone in our community. If we want to be successful in getting people to contribute to Mozmill we have to make it easier for them. Saving time for anyone of us is a big plus.

(In reply to Clint Talbert ( :ctalbert ) from comment #11)
> If we have problems in the framework such that things won't run, then
> NOTHING will run. Regardless of what file this is in.  This really feels
> like a problem in search of a solution. 

That's true when we leave the code where it is right now. Given all the dependencies Mozmill will be totally broken. But having the code in its own component loaded by Firefox itself, we can make sure to at least react as best as possible on such a situation. All that is for the JS side and not Python! The latter one we definitely can't cover.

> The only time you should have a problem in the framework is when you're
> developing for the framework.  And that is just a cost of being a framework
> developer. Testers don't hit these kinds of issues because testers shouldn't
> be using in development frameworks.

See above why this is not fully true. There are more possible failure conditions.

> While on the surface there may be nothing wrong with this change, on the
> other hand, we're moving code *out* of the driver module which is written to
> be an entirely re-usable UI automation driver utility for code that wants to
> rely on this. And I think that this change invalidates that architecture.
> Also, who knows what side effects this change will cause and since this
> change has very little benefit, I simply think the cost is too high.

Extensions which want to make use of it simply have to add the components/mozmill.js file to its content and declare the component in the manifest. Nothing more has to be done on that side. I don't see why this change invalidates the architecture. There is simply one more file to take care of.

If you still disagree with this change we can still move the code back into the original mozmill.js module. This would stop us from reporting failures in mozmill.js but leaves all other stuff in-tact.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Clint, given that you had concerns it would be great if you could follow-up first. But I also would like to get feedback again from Andrew and Jeff regarding my last comment. Thanks.

Comment 16

[cmtalbert]

Comment on attachment 646073 [details]
Patch

All right, so I've re-reviewed the code. My biggest problem with this change is that it isn't on the list for 2.0 and we've all wasted quite a bit of time on it. I feel like we have so little time here and we need 2.0 done about last month that it is crucial to not waste time.  I also don't think that capturing mozmill framework failures is nearly as important as Henrik thinks it is, but we can agree to disagree.

That said, it is a good patch. I echo Ahal's comment that the file in components should not be called mozmill.js. I'd much prefer it called mzmlConsoleObserver.js or something like that.mozmill.js doesn't tell anyone what it does or why they'd want it. 

Most of the patch simply protects the code from JSBridge losing the connection to the application which is good. The biggest and most annoying change is moving the console listener out of the original mozmill.js which makes the driver API something you now have to cherry-pick to get all of. I think that can be solved with packaging and/or documentation though.

I do like that by moving the window observers and console listeners to the file in components our listeners will be instantiated at the earliest possible moment. I think that will be a good change, though the window listeners haven't yet been hooked up in this patch (on-session-restore does nothing at the moment) are not being used in this patch at all. I can understand that we'll build on that, and having those observers in place that early should allow us to make mozmill's startup and shutdown a little more deterministic (I hope).

So, let's go ahead and land this, f+, r+.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Clint Talbert ( :ctalbert ) from comment #16)
> That said, it is a good patch. I echo Ahal's comment that the file in
> components should not be called mozmill.js. I'd much prefer it called
> mzmlConsoleObserver.js or something like that.mozmill.js doesn't tell anyone
> what it does or why they'd want it. 

I have made some thoughts and given that this file will only contain observer notification callbacks and event listener code in the future, we should probably call it handlers.js.

> Most of the patch simply protects the code from JSBridge losing the
> connection to the application which is good. The biggest and most annoying
> change is moving the console listener out of the original mozmill.js which
> makes the driver API something you now have to cherry-pick to get all of. I
> think that can be solved with packaging and/or documentation though.

Is it something we can do on a follow-up bug? I like the packaging idea which we could implement via a new Ant target.

An updated patch with the final code will come up soon.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Comment on attachment 646073 [details]
Patch

Re-requesting review from Ahal and Jeff given Clint's feedback and my updates on the patch.

Changes:
* Moved mozmill.js component to handlers.js (class: mozmill-handlers)
* Removed unnecessary imports for mozmill.js for initialization in overlaying code
* Removed accidentally added observer notification for window session restore (will be added with bug 753763)

Comment 19

[Andrew Halberstadt [:ahal]]

Comment on attachment 646073 [details]
Patch

Personally I find these pull requests very confusing, especially when dealing with incremental reviews. I'm never sure how much has changed or has been added since the last time I reviewed. I also think it makes it difficult for others to try out the patches before they get committed (as now you need to add whimboo/mozmill.git as a remote repo and figure out what commits you're interested in instead of simply applying a patch).

If you _really_ don't want to upload patches to bugzilla, could you at least provide a compare URL, so I can look at exactly what I'm supposed to be reviewing? It will also help others following the bug to see exactly what just got reviewed.
E.g: https://github.com/whimboo/mozmill/compare/e4fc25d...d2b2fba

That being said, assuming the above url is what I was supposed to review, r+.

Comment 20

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Andrew Halberstadt [:ahal] from comment #19)
> If you _really_ don't want to upload patches to bugzilla, could you at least
> provide a compare URL, so I can look at exactly what I'm supposed to be
> reviewing? It will also help others following the bug to see exactly what
> just got reviewed.
> E.g: https://github.com/whimboo/mozmill/compare/e4fc25d...d2b2fba

Haven't known about this feature yet. Sounds good. Will make sure to do it for complex patches.

> That being said, assuming the above url is what I was supposed to review, r+.


Yes it is. Thank you both. So I will get it landed and file the follow-up bug for the Ant script.

Comment 21

[Henrik Skupin [:whimboo][⌚️UTC+1]]

https://github.com/mozilla/mozmill/commit/2c8688a12ff842c4f8c32a7b9205b84ae38383d0