492136 - [shared module] If test in modal dialog fails no helpful failures are fired and dialogs stay open

Status: RESOLVED FIXED

(Mozilla QA Graveyard :: Mozmill Tests, defect, P2)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: testcase

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b5pre) Gecko/20090507 Shiretoko/3.5b5pre ID:20090507034334 and Mozmill trunk

With the fix of bug 479311 we are now able run tests inside modale dialogs. That's great. But as what I have seen in my Private Browsing test which is currently in development, our framework completely hangs when something breaks inside the modal dialog. In the testcase I've attached here a letter is missing in the element lookup of the Start Private Browsing mode button. So it cannot be found. Even after our global timeout of 30s an exception is thrown but we don't close the dialog. 

Can we somehow implement a hidden check which logs this failure? We shouldn't bother the user to have to enter some code each time inside his handler function.

To run the testcase clone the mozmill-test repository and save the testcase into the Firefox folder.

Running the test will throw the following exception:

Error: [Exception... "'Expression "{"icon":"accept","accesskey":"S","default":"true","step":"1","dlgtype":"accept","xbl:inherits":"disabled=buttondisabledaccep"}" returned null. Anonymous == false' when calling method: [nsIObserver::observe]"  nsresult: "0x8057001e (NS_ERROR_XPC_JS_THREW_STRING)"  location: "<unknown>"  data: no]

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Right now I cannot see this each time but sometimes even with the correct element specified the dialog hangs. As long as it doesn't happen all the time I would set it to P2.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

It's even more worse. If something fails inside a modal dialog callback the test is stopped immediately. That means there is no way to close open dialogs. Tests which will be run afterward will fail because they cannot access the browser window.

If there will be no way to implement a garbage collection in Mozmill after a test we should have another shared function which closes all windows - also modal ones except the last browser window and an open Mozmill window.

Mikeal or Clint, what do you think?

Comment 3

[Mikeal Rogers [:mikeal] (mrogers@)]

That's a bit too aggressive in my opinion.

First off, closing all windows won't necessarily clean up the state created by those windows. For example, changing prefs won't get cleared out just my killing the windows.

I think I could be in favor of killing all modal windows at the end of running a test *module* after teardownModule is called.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to comment #3)
> I think I could be in favor of killing all modal windows at the end of running
> a test *module* after teardownModule is called.

Or we implement it as a function in a shared module and call it ourself in those modules where a modal dialog is used and could fail.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

We have a couple of tests in modal dialogs. If a test fails in such a dialog it will not be reported as failure. Means those tests are useless at the moment. I think we should fix it before running tests on the build system.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: Patch v1

Ok, adding a try/catch clause inside the modalDialogAPI does the trick.

Mikeal, the only concern I have is that the test is proceeded even when a test fails inside the modal dialog and I call frame.fail. Shouldn't it stop immediately? In controller.js I don't see any usage of frame.fail. If something fails there we only throw an exception. Could this be the cause why the pass/fail counters report wrong results after the test has been finished?

Comment 7

[Mikeal Rogers [:mikeal] (mrogers@)]

It's absolutely possible from the frameworks perspective to have multiple failures in a single test function. The mozmill framework doesn't assume that failures are synchronous, this is what allows us to have unittests that can cause failures and passes without stopping the current function call.

In our functional tests we make sure to cause exceptions because it's much cleaner to have the test stop once there's a failure point. The framework picks up the exception and calls frame.events.fail() which is why you don't see this in the controller api.

Since you have code that is executing outside of the main execution thread, throwing an exception won't get picked up by the framework so the best you can hope for is calling frame.events.fail() on your own.

If you have some code that you know won't be outside the main thread and the exception will get picked up by the framework then you should have this API return true/false and then cause an exception in the main thread.

Comment 8

[cmtalbert]

Yeah, the issue here is that the modal dialog is exercising outside the main thread of execution and it breaks the synchronous model that the main testing in Mozmill uses (aka the non-unit test model).

While the more heavyweight way to fix this involves using observers/listeners between the framework and the modal dialog API, I think Mikeal's solutions in comment 7 are far better.  

If we can return a true/false return value from the modal dialog API on whether or not the test passed/failed and then throw an error based on that data I think it will solve this issue.

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

(In reply to comment #7)
> If you have some code that you know won't be outside the main thread and the
> exception will get picked up by the framework then you should have this API
> return true/false and then cause an exception in the main thread.

We cannot return true/false. We have to return at least the exception itself. Otherwise we can only throw a general exception which is also useless. I have to figure out how I can get it to run with nsIThread. The async API of Mozmill doesn't work for this special case. :(

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Attachment: Patch v2 (checked-in)

This patch changes the way how we detect modal dialogs. The current check for chrome flags is not reliable. There exists a isWindowModal function for the nsIWebBrowserChrome class which is doing this job very well.

Clint, this is not the final solution but this patch solves two major issues. It would be nice if we could get in this patch right now while I will figure out later how to exchange data between threads.

Comment 11

[cmtalbert]

Comment on attachment 393486 [details] [diff] [review]
Patch v2 (checked-in)

This looks great, Henrik.  Thanks!  r=ctalbert

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Thanks Clint. I have landed the first part as http://hg.mozilla.org/qa/mozmill-tests/rev/d0ac8ed2122e

This fixes:
* Returns a failure to the framework when something is broken in the modal dialog
* Closes the modal dialog when an error happens
* Now hopefully finds any type of a modal window (if not its a core bug)

Todo:
* Get the exception reported to the main Mozmill thread

Comment 13

[Aakash Desai [:aakashd]]

testClearFormHistory in bug 509125 passes via command line and IDE on browsers Shiretoko and Fx3.5 due to the fix mentioned in comment #12. I'd say we can push this to resolved:fixed now since we don't have a testcase for when an error will occur within the modal dialog API.

Comment 14

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

This is not fixed. Otherwise I would have it set to resolved. See comment 12 for the remaining work.

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Putting back severity to normal. It's not critical anymore.

(In reply to comment #13)
> push this to resolved:fixed now since we don't have a testcase for when an
> error will occur within the modal dialog API.

Make any click or assertNode fail inside the modal dialog. Then you will see that there we need one more fix.

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Lets close this bug. Remaining work to implement a threading model will happen in bug 512475.

Comment 18

[Henrik Skupin [:whimboo][⌚️UTC+2] (away 07/21 - 08/08)]

Mass move of Mozmill Test related project bugs to newly created components. You can filter out those emails by using "Mozmill-Tests-to-MozillaQA" as criteria.