751242 - Some tests fail with "can't access dead object" since patch on bug 695480 has landed

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect)

Description

[Maniac Vlad Florin (:vladmaniac)]

Version: 
Firefox 15.0a1 (15.0a1, en-US, 20120502030505)

Mozmill: 
1.5.9 

Error: 
handleWindow@resource://mozmill/stdlib/securable-module.js -> file:///tmp/tmpDtvfJC.mozmill-tests/lib/utils.js:412 @resource://mozmill/modules/frame.js -> file:///tmp/tmpDtvfJC.mozmill-tests/tests/functional/testPrivateBrowsing/testDisabledElements.js:38 @resource://mozmill/modules/frame.js:557 @resource://mozmill/modules/frame.js:626 @resource://mozmill/modules/frame.js:669 @resource://mozmill/modules/frame.js:506 @resource://mozmill/modules/frame.js:681 @resource://jsbridge/modules/server.js:179 @resource://jsbridge/modules/server.js:183 @resource://jsbridge/modules/server.js:283 

Tests affected:
http://hg.mozilla.org/qa/mozmill-tests/file/4d11b76a73ae/tests/functional/testPrivateBrowsing/testDisabledElements.js
http://hg.mozilla.org/qa/mozmill-tests/file/4d11b76a73ae/tests/functional/testPrivateBrowsing/testDownloadManagerClosed.js

I suspect this has something to do with bug 695480

There are no local investigations at the moment, but I am on it as we speak.

Comment 1

[Maniac Vlad Florin (:vladmaniac)]

The failure is reproducible locally

Comment 2

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

Please also check other platforms where even other tests could fail with the same failure message.

Comment 3

[Maniac Vlad Florin (:vladmaniac)]

The reason why we fail is explained in this pastebin: 

http://themaniak.pastebin.mozilla.org/1613688 

We are accessing the window object after the window has been closed, and then the object is dead. 
I'm going to also add bug 628576 and bug 639870 to dependencies, because they will have to consider this change. 
I am wondering how we are going to test for the closing event if the window object is dead? 
Do we have an event listener there?

Comment 4

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

Please never explain stuff in a pastebin. Always add an attachment if necessary to a bug but better do it wisely in a comment.

Comment 5

[Maniac Vlad Florin (:vladmaniac)]

(In reply to Henrik Skupin (:whimboo) from comment #4)
> Please never explain stuff in a pastebin. Always add an attachment if
> necessary to a bug but better do it wisely in a comment.

Ok. Well please consider comment 3 as an explanation (what is after the pastebin link which has the code snippet). The comment also contains my concerns at the moment

Comment 6

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

Complete list of tests which are affected:

functional/testPrivateBrowsing/testCloseWindow.js
functional/testPrivateBrowsing/testDisabledElements.js 
functional/testPrivateBrowsing/testDownloadManagerClosed.js

When we have landed the patch and it works I will port the code to the new API and Mozmill core.

Comment 7

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

Attachment: More robust window closing code (v1)

So it appears that we have to silently fail in some cases. Exactly when the test itself closes the window but doesn't specify the flag for handleWindow. In those circumstances our code still wants to force closing the window and fails because the window object is not available anymore.

Comment 8

[Dave Hunt [:davehunt] [he/him] ⌚BST]

Comment on attachment 620651 [details] [diff] [review]
More robust window closing code (v1)

># HG changeset patch
># Parent 4d11b76a73ae0a801fc5ea8b5eddf714f9663731
># User Henrik Skupin <[e-mail]>
>Bug 751242 - Make handleWindow code more robust when the test already closed the window. r=dhunt
>
>diff --git a/lib/utils.js b/lib/utils.js
>--- a/lib/utils.js
>+++ b/lib/utils.js
>+      } catch (e if e instanceof TypeError) {
>+        // The test itself has already been destroyed the window. Also the object
>+        // is not available anymore because it has been marked as dead. We can fail
>+        // silently.
>+      }
>+

Minor nit: Should this comment read "The test itself has already destroyed the window"? The "been" makes it unclear to me.

r=me with the above comment clarified

Comment 9

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

Attachment: More robust window closing code (v2)

Fixed the minor review nit. Will land it now.

Comment 10

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

Landed on default:
http://hg.mozilla.org/qa/mozmill-tests/rev/6f1bbe8238a7

Once we get a clean test-run we should backport this to older branches too.

Comment 11

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

No more test failures are visible. I'm going to land this change for the other branches.

Landed as:
http://hg.mozilla.org/qa/mozmill-tests/rev/457f311f0b9d (aurora)
http://hg.mozilla.org/qa/mozmill-tests/rev/59fd6a49b5fc (beta)
http://hg.mozilla.org/qa/mozmill-tests/rev/53216b03d6e7 (release)
http://hg.mozilla.org/qa/mozmill-tests/rev/746170de8030 (esr10)

Comment 12

[Maniac Vlad Florin (:vladmaniac)]

Now we are failing with this 

http://mozmill-release.blargon7.com/#/functional/report/c2b72632f20450b6d99d14c70981dfb9 

I think this needs to be filed separately in another bug, as a regression to this patch?

Comment 13

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

I don't think this is a regression. Please wait for todays tests before filing any new bug. Keep in mind that Mozmill was broken in the last 2 days due to bug 751424. Version 1.5.12 has been released right now.

Comment 14

[Maniac Vlad Florin (:vladmaniac)]

I agree now that my local test has finished, thanks Henrik for clarifications

http://mozmill-crowd.blargon7.com/db/c2b72632f20450b6d99d14c70983f971

Comment 15

[Remus Pop (:RemusPop)]

Seems to fail again on Mac with Nightly and can be reproduced locally.

Comment 16

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

How long are they failing again? Please always provide a link to the failure report.

Comment 17

[Remus Pop (:RemusPop)]

Sorry, on CI it stopped failing in 25th of May.
http://mozmill-ci.blargon7.com/#/functional/failure?branch=All&platform=All&from=2012-05-25&test=%2FtestPrivateBrowsing%2FtestCloseWindow.js&func=testCloseWindow.js%3A%3AtestCloseWindow

Though I can reproduce it locally.

Comment 18

[Remus Pop (:RemusPop)]

Tried to find something useful, but I think it's machine dependant. I used a sleep call and things solved. It is something related to the tab content not loading fast enough when switching tabs.
Do we still want this fixed given that I can reproduce only on 1 machine?

Comment 19

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

(In reply to Remus Pop (:RemusPop) from comment #18)
> Tried to find something useful, but I think it's machine dependant. I used a
> sleep call and things solved. It is something related to the tab content not
> loading fast enough when switching tabs.

Not sure what you mean by that. Where have you added a sleep call? Please tell us more details how you tried to find a workaround at least for the time being.

> Do we still want this fixed given that I can reproduce only on 1 machine?

It doesn't matter on how many machines it can be reproduced. It's failing and needs a fix.

Comment 20

[Remus Pop (:RemusPop)]

After this line I inserted a sleep(2000).
http://hg.mozilla.org/qa/mozmill-tests/file/73b913d2b1f4/tests/functional/testPrivateBrowsing/testCloseWindow.js#l87

The failure complains about a new elementslib when executing this._view = _document.defaultView;
So I think the DOM is not ready when executing line 88: var tab = controller.tabs.getTab(i);

Comment 21

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

(In reply to Remus Pop (:RemusPop) from comment #20)
> After this line I inserted a sleep(2000).
> http://hg.mozilla.org/qa/mozmill-tests/file/73b913d2b1f4/tests/functional/
> testPrivateBrowsing/testCloseWindow.js#l87

This is a property so you might want to experiment even more and add the sleep call within the corresponding tabBrowser.selectedIndex setter.

> The failure complains about a new elementslib when executing this._view =
> _document.defaultView;
> So I think the DOM is not ready when executing line 88: var tab =
> controller.tabs.getTab(i);

The former window which was existent at the beginning of the test doesn't exist anymore. So both the controller and tabbrowser have invalid cached windows and should probably recreated after the window has been reopened by leaving the private browsing mode.

Comment 22

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

Given that this is a test specific failure please file a new bug for it.