Closed Bug 1296562 Opened 5 years ago Closed 5 years ago

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

Categories

(Testing :: Marionette, defect)

Version 3
defect
Not set
normal

Tracking

(firefox50 fixed, firefox51 fixed)

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- fixed
firefox51 --- fixed

People

(Reporter: whimboo, Assigned: whimboo)

Details

Attachments

(1 file)

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 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`?
Attachment #8782799 - Flags: review?(mjzffr) → review+
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.
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/293fceb6b488
Revert order for crash and socket checks in do_process_check decorator. r=maja_zf
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.
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.
https://hg.mozilla.org/mozilla-central/rev/293fceb6b488
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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.
Whiteboard: [checkin-needed-aurora]
You need to log in before you can comment on or make changes to this bug.