1296562 - Revert order for crash and socket checks due to confusing Treeherder messages

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect)

Description

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

As you can see for the following job the way how we report the crash and disconnect of Firefox is confusing and might cause that sheriffs will file a wrong bug - for the disconnect instead for the crash:

https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&filter-searchStr=marionette&filter-tier=1&filter-tier=2&filter-tier=3&selectedJob=4677142

> TEST-UNEXPECTED-ERROR | test_about_pages.py TestAboutPages.test_back_forward | IOError: Process killed because the connection to Marionette server is lost. Check gecko.log for errors (Reason: Connection timed out after 360s)
> PROCESS-CRASH | test_about_pages.py TestAboutPages.test_back_forward | application crashed [@ mozalloc_abort(char const*)]

This should be the other way around.

Comment 1

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

Attachment: Bug 1296562 - Revert order for crash and socket checks in do_process_check decorator.

Review commit: https://reviewboard.mozilla.org/r/72846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/72846/

Comment 2

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8782799 [details]
Bug 1296562 - Revert order for crash and socket checks in do_process_check decorator.

https://reviewboard.mozilla.org/r/72846/#review70670

r+wc.

::: testing/marionette/client/marionette_driver/decorators.py:49
(Diff revision 1)
>              exc, val, tb = sys.exc_info()
>  
> +            # In case of no Marionette failures ensure to check for possible crashes.
> +            # Do it before checking for port disconnects, to avoid reporting of unrelated
> +            # crashes due to a forced shutdown of the application.
> +            if not isinstance(e, MarionetteException) or type(e) is MarionetteException:

Don't you want `type(e) is not MarionetteException`?

Comment 3

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

Comment on attachment 8782799 [details]
Bug 1296562 - Revert order for crash and socket checks in do_process_check decorator.

https://reviewboard.mozilla.org/r/72846/#review70670

> Don't you want `type(e) is not MarionetteException`?

No, it would actually change the current behavior. We only want to do a crash check if there is a build-in exception, or a base Marionette exception. With your proposed change we would also allow all subclasses of MarionetteException, which is not what we want.

Also changing this line looks to be out of scope for this bug, which is just to correct the order in how we do crash vs. socket checks.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/293fceb6b488
Revert order for crash and socket checks in do_process_check decorator. r=maja_zf

Comment 5

[Maja Frydrychowicz :impossibus (was :maja_zf) (needinfo me)]

Comment on attachment 8782799 [details]
Bug 1296562 - Revert order for crash and socket checks in do_process_check decorator.

https://reviewboard.mozilla.org/r/72846/#review70670

> No, it would actually change the current behavior. We only want to do a crash check if there is a build-in exception, or a base Marionette exception. With your proposed change we would also allow all subclasses of MarionetteException, which is not what we want.
> 
> Also changing this line looks to be out of scope for this bug, which is just to correct the order in how we do crash vs. socket checks.

My question was about adding a `not`, not replacing the whole line. I should have highlighted lines 46-49 in MozReview. 

Your patch adds a comment that states "In case of no Marionette failures...", but the current conditional allows MarionetteException, which is technically a "Marionette failure", so the added comment is not accurate.

Comment 6

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

Oh, so you were referring to the comment. That was not clear to me. If I have to fix we can do it in a follow-up bug. Otherwise I will wait until I'm at this code the next time.

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/293fceb6b488

Comment 8

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

I haven't seen issues with this patch landed so far. Given that we also have lots of crashes on aurora I would suggest to also get it backported to that branch. I don't think we need it for beta.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4008796e979a