Closed Bug 1299626 Opened 8 years ago Closed 7 years ago

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

Categories

(Remote Protocol :: Marionette, defect, P1)

Product:
Remote Protocol
Component:
Marionette
Type:
defect

Tracking

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

Status:
RESOLVED FIXED
Milestone:
mozilla55
Tracking Flags:
Tracking Status
firefox-esr52 --- wontfix
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: erahm, Assigned: whimboo)

References

(Blocks 1 open bug)

Details

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

Attachments

(1 file, 2 obsolete files)

Bug 1299626 - Fix "TypeError: can't access dead object" in assert.window().
59 bytes, text/x-review-board-request
ato
: review+
Details 
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.

        Attachment #8786949 -
        Flags: review?(hskupin)

    
    

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

    
    

    
  


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

        Attachment #8791817 -
        Flags: review?(hskupin)

    
  

        Attachment #8786949 -
        Attachment is obsolete: true

        Attachment #8786949 -
        Flags: review?(hskupin)

    
    

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

    
    

    
  
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)

    
    

    
  
(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!).

    
    

    
  
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)

    
    

    
  
(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)

    
    

    
  
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.

    
    

    
  
Henrik where do we stand on this?
Flags: needinfo?(hskupin)

    
    

    
  
(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)

    
    

    
  
(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)

    
    

    
  
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)

    
    

    
  
(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)

    
    

    
  
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)

    
    

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

    
    

    
  
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

    
    

    
  
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

        Attachment #8791817 -
        Attachment is obsolete: true

    
    

    
      
  


  

    
    

    
  
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-

    
    

    
  
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 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 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+
Whiteboard: [spec][17/03]

    
    

    
  
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

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/5b2e75b0776e
Status: ASSIGNED → RESOLVED
Closed: 7 years ago

          status-firefox55:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55

    
    

    
  
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]

    
    

    
  
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]

    
    

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

          status-firefox53:
          --- → fixed
Whiteboard: [spec][17/03][checkin-needed-beta] → [spec][17/03]

    
    

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

          status-firefox-esr52:
          affected → wontfix

    
  
Product: Testing → Remote Protocol

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: