1269039 - Make test_master_password.html work with e10s

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Matthew N. [:MattN]]

test_master_password.html tests master password prompts but also deals with content iframes

Comment 1

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1269039 - Use add_task in test_master_password.html. r=dolske

Review commit: https://reviewboard.mozilla.org/r/49867/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49867/

Comment 2

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1269039 - Make test_master_password.html work with e10s. r=dolske

Review commit: https://reviewboard.mozilla.org/r/49869/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49869/

Comment 3

[Matthew N. [:MattN]]

Attachment: MozReview Request: Bug 1269039 - 2-space indentation and combine <script> for test_master_password.html. r=dolske

Review commit: https://reviewboard.mozilla.org/r/49871/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/49871/

Comment 4

[Justin Dolske [:Dolske]]

Comment on attachment 8747378 [details]
MozReview Request: Bug 1269039 - Use add_task in test_master_password.html. r=dolske

https://reviewboard.mozilla.org/r/49867/#review46739

::: toolkit/components/passwordmgr/test/mochitest/test_master_password.html:198
(Diff revision 1)
> -
> -function checkTest4A() {
> -    ok(true, "checkTest4A starting");
> -    ok(didDialog, "handleDialog was invoked");
>  
>      // check contents of iframe1 fields

Seems like there should really be something here to make sure the iframe finished loading, or else it will be intermittent. The old test didn't, but maybe it was just lucky?

Simple enough to add a "var prom = onloadPromiseFor('iframe1');" + yield here.

::: toolkit/components/passwordmgr/test/mochitest/test_master_password.html:211
(Diff revision 1)
>      // XXX check that there's 1 MP window open
>  
>      // Load another iframe with a login form
>      // This should detect that there's already a pending MP prompt, and not
> -    // put up a second one. The load event will fire (note that when pwmgr is
> -    // driven from DOMContentLoaded, if that blocks due to prompting for a MP,
> +    // put up a second one.
> +    var loadPromise = new Promise(resolve => {

Can use the onloadPromiseFor helper here too, fwiw.

Comment 5

[Justin Dolske [:Dolske]]

Comment on attachment 8747379 [details]
MozReview Request: Bug 1269039 - Make test_master_password.html work with e10s. r=dolske

https://reviewboard.mozilla.org/r/49869/#review46743

::: toolkit/components/passwordmgr/test/pwmgr_common.js:221
(Diff revision 1)
> -  var sdr = Cc["@mozilla.org/security/sdr;1"].getService(Ci.nsISecretDecoderRing);
> +    var sdr = Cc["@mozilla.org/security/sdr;1"].getService(Ci.nsISecretDecoderRing);
> -  sdr.logoutAndTeardown();
> +    sdr.logoutAndTeardown();
> +  });
> +}
> +
> +function isLoggedIn() {

This is another case where I (still) think it would be useful to have pwmgr / pwmgrCrypto "proxy" objects... With calls like pwmgrCrypto.isLoggedIn / pwmgr.getAllLogins() it would be clearer that they're proxying standard nsIXXX API calls to the implementation in the parent. (Ok, well, the name should be pwmgrProxy or something like that.)

Code cleanup for some other time, not a big deal here.

Comment 6

[Justin Dolske [:Dolske]]

Comment on attachment 8747380 [details]
MozReview Request: Bug 1269039 - 2-space indentation and combine <script> for test_master_password.html. r=dolske

https://reviewboard.mozilla.org/r/49871/#review46745

Comment 7

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49867/#review46739

> Seems like there should really be something here to make sure the iframe finished loading, or else it will be intermittent. The old test didn't, but maybe it was just lucky?
> 
> Simple enough to add a "var prom = onloadPromiseFor('iframe1');" + yield here.

Are you worried about the prompt appearing before the load event then we reach inside the frame to get the element values? I don't see how that would be a problem in practice since the password field has to be in the document in order to trigger the prompt in the first place.

> Can use the onloadPromiseFor helper here too, fwiw.

Good idea

Comment 8

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/49869/#review46743

> This is another case where I (still) think it would be useful to have pwmgr / pwmgrCrypto "proxy" objects... With calls like pwmgrCrypto.isLoggedIn / pwmgr.getAllLogins() it would be clearer that they're proxying standard nsIXXX API calls to the implementation in the parent. (Ok, well, the name should be pwmgrProxy or something like that.)
> 
> Code cleanup for some other time, not a big deal here.

Yeah, I agree but figured I'd tackle that in/after bug 1258912.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/0dbe09e2ce98
https://hg.mozilla.org/integration/fx-team/rev/c93e30cbbed8
https://hg.mozilla.org/integration/fx-team/rev/902965a6ae7a

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8f204eb163a3

Comment 11

[Carsten Book [:Tomcat]]

hey Matt, this still caused failures like https://treeherder.mozilla.org/logviewer.html#?job_id=9114473&repo=fx-team which where windows only :(

Comment 12

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/fx-team/rev/01fd8e3a44f6
https://hg.mozilla.org/integration/fx-team/rev/80a2253906fa
https://hg.mozilla.org/integration/fx-team/rev/3f6d2389313c
https://hg.mozilla.org/integration/fx-team/rev/dc5e2d41cf6d

Comment 13

[Justin Dolske [:Dolske]]

Screenshot shows the master password prompt open. Presumably it's from the chromeScript.sendSyncMessage("getAllLogins"), as expected... Is that call being synchronous somehow interfering with the ability of the parent to also watch for the prompt being shown?

But weird it's working on other platforms (and sometimes Windows too, other M(5) / M-e10s(5) runs were ok). Not sure what would be racing here.

Comment 14

[Matthew N. [:MattN]]

Anyone can feel free to finish this for me.

Comment 15

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #13)

Screenshot shows the master password prompt open. Presumably it's from the
chromeScript.sendSyncMessage("getAllLogins"), as expected... Is that call
being synchronous somehow interfering with the ability of the parent to also
watch for the prompt being shown?

But weird it's working on other platforms (and sometimes Windows too, other
M(5) / M-e10s(5) runs were ok). Not sure what would be racing here.

I'm hoping that https://hg.mozilla.org/mozilla-central/rev/7f26ccd5a281 of bug 1303167 fixed this…

I rebased and I'm doing a try push now: https://treeherder.mozilla.org/#/jobs?repo=try&revision=cea8283581d437a61d8fdb6e5586ecd47065f73c

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce50b241dbe2
Use add_task in test_master_password.html. r=dolske
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9282e1de3b7
Make test_master_password.html work with e10s. r=dolske
https://hg.mozilla.org/integration/mozilla-inbound/rev/c82a419aff34
2-space indentation, lint, and combine <script> for test_master_password.html. r=dolske

Comment 17

[Oana Pop-Rus]

https://hg.mozilla.org/mozilla-central/rev/ce50b241dbe2
https://hg.mozilla.org/mozilla-central/rev/f9282e1de3b7
https://hg.mozilla.org/mozilla-central/rev/c82a419aff34