831297 - Test Failure "Disconnect Error: Application unexpectedly closed" in /l10n/testAccessKeys/test1.js

Status: RESOLVED FIXED

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

Description

[Andrei Eftimie]

Test hangs when it changes the Privacy > Remember History option on the modal dialog:
"Restart Nightly. Nightly must restart to enable this feature."

A failed testrun: http://mozmill-ci.blargon7.com/#/l10n/report/f25fe2f500e5e4086802832f521049e9

Comment 1

[Andreea Matei [:AndreeaMatei]]

I see this failing only on Aurora, at least from reports. Can you check if all branches that have this test are affected?

Comment 2

[Andrei Eftimie]

Only Nightly and Aurora.
I am doing a regression range, so far it started somewhere around christmas :)

Comment 3

[Andreea Matei [:AndreeaMatei]]

Thanks Andrei! 
So if this new dialog has been introduced recently, we need to update our test to handle it (probably close it since we don't need to restart Nightly, just check the access keys).

Comment 4

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

Well, we should never spawn this dialog actually. Whatever it triggers has to be updated. I can't imagine what this is.

Comment 5

[Andrei Eftimie]

You can trigger it manually by changing the value of the dropdown:
Preferences > Privacy > History > Nightly will <dropdown>

I am working now on fixing it -- if I figure out how the DOMWalker actually works :)

Comment 6

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

No, at this point there is no value in fixing that on our side. We have to understand why this is happening. So please find the bug which landed this 'feature'.

Comment 7

[Andrei Eftimie]

This change in firefox was added with revision: http://hg.mozilla.org/mozilla-central/rev/33d3e7354bf9
under bug 801823 

It is intentional in nature, so we'll probably need to adapt our test to not fail there anymore.

I've tried closing the prompt with Cancel (our test will pass if we do this), but I'm not sure how to achieve this.
Has anyone any pointers for me? (DOM Walker from lib/dom-utils.js)

Comment 8

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

Thanks Andrei. Marking bug 801823 as the regressor in the dependency field.

Hm, so when we close the dialog do we see the new selected pane? Or is it reset to the one which was set before?

Comment 9

[Andrei Eftimie]

Not sure what you mean.

If we run the test, and manually press Cancel, the test proceeds as planned and passes.
The underlying pane does not change, we only have a new modal/confirm window on top.

Comment 10

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

I think for historyMode 'dontRemember' we have to change the target from WINDOW_CURRENT to WINDOW_MODAL. Does that help right away?

Comment 11

[Andrei Eftimie]

I've tried that (and other variations of adding subContent items). Doesn't work.

The Restart dialog appears before the new value is set in the menulist item, so I'll have to patch the DOMWalker to handle this. Probably by setting a specific property on these nodes, instructing the DOMWalker to press cancel when the dialog appears.

Comment 12

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

When is that dialog fired exactly? Can you please reference the line of code from Firefox? I think it would help me to understand what we should be able to do.

Comment 13

[Andrei Eftimie]

I am not sure, I think this one:
http://mxr.mozilla.org/mozilla-central/source/browser/components/preferences/privacy.js#248

I have managed to get the test running by invoking a new Modal Dialog instance at the steps where it opens.
But I now have a 50% change that the dropDown menu does not update anymore... still trying to figure out why this happens 
( this._controller.select(dropDown, null, null, idSet.value); // actually fails to update the value )

Comment 14

[Andrei Eftimie]

Attachment: skipTest v1

One possible solution would be to skip tests that require restarts altogether.
I am not sure if this would be the right call. If we'll end up taking this
route, I have uploaded a patch for this.

The Problem:

When we close the Restart Dialog (basically Cancel) the value of the menulist 
is reset to its original state, but we are asserting that the value has changed
invalidating our test.

If we try to read the value _before_ closing the modal dialog, the assert does 
not resolve even if its return value is true.
Which is weird, but I might infer that it is due to the Modal Dialog being
open, and behaving really modal, thus not letting our controller do anything.
(Not sure how this actually behaves, but this is the impression I get)

If we close the Modal Dialog first, we have a chance of about 50% to read the
value of the menulist correctly before the closing event of the MD resets the
value to its original state.

===

Possible solutions:
1. we could skip the tests that require restart
2. we could do the restart, and go forward with the test from where we were
(not sure if actually doable/feasible)
3. we might be able to read the menulist entry while the Reset Dialog is open
(I have not been able to at this )

Comment 15

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

(In reply to Andrei Eftimie from comment #14)
> When we close the Restart Dialog (basically Cancel) the value of the
> menulist 
> is reset to its original state, but we are asserting that the value has
> changed
> invalidating our test.

This is an absolute worst case scenario we hit here. I don't see a way for us to test this specific panel in a testrun as general as our l10n tests are. We can't accept the restart which would make our test invalid, because we don't know where to continue.

Ehsan, why do we have such a hard requirement for the restart at this point? Was it not enough to warn the user only?

> Possible solutions:
> 1. we could skip the tests that require restart

We might have to do that, at least as long as we have a working solution.

> 2. we could do the restart, and go forward with the test from where we were
> (not sure if actually doable/feasible)

No, that's not doable given that we could be everywhere and we can't reply to go through the same steps again. Also we would stay in PB mode which we do not want!

> 3. we might be able to read the menulist entry while the Reset Dialog is open
> (I have not been able to at this )

That will not work.

I would request feedback from Ehsan first. The new behavior adds a (for now) non-resolvable problem to our tests.

Comment 16

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

Comment on attachment 704536 [details] [diff] [review]
skipTest v1

If we use the skip it's enough to skip only the dontremember and privateBrowsingAutoStart part.

Comment 17

[(no longer active)]

(In reply to Henrik Skupin (:whimboo) from comment #15)
> (In reply to Andrei Eftimie from comment #14)
> > When we close the Restart Dialog (basically Cancel) the value of the
> > menulist 
> > is reset to its original state, but we are asserting that the value has
> > changed
> > invalidating our test.
> 
> This is an absolute worst case scenario we hit here. I don't see a way for
> us to test this specific panel in a testrun as general as our l10n tests
> are. We can't accept the restart which would make our test invalid, because
> we don't know where to continue.
> 
> Ehsan, why do we have such a hard requirement for the restart at this point?
> Was it not enough to warn the user only?

It's not possible to switch the PB autostart mode at runtime any more, since there is no way we can make sure that everything works correctly after doing that.  That is why we require a restart now.

> > Possible solutions:
> > 1. we could skip the tests that require restart
> 
> We might have to do that, at least as long as we have a working solution.
> 
> > 2. we could do the restart, and go forward with the test from where we were
> > (not sure if actually doable/feasible)
> 
> No, that's not doable given that we could be everywhere and we can't reply
> to go through the same steps again. Also we would stay in PB mode which we
> do not want!
> 
> > 3. we might be able to read the menulist entry while the Reset Dialog is open
> > (I have not been able to at this )
> 
> That will not work.
> 
> I would request feedback from Ehsan first. The new behavior adds a (for now)
> non-resolvable problem to our tests.

Hmm, I may be missing something here, but this test doesn't actually test anything PB related, right?  I think you should be fine if you just cancel this dialog and assert that the menulist value has been reset to its original value, right?

Comment 18

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

(In reply to :Ehsan Akhgari from comment #17)
> Hmm, I may be missing something here, but this test doesn't actually test
> anything PB related, right?  I think you should be fine if you just cancel
> this dialog and assert that the menulist value has been reset to its
> original value, right?

This test is executing various l10n checks for the preferences dialog including multiple access keys and cropped elements. The results are important for localizers.

So we crawl through any existing elements and sub windows of the given top-most window. Given that new behavior we are no longer able to test the pane which is shown with the 'dontremember' menulist item selected. Means any possible fallout for the above mentioned l10n checks will no longer be propagated correctly to our dashboard. It looks like we have to live with this.

Comment 19

[(no longer active)]

(In reply to comment #18)
> (In reply to :Ehsan Akhgari from comment #17)
> > Hmm, I may be missing something here, but this test doesn't actually test
> > anything PB related, right?  I think you should be fine if you just cancel
> > this dialog and assert that the menulist value has been reset to its
> > original value, right?
> 
> This test is executing various l10n checks for the preferences dialog including
> multiple access keys and cropped elements. The results are important for
> localizers.
> 
> So we crawl through any existing elements and sub windows of the given top-most
> window. Given that new behavior we are no longer able to test the pane which is
> shown with the 'dontremember' menulist item selected. Means any possible
> fallout for the above mentioned l10n checks will no longer be propagated
> correctly to our dashboard. It looks like we have to live with this.

Hmm, I see.  Perhaps you could monkey patch things to disable the function which performs the restart check?

Comment 20

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

Might be possible, yes. We could introduce a new type of action for elements to test like preAction and postAction, which we would have to process before or after the actual test happens. That could work. Thanks for the hint!

Comment 21

[Andrei Eftimie]

Attachment: Patch v1

Implemented hooks in the DOMWalker.
Then used them to disable the events that were triggering the Restart Dialog.

-- I initially used them to disable the events, and reenable them for each node.
But for the sake of simplicity, I've used them only once to disable what we don't need on the first occurence of the each item (the "historyMode" element, and the "privateBrowsingAutoStart" preference)

I've also made named function for each one initially, but as they are a 1 liner, I've now reverted to using anonymous functions instead.

Passing testrun:
http://mozmill-crowd.blargon7.com/#/l10n/report/72e15dc943833b8fcba70aeb5106959e

Comment 22

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

Comment on attachment 704815 [details] [diff] [review]
Patch v1

Review of attachment 704815 [details] [diff] [review]:
-----------------------------------------------------------------

Looks ok, but I miss the handling of the changed values. We really have to reset those.

::: lib/dom-utils.js
@@ +243,5 @@
>        if (node) {
>          var idSet = ids[i];
>  
> +        // Pre Hook
> +        if (idSet.preHook) {

You should do 'if ('preHook' in idSet)' to avoid a strict warning.

@@ +245,5 @@
>  
> +        // Pre Hook
> +        if (idSet.preHook) {
> +          assert.equal(typeof idSet.preHook, "function",
> +                 "preHook is not a function");

Please fix the indendation

@@ +301,5 @@
> +        // Post Hook
> +        if (idSet.postHook) {
> +          assert.equal(typeof idSet.postHook, "function",
> +                 "postHook is not a function");
> +          idSet.postHook.call(node);

Same as above.

::: tests/l10n/testAccessKeys/test1.js
@@ +22,4 @@
>    controller = mozmill.getBrowserController();
>  }
>  
> +var prefPaneInit = function(controller, prefDialog) {

Please stick what we had before. It might be worth to agree on a standard via the mailing list. Would you mind to raise this topic?

@@ +74,5 @@
>          { getBy : GET_BY_ID,
>            id : "historyMode",
>            target : WINDOW_CURRENT,
> +          preHook : function() {
> +            this.setAttribute("oncommand", "gPrivacyPane.updateHistoryModePane();");

Why do you already set this hook for this element and not for dontremember? Also we have to backup values for whatever we do in those hooks. Also I miss the postHook which resets it.

@@ +89,5 @@
>            subContent : [
>              { getBy : GET_BY_ID,
>                id : "privateBrowsingAutoStart",
> +              preHook : function() {
> +                this.removeAttribute("preference");

Hm, why do we need this here? Do we really click on the element? I don't think we do.

Comment 23

[Andrei Eftimie]

Attachment: Patch v2

Addressed all requests, and with additional information:

> ::: tests/l10n/testAccessKeys/test1.js
> @@ +22,4 @@
> >    controller = mozmill.getBrowserController();
> >  }
> >  
> > +var prefPaneInit = function(controller, prefDialog) {
>
> Please stick what we had before. It might be worth to agree on a standard 
> via the mailing list. Would you mind to raise this topic?

I do not have a specific preference here, I was told that with any 
opportunity we have from "function name()" in "var name = function()".

I have restored the names as they previously were.


> @@ +74,5 @@
> >          { getBy : GET_BY_ID,
> >            id : "historyMode",
> >            target : WINDOW_CURRENT,
> > +          preHook : function() {
> > +            this.setAttribute("oncommand", "gPrivacyPane.
> > updateHistoryModePane();");
> 
> Why do you already set this hook for this element and not for dontremember?
> Also we have to backup values for whatever we do in those hooks. Also I miss 
> the postHook which resets it.

Since it was the same element, and they are accessed in order, it didn't 
_need_ setting on both changes. I do agree that it is better to have them more encapsulated.

I have added pre and post calls to all nodes that needed it, saving the
original value on pre, then restoring it on post.

> @@ +89,5 @@
> >            subContent : [
> >              { getBy : GET_BY_ID,
> >                id : "privateBrowsingAutoStart",
> > +              preHook : function() {
> > +                this.removeAttribute("preference");
> 
> Hm, why do we need this here? Do we really click on the element? I don't 
> think we do.

We click this element, and it also opens the Restart Dialog, that's why it is
also disabled

Comment 24

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

Comment on attachment 704893 [details] [diff] [review]
Patch v2

Review of attachment 704893 [details] [diff] [review]:
-----------------------------------------------------------------

::: lib/dom-utils.js
@@ +302,5 @@
> +        if ('postHook' in idSet) {
> +          assert.equal(typeof idSet.postHook, "function",
> +                       "postHook is not a function");
> +          idSet.postHook.call(node);
> +        }

If the code in between pre and post fails, we might never call the postHook callback and reset everything. Can we wrap the above in a try/finally?

::: tests/l10n/testAccessKeys/test1.js
@@ +209,5 @@
> +/**
> + * Disables the Reset Dialog by overwriting the oncommand attribute
> + * and saving the original value in a temporary location
> + */
> +function disableCommand(){

Please give that callback a better name.

@@ +211,5 @@
> + * and saving the original value in a temporary location
> + */
> +function disableCommand(){
> +  var aCommand = this.getAttribute("oncommand");
> +  this.setAttribute("data-tempCommand", aCommand);

I would give it a name which includes `backup` like `data-command-backup`. Otherwise nice idea how to do it.

Comment 25

[Andrei Eftimie]

Attachment: patch v3

Addressed review requests.

Comment 26

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

Comment on attachment 708039 [details] [diff] [review]
patch v3

Review of attachment 708039 [details] [diff] [review]:
-----------------------------------------------------------------

Looks really fine, but please do not forget to also update the test for cropped elements. It needs the same changes.

::: lib/dom-utils.js
@@ +299,5 @@
> +              throw new Error("Node does not exist: " + ids[i][ids[i].getBy]);
> +          }
> +        }
> +
> +        // Post Hook

Please move this comment inside of finally and state that we always have to make sure to call the post hook.

Comment 27

[Andrei Eftimie]

Attachment: patch v4

Also applied the changes to the Cropped tests, and changed the comment for the Post Hook.

A passing testrun: http://mozmill-crowd.blargon7.com/#/l10n/report/ad726b5c70cf80fbf8135edfca0402ed

Comment 28

[Andreea Matei [:AndreeaMatei]]

Comment on attachment 709680 [details] [diff] [review]
patch v4

Review of attachment 709680 [details] [diff] [review]:
-----------------------------------------------------------------

It works well for me, has all requested changes.

Comment 29

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

Comment on attachment 709680 [details] [diff] [review]
patch v4

Review of attachment 709680 [details] [diff] [review]:
-----------------------------------------------------------------

I landed this on default and aurora, given that we only run those tests for l10n builds on aurora. Andrei please watch the l10n results for today and tomorrow. But it should be fixed now. Thanks a lot!

http://hg.mozilla.org/qa/mozmill-tests/rev/c0159d65ac75 (default)
http://hg.mozilla.org/qa/mozmill-tests/rev/7a5e04700744 (aurora)