1478094 - Update crash unit tests to make use of about:crashparent and about:crashcontent

Reporter

Description

•

6 years ago

Bug 1462702 brought us two hidden about pages which allow us to crash Firefox. We should update our crash unit tests of Marionette for using the about:crashparent and about:crashcontent pages.

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

Reporter

Comment 1

•

6 years ago

The test file which needs to be changed here is:
https://dxr.mozilla.org/mozilla-central/source/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py

And here specifically the `crash()` method of the `BaseCrashTestCase` class. Instead of using `execute_script` with some ctypes code, we can simply use `navigate()` with the above mentioned about pages. The navigation always has to happen in the content context, but depending on the `chrome` argument (which I would rename to `parent`) the appropriate about page has to be loaded.

Mentor: hskupin

User Story: (updated)

Priority: P2 → P3

Whiteboard: [lang=py]

Comment hidden (mozreview-request)

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

Venkatesh (:vpit3833)

Assignee

Updated

•

6 years ago

Attachment #8995182 - Flags: review?(hskupin)

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

Reporter

Comment 3

•

6 years ago

mozreview-review

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review266736

Thank you for the patch. Please see my opened issues for items which need to be addressed. Also ensure to run the test file.

::: commit-message-7ba07:1
(Diff revision 1)
> +Bug 1478094 - Update crash unit tests to make use of about:crashparent and about:crashcontent

Please add `[marionette]` after the dash so it's better visible for which component this patch is.

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:89
(Diff revision 1)
>  
>          super(BaseCrashTestCase, self).tearDown()
>  
>      def crash(self, chrome=True):
>          context = 'chrome' if chrome else 'content'
>          sandbox = None if chrome else 'system'

This is not needed anymore. Please remove all dead code.

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:94
(Diff revision 1)
>          sandbox = None if chrome else 'system'
>  
>          socket_timeout = self.marionette.client.socket_timeout
>          self.marionette.client.socket_timeout = self.socket_timeout
>  
>          self.marionette.set_context(context)

Please note that with navigation we always have to execute it in content context (as mentioned in my previous comment). As you still set the context here, the tests all fail for chrome context. Did you actually run the test_crash.py file?

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:96
(Diff revision 1)
>          socket_timeout = self.marionette.client.socket_timeout
>          self.marionette.client.socket_timeout = self.socket_timeout
>  
>          self.marionette.set_context(context)
>          try:
> -            self.marionette.execute_script("""
> +            self.marionette.navigate("about:crashparent" if chrome else "about:crashcontent")

I would prefer something like:

> self.marionette.navigate("about:crash{}".format(process))

Whereby process is "parent" or "content" based on the function argument, which I also would like to see renamed from `chrome` to `parent`.

Attachment #8995182 - Flags: review?(hskupin) → review-

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

Reporter

Updated

•

6 years ago

Assignee: hskupin → venkateshpitta

Venkatesh (:vpit3833)

Assignee

Comment 4

•

6 years ago

I submitted for review too soon.  Tests fail, and from my understanding of the fail/error messages, all the users of the crash method expect a return value that is not in (None, 0), as well as an IOError be raised with a message as expected by the user.  I know this is not possible, at least with Python, and seem stuck here without knowing how to move forward.

Please provide some pointers.  Is the script passed to execute_script truly returning something not in (None, 0) as well as raising IOError?  How to get this done with Python?

Flags: needinfo?(hskupin)

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

Reporter

Comment 5

•

6 years ago

Can you please give some more details of the failures? I cannot help if nothing is provided. Please show the log entries from one of those tests which fail for you. Note that when an application crashes its exit code should never be 0, if it's None it would mean the process is still alive.

Flags: needinfo?(hskupin)

Comment hidden (mozreview-request)

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259674/diff/1-2/

Venkatesh (:vpit3833)

Assignee

Comment 7

•

6 years ago

mozreview-review-reply

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review266736

> Please note that with navigation we always have to execute it in content context (as mentioned in my previous comment). As you still set the context here, the tests all fail for chrome context. Did you actually run the test_crash.py file?

This patch passes the tests.

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

Reporter

Comment 8

•

6 years ago

mozreview-review

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review267700

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:88
(Diff revision 2)
>          self.marionette.crashed = self.crash_count
>  
>          super(BaseCrashTestCase, self).tearDown()
>  
>      def crash(self, chrome=True):
> -        context = 'chrome' if chrome else 'content'
> +        process = 'parent' if chrome else 'content'

Please rename the argument of the method to directly accept `parent` as bool.

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:93
(Diff revision 2)
> -        sandbox = None if chrome else 'system'
>  
>          socket_timeout = self.marionette.client.socket_timeout
>          self.marionette.client.socket_timeout = self.socket_timeout
>  
> -        self.marionette.set_context(context)
> +        self.marionette.using_context(process)

This should not work and fail. First `using_context` is a context manager and has to be used with `with`. You can check other tests in this folder for an example.

Second a navigation is only allowed for `content` context, and for nothing else. Also `parent` is not a valid context.

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:95
(Diff revision 2)
>          socket_timeout = self.marionette.client.socket_timeout
>          self.marionette.client.socket_timeout = self.socket_timeout
>  
> -        self.marionette.set_context(context)
> +        self.marionette.using_context(process)
>          try:
> -            self.marionette.execute_script("""
> +            self.marionette.navigate("about:crash{}".format(process))

This is the right thing to do.

Attachment #8995182 - Flags: review?(hskupin) → review-

Venkatesh (:vpit3833)

Assignee

Comment 9

•

6 years ago

mozreview-review-reply

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review267700

> This should not work and fail. First `using_context` is a context manager and has to be used with `with`. You can check other tests in this folder for an example.
> 
> Second a navigation is only allowed for `content` context, and for nothing else. Also `parent` is not a valid context.

Does not fail on my PC.  Using 'with' causes error (on my PC, as well).

For example,

        try:
            with self.marionette.using_context("content"):
                self.marionette.navigate("about:crash{}".format(process))
        finally:
            self.marionette.client.socket_timeout = socket_timeout

causes error in these three cases.  What am I missing?

Unexpected Results
------------------
ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrash.test_crash_chrome_process - MarionetteException: Please start a session
Traceback (most recent call last):
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
    testMethod()
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 105, in test_crash_chrome_process
    self.crash, parent=True)
  File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
    callable_obj(*args, **kwargs)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
    self.marionette.navigate("about:crash{}".format(process))
  File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
    self.set_context(scope)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
    {"value": context})
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
    return func(*args, **kwargs)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
    raise errors.MarionetteException("Please start a session")
ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInSetUp.test_crash_in_setup - MarionetteException: Please start a session
Traceback (most recent call last):
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 140, in run
    self.setUp()
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 158, in setUp
    self.crash, parent=True)
  File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
    callable_obj(*args, **kwargs)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
    self.marionette.navigate("about:crash{}".format(process))
  File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
    self.set_context(scope)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
    {"value": context})
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
    return func(*args, **kwargs)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
    raise errors.MarionetteException("Please start a session")
ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInTearDown.test_crash_in_teardown - MarionetteException: Please start a session
Traceback (most recent call last):
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 190, in run
    self.tearDown()
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 176, in tearDown
    self.crash, parent=True)
  File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
    callable_obj(*args, **kwargs)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
    self.marionette.navigate("about:crash{}".format(process))
  File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
    self.set_context(scope)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
    {"value": context})
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
    return func(*args, **kwargs)
  File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
    raise errors.MarionetteException("Please start a session")

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

Reporter

Comment 10

•

6 years ago

mozreview-review-reply

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review267700

> Does not fail on my PC.  Using 'with' causes error (on my PC, as well).
> 
> For example,
> 
>         try:
>             with self.marionette.using_context("content"):
>                 self.marionette.navigate("about:crash{}".format(process))
>         finally:
>             self.marionette.client.socket_timeout = socket_timeout
> 
> causes error in these three cases.  What am I missing?
> 
> Unexpected Results
> ------------------
> ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrash.test_crash_chrome_process - MarionetteException: Please start a session
> Traceback (most recent call last):
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 159, in run
>     testMethod()
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 105, in test_crash_chrome_process
>     self.crash, parent=True)
>   File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
>     callable_obj(*args, **kwargs)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
>     self.marionette.navigate("about:crash{}".format(process))
>   File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
>     self.gen.throw(type, value, traceback)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
>     self.set_context(scope)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
>     {"value": context})
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
>     return func(*args, **kwargs)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
>     raise errors.MarionetteException("Please start a session")
> ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInSetUp.test_crash_in_setup - MarionetteException: Please start a session
> Traceback (most recent call last):
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 140, in run
>     self.setUp()
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 158, in setUp
>     self.crash, parent=True)
>   File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
>     callable_obj(*args, **kwargs)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
>     self.marionette.navigate("about:crash{}".format(process))
>   File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
>     self.gen.throw(type, value, traceback)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
>     self.set_context(scope)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
>     {"value": context})
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
>     return func(*args, **kwargs)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
>     raise errors.MarionetteException("Please start a session")
> ERROR testing/marionette/harness/marionette_harness/tests/unit/test_crash.py TestCrashInTearDown.test_crash_in_teardown - MarionetteException: Please start a session
> Traceback (most recent call last):
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/marionette_test/testcases.py", line 190, in run
>     self.tearDown()
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 176, in tearDown
>     self.crash, parent=True)
>   File "/usr/lib/python2.7/unittest/case.py", line 994, in assertRaisesRegexp
>     callable_obj(*args, **kwargs)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/harness/marionette_harness/tests/unit/test_crash.py", line 96, in crash
>     self.marionette.navigate("about:crash{}".format(process))
>   File "/usr/lib/python2.7/contextlib.py", line 35, in __exit__
>     self.gen.throw(type, value, traceback)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1454, in using_context
>     self.set_context(scope)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 1431, in set_context
>     {"value": context})
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/decorators.py", line 26, in _
>     return func(*args, **kwargs)
>   File "/home/venkatesh/src/mozilla-central/testing/marionette/client/marionette_driver/marionette.py", line 750, in _send_message
>     raise errors.MarionetteException("Please start a session")

Oh, that reminds me to a problem we still have and that's why it was changed from `using_context()` to `set_context()`. The reason why it's failing is that the context manager (with) will try to reset the context when its leaving the code block. But at this time the crash happened and there is no active session anymore.

So please don't use `using_context()` but leave it as `set_context()`. Then it will be fine.

Comment hidden (mozreview-request)

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259674/diff/2-3/

Venkatesh (:vpit3833)

Assignee

Comment 12

•

6 years ago

mozreview-review-reply

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review267700

> Oh, that reminds me to a problem we still have and that's why it was changed from `using_context()` to `set_context()`. The reason why it's failing is that the context manager (with) will try to reset the context when its leaving the code block. But at this time the crash happened and there is no active session anymore.
> 
> So please don't use `using_context()` but leave it as `set_context()`. Then it will be fine.

Good to know.  I just submitted a review.  Shall I add a note in the crash method explaining this to the future?

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

Reporter

Comment 13

•

6 years ago

mozreview-review-reply

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review267700

> Good to know.  I just submitted a review.  Shall I add a note in the crash method explaining this to the future?

No, you can leave it out. Hopefully not that far in the future I will be able to finish bug 1433873 which will fix it.

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

Reporter

Comment 14

•

6 years ago

mozreview-review

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review268014

This looks good to me. Lets just fix the one minor nit.

Meanwhile I will push a try build to verify everything is working as expected.

Thanks!

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:88
(Diff revision 3)
>          self.marionette.crashed = self.crash_count
>  
>          super(BaseCrashTestCase, self).tearDown()
>  
> -    def crash(self, chrome=True):
> -        context = 'chrome' if chrome else 'content'
> +    def crash(self, parent=True):
> +        process = 'parent' if parent else 'content'

Lets get rid of this extra variable and directly use it in the `format()` call.

Attachment #8995182 - Flags: review?(hskupin) → review+

Comment hidden (mozreview-request)

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259674/diff/3-4/

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

Reporter

Comment 16

•

6 years ago

mozreview-review

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review268190

::: testing/marionette/harness/marionette_harness/tests/unit/test_crash.py:94
(Diff revisions 3 - 4)
>          self.marionette.client.socket_timeout = self.socket_timeout
>  
>          self.marionette.set_context("content")
>          try:
> -            self.marionette.navigate("about:crash{}".format(process))
> +            self.marionette.navigate("about:crash{}".
> +                                     format("parent" if parent else "content"))

I think the linter might not like it. Can you better format it like the following?

> self.marionette.navigate(
>     "about:crash{}".format("parent" if parent else "content")) 

This will also be much better readable.

Thanks.

Comment hidden (mozreview-request)

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259674/diff/4-5/

Venkatesh (:vpit3833)

Assignee

Comment 18

•

6 years ago

mozreview-review-reply

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

https://reviewboard.mozilla.org/r/259674/#review268190

> I think the linter might not like it. Can you better format it like the following?
> 
> > self.marionette.navigate(
> >     "about:crash{}".format("parent" if parent else "content")) 
> 
> This will also be much better readable.
> 
> Thanks.

Oh, looked different on BugZilla.  I broke the line because it is longer than 80 characters.  I am pasting this code shown by you verbatim and submitting new review.

Comment hidden (mozreview-request)

Comment on attachment 8995182 [details]
Bug 1478094 - [marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/259674/diff/5-6/

Pulsebot

Comment 20

•

6 years ago

Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/864df2345f9b
[marionette] Update crash unit tests to make use of about:crashparent and about:crashcontent r=whimboo

Daniel Varga [:dvarga]

Comment 21

•

6 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/864df2345f9b

Status: ASSIGNED → RESOLVED

Closed: 6 years ago

status-firefox63: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla63

BMO Automation

Updated

•

1 year ago

Product: Testing → Remote Protocol