Closed Bug 1200975 Opened 9 years ago Closed 9 years ago

[mozrunner] mozrunner>6.7 don't show the crash window

Categories

(Testing :: Mozbase, defect)

defect
Not set
normal

Tracking

(firefox43 fixed)

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: parkouss, Assigned: parkouss)

References

Details

Attachments

(3 files, 1 obsolete file)

So this happen at least on linux 64.

To reproduce, take one nightly build from say 2011-06-03:

mozdownload -t daily --date 2011-06-03
or
http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2011/06/2011-06-03-03-mozilla-central/firefox-7.0a1.en-US.linux-x86_64.tar.bz2

untar it, then run:

> mozrunner --binary /path/to/bin/firefox

using latest 6.9, it just exit, nothing is shown. same thing with 6.8.

using mozrunner==6.7, a crash window is visible.

This looks like a regression.

Another thing to note, I am able to run:

> cd /path/to/bin/
> ./firefox

with success! Not sure if it is another bug.
Julien, can you please check 'pip freeze' for each mozrunner 6.7 and 6.8 installed? I wonder if it comes from another package, because between 6.7 and 6.8 I can only see the structured logging changes in mozrunner.
Attached file pipfreeze_6.7
Attached file pipfreeze_6.8
And running diff:

diff pipfreeze_6.8 pipfreeze_6.7
19c19
< mozrunner==6.8
---
> mozrunner==6.7
So, looks like it comes from http://hg.mozilla.org/mozilla-central/annotate/fb720c90eb49/testing/mozbase/mozrunner/mozrunner/base/browser.py#l30, ie:

http://hg.mozilla.org/mozilla-central/rev/69701baa4c9f

coming from bug 1154761.

problem is that it is hardcoded. but using mozregression or mozrunner for example, nothing is shown, not even a crash log message. So this is hard to understand what is going on.

I think we should only activate that if a flag is given, or a command line. Or to deactivate it, if you really want this to be the default.
Flags: needinfo?(ted)
Flags: needinfo?(james)
So the current behaviour is required to work well in automation, and to allow the use of a debugger. It's the responsibility of the caller to run mozcrash to look for any crash dump file that was created and log appropriately. It's hard for mozrunner to do that directly since it doesn't know about some parameters related to logging (e.g. it doesn't know about structured vs unstructured, or allow us to log which test was running at the time of the crash).
Flags: needinfo?(james)
Blocks: 1200987
Well, so what if I want to test that crash window from mozregression ? We need a way to at least activate it I think.
It's possible for mozregression to unset these variables if it's required for some specific use case.
Hm yes. But it have to know that

GNOME_DISABLE_CRASH_DIALOG
XRE_NO_WINDOWS_CRASH_DIALOG

are defined by mozreg. And if we add another variable (I don't know, for KDE or something) or if one variable is renamed then it will break again...
why not just add a disable_crash_window variable in __init__ that defaults to True ? I think I should then be able to pass it to False easily.
I think that would be fine.
Flags: needinfo?(ted)
Bug 1200975 - [mozrunner] mozrunner>6.7 don't show the crash window. r=whimboo
Attachment #8655929 - Flags: review?(hskupin)
So I tested it locally (using mozregression), works fine and do the job.
https://reviewboard.mozilla.org/r/18051/#review16155

::: testing/mozbase/mozrunner/mozrunner/base/browser.py:20
(Diff revision 1)
> +        self.crash_reporter_window = \

Please try to avoid using line continuation characters unless there is no alternative.
Comment on attachment 8655929 [details]
MozReview Request: Bug 1200975 - [mozrunner] mozrunner>6.7 don't show the crash window. r=whimboo

Bug 1200975 - [mozrunner] mozrunner>6.7 don't show the crash window. r=whimboo
Comment on attachment 8655929 [details]
MozReview Request: Bug 1200975 - [mozrunner] mozrunner>6.7 don't show the crash window. r=whimboo

https://reviewboard.mozilla.org/r/18051/#review16277

::: testing/mozbase/mozrunner/mozrunner/base/browser.py:23
(Diff revision 2)
> +        )

I would suggest we move this code some lines down to the environment variable handling. So it sticks together. This option shouldn't confuse the __init__ method of BaseRunner, right?

::: testing/mozbase/mozrunner/mozrunner/runners.py:28
(Diff revision 2)
> +        Defaults to False.

Maybe we name this option `show_crash_reporter`?
Attachment #8655929 - Flags: review?(hskupin)
https://reviewboard.mozilla.org/r/18051/#review16277

> I would suggest we move this code some lines down to the environment variable handling. So it sticks together. This option shouldn't confuse the __init__ method of BaseRunner, right?

Unfortunately we can't do that - the option WILL confuse BaseRunner.__init__:
https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/base/runner.py#29

> Maybe we name this option `show_crash_reporter`?

I'm not against it, why not. :)
https://reviewboard.mozilla.org/r/18051/#review16277

> Unfortunately we can't do that - the option WILL confuse BaseRunner.__init__:
> https://dxr.mozilla.org/mozilla-central/source/testing/mozbase/mozrunner/mozrunner/base/runner.py#29

I see. So lets keep it. Maybe have it all in a single line? I assume we don't have a line limit of 80 chars in mozbase.
Attached patch 1200975.patchSplinter Review
hm, had some issue with mozreview. Anyway, patch should be fixed. :)
Assignee: nobody → j.parkouss
Attachment #8655929 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8656486 - Flags: review?(hskupin)
Attachment #8656486 - Flags: review?(hskupin) → review+
Blocks: 1201511
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/39ed9e9defa7
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: