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

RESOLVED FIXED in Firefox 53

Status

P1
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: erahm, Assigned: whimboo)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox53 fixed, firefox54 fixed, firefox55 fixed)

Details

(Whiteboard: [spec][17/03])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

2 years ago
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.
(Reporter)

Comment 1

2 years ago
Created attachment 8786949 [details] [diff] [review]
Switch back to main window during fxui tests
Attachment #8786949 - Flags: review?(hskupin)
(Assignee)

Comment 2

2 years ago
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?
(Assignee)

Updated

2 years ago
Flags: needinfo?(erahm)
(Reporter)

Comment 3

2 years ago
(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.
Flags: needinfo?(erahm)
(Reporter)

Comment 4

2 years ago
Created attachment 8791817 [details] [diff] [review]
Switch back to main window during fxui tests

Avoid accessing a potentially dead window by switching back to the main window.
Attachment #8791817 - Flags: review?(hskupin)
(Reporter)

Updated

2 years ago
Attachment #8786949 - Attachment is obsolete: true
Attachment #8786949 - Flags: review?(hskupin)
(Reporter)

Comment 6

2 years ago
Henrik, can you take a look at this when you get a chance? I updated the patch with your suggested changes.
Flags: needinfo?(hskupin)
(Assignee)

Comment 7

2 years ago
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.
Flags: needinfo?(hskupin)
Attachment #8791817 - Flags: review?(hskupin)
(Reporter)

Comment 8

2 years ago
(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!).
(Assignee)

Comment 9

2 years ago
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.
Flags: needinfo?(erahm)
(Reporter)

Comment 10

2 years ago
(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
Flags: needinfo?(erahm)
(Reporter)

Comment 11

2 years ago
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.
(Reporter)

Comment 12

2 years ago
Henrik where do we stand on this?
Flags: needinfo?(hskupin)
(Assignee)

Comment 13

2 years ago
(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.
Flags: needinfo?(hskupin) → needinfo?(erahm)
(Reporter)

Comment 14

2 years ago
(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
Flags: needinfo?(erahm) → needinfo?(hskupin)
(Assignee)

Comment 15

2 years ago
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.
Flags: needinfo?(hskupin) → needinfo?(erahm)
(Reporter)

Comment 16

2 years ago
(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?
Flags: needinfo?(erahm) → needinfo?(hskupin)
(Assignee)

Comment 17

2 years ago
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.
Flags: needinfo?(hskupin)
(Assignee)

Comment 18

2 years ago
Maybe a ride-along commit for bug 1322383 fixes it. Lets wait until it has been landed.
Depends on: 1322383
(Assignee)

Comment 20

2 years ago
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
(Assignee)

Comment 21

2 years ago
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.
Assignee: erahm → hskupin
Component: Firefox UI Tests → Marionette
QA Contact: hskupin
Summary: Switch back to main window during fxui tests → "TypeError: can't access dead object" in assert.window
(Assignee)

Updated

2 years ago
Attachment #8791817 - Attachment is obsolete: true
Comment hidden (mozreview-request)

Comment 23

2 years ago
mozreview-review
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.
Attachment #8852145 - Flags: review?(ato) → review-
(Assignee)

Comment 24

2 years ago
mozreview-review-reply
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

2 years ago
mozreview-review-reply
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.
[mass] Setting priority
Priority: -- → P1
Comment hidden (mozreview-request)

Comment 28

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

https://reviewboard.mozilla.org/r/124356/#review128866
Attachment #8852145 - Flags: review?(ato) → review+
(Assignee)

Updated

2 years ago
Whiteboard: [spec][17/03]

Comment 29

2 years ago
Pushed by hskupin@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/5b2e75b0776e
Fix "TypeError: can't access dead object" in assert.window(). r=ato

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/5b2e75b0776e
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
(Assignee)

Comment 31

2 years ago
Please uplift this test-only patch to aurora and beta. For esr52 we have to wait for bug 1322383 being uplifted too. Thanks.
Whiteboard: [spec][17/03] → [spec][17/03][checkin-needed-aurora][checkin-needed-beta]

Comment 32

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/9375d84a5ae4
status-firefox54: --- → fixed
Whiteboard: [spec][17/03][checkin-needed-aurora][checkin-needed-beta] → [spec][17/03][checkin-needed-beta]

Comment 33

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/e081483e770d
status-firefox53: --- → fixed
Whiteboard: [spec][17/03][checkin-needed-beta] → [spec][17/03]
(Assignee)

Updated

2 years ago
status-firefox-esr52: --- → affected
(Assignee)

Comment 34

2 years ago
Not necessary for esr52 because the other required patches aren't getting uplifted to this branch.
status-firefox-esr52: affected → wontfix
You need to log in before you can comment on or make changes to this bug.