Closed Bug 626674 Opened 11 years ago Closed 11 years ago

Page Info dialog is not closed after testAccessPageInfoDialog.js

Categories

(Mozilla QA Graveyard :: Mozmill Tests, defect)

defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: u279076, Assigned: u279076)

References

Details

Attachments

(1 file, 9 obsolete files)

While investigating bug 614973, I noticed that the Page Info dialog remains open after the test finishes.  It's completely possible that some of the failures which occur after this test are due to this window remaining open.

I'll follow up with a patch soon which ensures this dialog is closed on all branches.
Blocks: 614973
Assignee: nobody → anthony.s.hughes
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Fundamentally, this patch does not change what we are doing all that much.

1. I've added a waitFor after handleWindow in the test as a safety-net to report window state in case it is not closed.
2. I've revised handleWindow to check for window.closed.  When a window is closed this property is set to true.

As I said before, this doesn't really change things all that much.  In fact, it does not resolve the testPasswordNotSaved failure.  However, the Page Info dialog no longer seems to linger after the test with these changes.
Attachment #505015 - Flags: review?(hskupin)
Comment on attachment 505015 [details] [diff] [review]
Patch v1

Ignore this patch...I made a mistake.  New one coming up.
Attachment #505015 - Flags: review?(hskupin)
Attached patch Patch v2 (default) (obsolete) — Splinter Review
I've tested this patch thoroughly, watching various test runs, and I no longer see any dialogs linger after a test is complete.  In fact, it's also reduced (if not eliminated) the cases where an identity pop-up or context menu lingers.
Attachment #505015 - Attachment is obsolete: true
Attachment #505248 - Flags: review?(hskupin)
Comment on attachment 505248 [details] [diff] [review]
Patch v2 (default)

>+  var contextMenu = new elementslib.ID(controller.window.document,
>+                                       "context-viewinfo");

That's not the context menu, it's the page info menu entry. It's a bit confusing. Also if you want, you could use the new MenuAPI (sadly no documentation yet) which has been introduced with Mozmill 1.5.1. It will make the handling of context menus really stable and also takes care of closing the context menu immediately, so we don't need the teardown anymore.

>+  // Handle the Page Info dialog
>   utils.handleWindow("type", "Browser:page-info", checkPageInfoWindow);
>+  
>+  // Verify the Page Info dialog is closed
>+  controller.waitFor(function () {
>+    return checkPageInfoWindow.closed !== false;
>+  }, "Check for Page Info dialog closed: got " + checkPageInfoWindow.closed +
>+    ", expected true");

This last check to wait for the window has been closed has to be part of the handleWindow function and should not be used in tests itself. Also our call above enforces the closing of the window by not passing in the last parameter.

>-      return window != null;
>+      return window !== null;

We can even simplify that by checking for !!window. The Mozilla style guide says that objects shouldn't be compared to null. 

>     // XXX: We still have to find a reliable way to wait until the new window
>     // content has been finished loading. Let's wait for now.
>+    // YYY: waitFor() seems to be working in favour of sleep()

There is no need for an additional comment. When waitFor is much safer, lets use it. But lets stop adding comments for changes which will not be visible for later inspection.

>+    mozmill.utils.waitFor(function () {
>+      return window.closed === false;
>+    }, "Checking for closed window: got " + window.closed + ", expected false");
>     controller = new mozmill.controller.MozMillController(window);
>-    controller.sleep(200);

But why are you waiting for "closed === false"? This step doesn't close the window and we really have to wait for .documentLoaded set on the window object itself. That will indicate that the window has been finished loading. The .closed property we can use later... 

>     if (close && window) {
>       controller.window.close();
>       mozmill.utils.waitFor(function () {
>-        return func_ptr(text) != window;
>-      }, "Window has been closed.");
>+        return window.closed === true;
>+      }, "Checking for closed window: got " + window.closed + ", expected true");

Returning 'window.closed' is enough here. It's already a boolean and doesn't have to compared to a constant. For the message simply leave it as it was before. Because it's a boolean we only have two states and it is clear if this step fails.

Also this last check makes the additional waitFor call in the page info test unnecessary.

Thanks for catching that! Lets get those remaining comments fixed and it will be ready to land.
Attachment #505248 - Flags: review?(hskupin) → review-
Attached patch Patch v2.1 (default) (obsolete) — Splinter Review
I believe I addressed all the revisions.
Attachment #505248 - Attachment is obsolete: true
Attachment #505489 - Flags: review?(hskupin)
Attached patch Patch v2.1 (default) (obsolete) — Splinter Review
Uploaded the wrong patch.  This *should* be the right one.
Attachment #505489 - Attachment is obsolete: true
Attachment #505490 - Flags: review?(hskupin)
Attachment #505489 - Flags: review?(hskupin)
Attachment #505490 - Attachment is patch: true
Attachment #505490 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 505490 [details] [diff] [review]
Patch v2.1 (default)

> var teardownModule = function(module) {
>   utils.closeContentAreaContextMenu(controller);
> }

Please get rid of the teardown module. It's not necessary anymore.

Otherwise it looks great. You have my r+. Please attach a new patch and take over the review before the landing. Thanks!
Attachment #505490 - Flags: review?(hskupin) → review+
Attached patch Patch v2.2 (default) (obsolete) — Splinter Review
Attachment #505490 - Attachment is obsolete: true
Attachment #505655 - Flags: review+
Attachment #505655 - Attachment is patch: true
Attachment #505655 - Attachment mime type: application/octet-stream → text/plain
(In reply to comment #8)
> Created attachment 505655 [details] [diff] [review]
> Patch v2.2 (default)

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/7feb3156e11e [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/f5ef39e83dd4 [mozilla1.9.2]

Backport for 1.9.1 coming up.
Attached patch Patch v2.2 (mozilla1.9.1) (obsolete) — Splinter Review
The patch is failing to open the context menu on Shiretoko.  Henrik, can you look at my patch and see what I might be doing wrong?  It should be identical to the default/mozilla1.9.2 patch.
Attachment #505671 - Flags: feedback?(hskupin)
Comment on attachment 505671 [details] [diff] [review]
Patch v2.2 (mozilla1.9.1)

See bug 627753 which is the reason why it doesn't work. Lets keep the old way of handling the context menu in this test.
Attachment #505671 - Flags: feedback?(hskupin)
Attached patch Patch v2.3 (mozilla1.9.1) (obsolete) — Splinter Review
Going back to the old method of context menu handling works with one caveat; the context menu does not disappear after being clicked.  This does not appear to cause any errors. I've tried adding a waitFor() to check for contextMenu.state === 'closed' but it says it's undefined.  

Personally, I think we are ok for now with this context menu not disappearing since it's not causing any errors.  Once the MenuAPI works on 1.9.1 (via bug 627753), this glitch will be resolved.
Attachment #505671 - Attachment is obsolete: true
Attachment #505847 - Flags: review?(hskupin)
Comment on attachment 505847 [details] [diff] [review]
Patch v2.3 (mozilla1.9.1)

>-var teardownModule = function(module) {
>-  utils.closeContentAreaContextMenu(controller);
>-}

You will have to restore the teardown function which makes sure the context menu gets closed.

Otherwise looks good. r=me with that change.
Attachment #505847 - Flags: review?(hskupin) → review+
Here's a patch showing the problem.  If we check for "return func_ptr(text) != window" the dialog fails to close properly.  If we check for "return window.closed === true" the controller is lost during the waitThenClick() for the Advanced Pane button.

FAILURE:
{"exception": {"message": "this._controller.window.document is null", "lineNumber": 115, "stack": "(\"paneAdvanced\")@resource://mozmill/stdlib/securable-module.js -> file:///tmp/tmpzXVYu9.mozmill-tests/shared-modules/prefs.js:115

CODE:
set paneId(id) {
    var button = this.getElement({type: "selector_button", value: id});
    this._controller.waitThenClick(button, gTimeout);
Attachment #505978 - Flags: feedback?(hskupin)
Comment on attachment 505978 [details] [diff] [review]
Patch: waitThenClick bug (default)

No need for a feedback request. It's more a testcase as a patch at the moment.
Attachment #505978 - Flags: feedback?(hskupin)
Comment on attachment 505978 [details] [diff] [review]
Patch: waitThenClick bug (default)

>     if (close && window) {
>       controller.window.close();
>       mozmill.utils.waitFor(function () {
>-        return func_ptr(text) != window;
>+        //return func_ptr(text) != window;
>+        return window.closed === true;
>+dump("\n *** window: " + window + ", closed: " + window.closed + " *** \n");
>       }, "Window has been closed.");
> 
>       window = null;
>       controller = null;

It's a timing issue. Adding a controller.sleep(0) call right after the waitFor call fixes the problem for me. Looks like we block any kind of event handling. Probably it would be more wise to check for an even fired by the closing of the window. Probably Neil should know about it.
(In reply to comment #17)
>> Comment on attachment 505978 [details] [diff] [review]
>> Patch: waitThenClick bug (default)
> Adding a controller.sleep(0) call right after the waitFor call fixes the
> problem for me. Looks like we block any kind of event handling.

Would it be sufficient for me to submit a patch which uses the sleep(0)? I can also file a bug to investigate a better window handling API.
(In reply to comment #18)
> Would it be sufficient for me to submit a patch which uses the sleep(0)? I can
> also file a bug to investigate a better window handling API.

To get the failures fixed I would say go ahead. But please file a new bug and mention that one in the code. We can use the new bug to get the necessary information from devs. Also please run it with sleep(0) on other platforms, which I haven't done yet. We should be sure that it will pass everywhere. Not that it is only a workaround on OS X.
Not sure why, but sleep(0) no longer seems to fix this.  I've tested a few times on Mac, Linux and Windows and it always fails now:

this._controller.sleep(0);

Error: this._controller is undefined

I almost think we should block this on bug 628576.  Afterall, any work done here will just be a crutch which has to be removed for that bug.
(In reply to comment #20)
> Not sure why, but sleep(0) no longer seems to fix this.  I've tested a few
> times on Mac, Linux and Windows and it always fails now:
> 
> this._controller.sleep(0);
> 
> Error: this._controller is undefined

Not sure why you are using this._controller here. There is no class and no _controller getter on it, but you have controller. That's the only reason why you get this failure.
Thanks for pointing that out. I'm testing it now across all platforms and branches.  I'll attach a patch as soon as it tests ok.
Attached patch Patch v3 (obsolete) — Splinter Review
This has been tested and works for me locally.
Attachment #505655 - Attachment is obsolete: true
Attachment #505847 - Attachment is obsolete: true
Attachment #505978 - Attachment is obsolete: true
Attachment #506787 - Flags: review?(hskupin)
Comment on attachment 506787 [details] [diff] [review]
Patch v3

>+    mozmill.utils.waitFor(function () {
>+      return window.documentLoaded;
>+    }, "Window content should now be loaded.");

Please replace 'should' with 'has been loaded'.

>-        return func_ptr(text) != window;
>+        return window.closed === true;

Remove the '=== true' part, because window.closed is already a boolean.

>+      // XXX: Bug 628576 - We need to sleep here or else we fail
>+      controller.sleep(0);

Please use what we want to have in the comment. The current comment except the bug id, isn't helpful for upcoming work.

With those changes r=me.
Attachment #506787 - Flags: review?(hskupin) → review+
Attachment #506787 - Attachment is obsolete: true
Attachment #507237 - Flags: review+
Comment on attachment 507237 [details] [diff] [review]
Patch v3.1 [checked-in]

Landed:
http://hg.mozilla.org/qa/mozmill-tests/rev/0a4d861744be [default]
http://hg.mozilla.org/qa/mozmill-tests/rev/4735276aec0f [mozilla1.9.2]
http://hg.mozilla.org/qa/mozmill-tests/rev/fb8b0e461d23 [mozilla1.9.1]
Attachment #507237 - Attachment description: Patch v3.1 → Patch v3.1 [checked-in]
Marking as resolved fixed. I'll check tomorrow's testrun results to mark as verified.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Todays results look great! This improvement has helped a lot.
Status: RESOLVED → VERIFIED
Product: Mozilla QA → Mozilla QA Graveyard
You need to log in before you can comment on or make changes to this bug.