1299626 - "TypeError: can't access dead object" in assert.window

Status: RESOLVED FIXED

(Remote Protocol :: Marionette, defect, P1)

Description

[Eric Rahm [:erahm]]

This fixes fallout from bug 1276366. This avoid accessing a potentially dead window by switching back to the main window before running more commands against the active window.

Comment 1

[Eric Rahm [:erahm]]

Attachment: Switch back to main window during fxui tests

Comment 2

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

Comment on attachment 8786949 [details] [diff] [review]
Switch back to main window during fxui tests

Review of attachment 8786949 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/firefox-ui/tests/functional/private_browsing/test_about_private_browsing.py
@@ +21,5 @@
>          self.pb_url = support_url + 'private-browsing'
>  
> +    def tearDown(self):
> +        # Pop back over to the current window.
> +        self.marionette.switch_to_window(self.marionette.window_handles[0])

Shouldn't we better add this here:
https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py#108

I believe it's not only this specific test which can fail due to this.

::: testing/firefox-ui/tests/functional/security/test_ssl_status_after_restart.py
@@ +58,1 @@
>          self.restart()

Mind if we put this directly into the restart() method? What happens if a handle of a tab is currently selected?

Comment 3

[Eric Rahm [:erahm]]

(In reply to Henrik Skupin (:whimboo) from comment #2)
> Comment on attachment 8786949 [details] [diff] [review]
> Switch back to main window during fxui tests
> 
> Review of attachment 8786949 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> :::
> testing/firefox-ui/tests/functional/private_browsing/
> test_about_private_browsing.py
> @@ +21,5 @@
> >          self.pb_url = support_url + 'private-browsing'
> >  
> > +    def tearDown(self):
> > +        # Pop back over to the current window.
> > +        self.marionette.switch_to_window(self.marionette.window_handles[0])
> 
> Shouldn't we better add this here:
> https://dxr.mozilla.org/mozilla-central/source/testing/puppeteer/firefox/
> firefox_puppeteer/testcases/base.py#108
> 
> I believe it's not only this specific test which can fail due to this.

Makes sense, I'll move it.

> :::
> testing/firefox-ui/tests/functional/security/test_ssl_status_after_restart.py
> @@ +58,1 @@
> >          self.restart()
> 
> Mind if we put this directly into the restart() method? What happens if a
> handle of a tab is currently selected?

I kind of forget the rationale of this, just that it fixed a failure. I can move it into restart of course.

Comment 4

[Eric Rahm [:erahm]]

Attachment: Switch back to main window during fxui tests

Avoid accessing a potentially dead window by switching back to the main window.

Comment 5

[Eric Rahm [:erahm]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9117f25b00f1

Comment 6

[Eric Rahm [:erahm]]

Henrik, can you take a look at this when you get a chance? I updated the patch with your suggested changes.

Comment 7

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

Comment on attachment 8791817 [details] [diff] [review]
Switch back to main window during fxui tests

I would love to get mozreview patches, which are better to review and comment on. Maybe check this for the future.

>+++ b/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py
>@@ -66,16 +66,18 @@ class BaseFirefoxTestCase(unittest.TestC
>             self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]])
>             self.browser.tabbar.tabs[0].switch_to()
> 
>     def restart(self, **kwargs):
>         """Restart Firefox and re-initialize data.
> 
>         :param flags: Specific restart flags for Firefox
>         """
>+        # For clean-up make sure we work on a proper browser window
>+        self.browser.switch_to()

I tried to get some info out of bug 1276366 but I do not see why this is necessary. Would you be so kind and tell me a bit more why this is necessary? What in contrast should happen when Marionette clicks the restart button in the about dialog for applying an update? In such a case we also wouldn't switch to the browser window first.

>     def tearDown(self, *args, **kwargs):
>+        # Pop back over the the current window.
>+        self.marionette.switch_to_window(self.marionette.window_handles[0])

Some lines below we have a call to `_check_and_fix_leaked_handles()`. So this case isn't covered there yet? Sorry that I have to ask, but I do not understand the underlying issue yet as mentioned above.

I also would like to see a try build for this patch on Linux64 for all available fx-ui-functional tests. Thanks.

Comment 8

[Eric Rahm [:erahm]]

(In reply to Henrik Skupin (:whimboo) [away 09/30 - 10/06] from comment #7)
> Comment on attachment 8791817 [details] [diff] [review]
> Switch back to main window during fxui tests
> 
> I would love to get mozreview patches, which are better to review and
> comment on. Maybe check this for the future.

Sure I'll do that for any future patches.

> 
> >+++ b/testing/puppeteer/firefox/firefox_puppeteer/testcases/base.py
> >@@ -66,16 +66,18 @@ class BaseFirefoxTestCase(unittest.TestC
> >             self.browser.tabbar.close_all_tabs([self.browser.tabbar.tabs[0]])
> >             self.browser.tabbar.tabs[0].switch_to()
> > 
> >     def restart(self, **kwargs):
> >         """Restart Firefox and re-initialize data.
> > 
> >         :param flags: Specific restart flags for Firefox
> >         """
> >+        # For clean-up make sure we work on a proper browser window
> >+        self.browser.switch_to()
> 
> I tried to get some info out of bug 1276366 but I do not see why this is
> necessary. Would you be so kind and tell me a bit more why this is
> necessary? What in contrast should happen when Marionette clicks the restart
> button in the about dialog for applying an update? In such a case we also
> wouldn't switch to the browser window first.

I fixed a ton of tests elsewhere so I don't remember the specific details at this point. I've pushed https://treeherder.mozilla.org/#/jobs?repo=try&revision=14d16c8fae20 which includes the parent patch, but not the patch from this bug. That should highlight the failures.

> 
> >     def tearDown(self, *args, **kwargs):
> >+        # Pop back over the the current window.
> >+        self.marionette.switch_to_window(self.marionette.window_handles[0])
> 
> Some lines below we have a call to `_check_and_fix_leaked_handles()`. So
> this case isn't covered there yet? Sorry that I have to ask, but I do not
> understand the underlying issue yet as mentioned above.
> 
> I also would like to see a try build for this patch on Linux64 for all
> available fx-ui-functional tests. Thanks.

There's a try run in comment 6 (all green!).

Comment 9

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

Eric, to continue on this bug... what would be necessary for me to see those failures in a local build? I kinda would like to see how this patch works and specifically covers edge cases as mentioned in my last review comments. Thanks.

Comment 10

[Eric Rahm [:erahm]]

(In reply to Henrik Skupin (:whimboo) from comment #9)
> Eric, to continue on this bug... what would be necessary for me to see those
> failures in a local build? I kinda would like to see how this patch works
> and specifically covers edge cases as mentioned in my last review comments.
> Thanks.

Build locally with attachment 8757489 [details] [diff] [review] from bug 1276366. So something like:

> hg import https://bug1276366.bmoattachments.org/attachment.cgi?id=8757489
> ./mach build
> ./mach firefox-ui-functional

Comment 11

[Eric Rahm [:erahm]]

Sorry and if I haven't been clear, the best I can tell firefox-ui-functional is trying to run a script in the context of dead window, hence the failures. Switching to a non-dead window fixes the issue.

Comment 12

[Eric Rahm [:erahm]]

Henrik where do we stand on this?

Comment 13

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

(In reply to Eric Rahm [:erahm] from comment #10)
> > Eric, to continue on this bug... what would be necessary for me to see those
> > failures in a local build? I kinda would like to see how this patch works
> > and specifically covers edge cases as mentioned in my last review comments.
> > Thanks.
> 
> Build locally with attachment 8757489 [details] [diff] [review] from bug
> 1276366. So something like:
> 
> > hg import https://bug1276366.bmoattachments.org/attachment.cgi?id=8757489
> > ./mach build
> > ./mach firefox-ui-functional

Sorry for the late reply but I'm kinda busy at the moment. Given that this involves changing a CPP file, I cannot do my usual artifact builds. Would you mind pushing this to try so I can download a build for OS X? I can then test all this. Thanks.

Comment 14

[Eric Rahm [:erahm]]

(In reply to Henrik Skupin (:whimboo) from comment #13)
> Would you mind pushing this to try so I can download a build for OS X? I can then
> test all this. Thanks.

Sure, build is pending: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f39b0727f9e4a625a28e80602f0c6c6eba5fde2f

Comment 15

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

Comment on attachment 8791817 [details] [diff] [review]
Switch back to main window during fxui tests

Ok, I can see the problem now, and find it kinda helpful. Sorry Eric for such a long delay on this bug! I hope we can get it fixed soonish.

>     def restart(self, **kwargs):
>         """Restart Firefox and re-initialize data.
> 
>         :param flags: Specific restart flags for Firefox
>         """
>+        # For clean-up make sure we work on a proper browser window
>+        self.browser.switch_to()

I don't think that we ever would have to switch the window here. When we do a restart the window which is currently active should be kept. Also because the code has been changed meanwhile (you will get a merge conflict), and we allow a restart via clicking eg. ui elements. If we would switch the window in this line the restart won't work because Marionette won't be able to find the element.

>     def tearDown(self, *args, **kwargs):
>+        # Pop back over the the current window.
>+        self.marionette.switch_to_window(self.marionette.window_handles[0])
>+

Similar as mentioned above we also had lots of changes here. So a rebase might fail. Beside that I'm not sure what the best effort will be in tearDown because we have two situations:

1. The test is passing as usual. In such a case the test should make sure that all additional windows have been closed, and an existing browser has the focus.

2. The test fails before cleaning up the windows and tabs, or switching to a valid window. It means we end-up in teardown with an unknown list of windows and tabs. So we call `_check_and_fix_leaked_handles()`. But given that this code is run after the tearDown code of the test, we might be too late.

Thinking more about all that, I may have been wrong by rejecting your first version of the patch which simply added a line to the test itself to ensure that 1) is happening. For 2) we might have to refactor our tests to ensure that we close all additionally opened windows and tabs in the tearDown of the test.

Eric,if that is too much work for you please let me know and I can get this done.

Comment 16

[Eric Rahm [:erahm]]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> Eric,if that is too much work for you please let me know and I can get this
> done.

Yes, can you please update the patches as you see fit?

Comment 17

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

A lot has been changed lately for Marionette and also Puppeteer. So maybe this is not a problem anymore. To verify that I created new builds with the mentioned patch applied (slight update was necessary due to conflicts).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=151ffac6a2f4dbbc4eb86f04408895d4ec19e6da

I will try to get to it the next days.

Comment 18

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

Maybe a ride-along commit for bug 1322383 fixes it. Lets wait until it has been landed.

Comment 19

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

Now with bug 1322383 fixed, I created another try build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=920f47efb9580f182c7658b43ca272366d0f89b4

Comment 20

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

The last build was accidentally an artifact build. I pushed again to get a full build:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ce4eaac3e1cd186d9ed61abe749ac487ff3ee9e

Comment 21

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

So basically the underlying issue is no longer part of any of our Firefox UI tests. With the patches on bug 1322383 it got all fixed! 

But there is still one issue remaining which lays in Marionette, specifically in assert.js' `window()` method. With the above patch applied it can still raise:

> TEST-UNEXPECTED-ERROR | test_windows.py TestBaseWindow.test_open_close | MarionetteException: TypeError: can't access dead object
>
> stacktrace:
>	assert.window/<@chrome://marionette/content/assert.js:138:7
>	assert.that/<@chrome://marionette/content/assert.js:345:10
>	assert.window@chrome://marionette/content/assert.js:136:10
>	GeckoDriver.prototype.getChromeWindowHandle@chrome://marionette/content/driver.js:1228:3

In this case it's for the `getChromeWindowHandle` command.

The fix here is to not further propagate the failure but let `assert.window()` return false, because the underlying window is no longer existent.

Comment 22

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

Attachment: Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().

The assertion method is used to check for a valid window. But it can fail
itself when already accessing the ChromeWindow. To fix that we have to
catch a possible TypeError and let the method throw a NoSuchWindowError
instead.

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

Comment 23

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8852145 [details]
Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().

https://reviewboard.mozilla.org/r/124356/#review126912

::: testing/marionette/assert.js:134
(Diff revision 1)
>  assert.window = function (win, msg = "") {
>    msg = msg || "Unable to locate window";
> -  return assert.that(w => w && w.document.defaultView, msg, NoSuchWindowError)(win);
> +  return assert.that(w => {
> +    try {
> +      return w && w.document.defaultView;
> +
> +    // If the window is no longer available a TypeError is thrown.
> +    } catch (e if e.name === "TypeError") {
> +      return false;
> +    }
> +  }, msg, NoSuchWindowError)(win);
>  }

Please add a test in testing/marionette/test_assert.js.  I believe we are missing a test for the original behaviour in assert.window too, so it would be good to add that while you’re at it.

Comment 24

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

Comment on attachment 8852145 [details]
Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().

https://reviewboard.mozilla.org/r/124356/#review126912

> Please add a test in testing/marionette/test_assert.js.  I believe we are missing a test for the original behaviour in assert.window too, so it would be good to add that while you’re at it.

We cannot test this right now because the underlying code which triggers this TypeError isn't active yet. First all broken code has to be fixed. See Eric's first comment on this bug.

Comment 25

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8852145 [details]
Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().

https://reviewboard.mozilla.org/r/124356/#review126912

> We cannot test this right now because the underlying code which triggers this TypeError isn't active yet. First all broken code has to be fixed. See Eric's first comment on this bug.

I suggest you write a mock-style test where you pass an object that throws TypeError, and a second test where you pass an object which has document.defaultView defined.

If you look at some of the other tests in testing/marionette/test_assert.js, this is the pattern we’re using to ensure we don’t regress behaviour accidentally.

Comment 26

[David Burns :automatedtester]

[mass] Setting priority

Comment 27

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

Comment on attachment 8852145 [details]
Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().

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

Comment 28

[Andreas Tolfsen ❲:ato❳]

Comment on attachment 8852145 [details]
Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().

https://reviewboard.mozilla.org/r/124356/#review128866

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b2e75b0776e
Fix "TypeError: can't access dead object" in assert.window(). r=ato

Comment 30

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5b2e75b0776e

Comment 31

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

Please uplift this test-only patch to aurora and beta. For esr52 we have to wait for bug 1322383 being uplifted too. Thanks.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/9375d84a5ae4

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e081483e770d

Comment 34

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

Not necessary for esr52 because the other required patches aren't getting uplifted to this branch.