Closed
Bug 901409
Opened 11 years ago
Closed 11 years ago
Update password manager tests to prepare for bug 355063
Categories
(Toolkit :: Password Manager, defect)
Toolkit
Password Manager
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: Dolske, Assigned: Dolske)
References
Details
Attachments
(1 file, 1 obsolete file)
|
(deleted),
patch
|
MattN
:
review+
|
Details | Diff | Splinter Review |
A bunch of password manager tests generally look like:
<form>
...form fields...
</form>
<script>
...add logins...
window.onload = startTest
</script>
This works just fine and dandy in the current world, where password manager inherently runs via DOMContentLoaded, and so the inline scripts execute before that event (and thus the form fields are successfully filled with the logins added in the <script>).
Bug 355063 will be making password manager perform its duties during the DOMFormHasPassword event. This event can fire before DOMContentLoaded. Thus various tests need to initialize the test logins used _before_ the <form> occurs in the DOM. Otherwise password manager does its stuff before any logins have been added, and a bunch of tests fail.
This patch reorders the code run in tests to ensure that test logins have been initialized _before_ any form fields trigger password maanger's actions.
I also did some en passant cleanup to use SpecialPowers.Cc and .Ci a bit more consistently.
Note that these adjustments should work with or without the change in bug 355063.
|
Assignee | |
Comment 1•11 years ago
|
||
Need a little more complex changes to test_master_password.html... As the comment there already indicated: when DOMContentLoaded fires and pwmgr puts up a master passwrod prompt during formfill, the |load| event for for that iframe will also be blocked. Once the prompt is dismissed, the DOMContentLoaded stuff finishes and allows the |load| event to fire.
The test was relying on this, in that it added a load handler early on and expected it not to be called until after the master password prompt had been fiddled with. I changed this to instead just call those functions directly once the prompt is dismissed. [Well, actually, it's called indirectly via executeSoon to allow the stack to unwind so that the call triggering the master password prompt can actually complete.]
Test pass locally for me with the signon.useDOMFormPassword pref set to either true or false.
Pushed to Try: https://tbpl.mozilla.org/?tree=Try&rev=f4d2417daf79
Assignee: nobody → dolske
Attachment #785608 -
Attachment is obsolete: true
Attachment #786699 -
Flags: review?(mnoorenberghe+bmo)
|
|
Assignee | |
Comment 2•11 years ago
|
||
|
||
Comment 3•11 years ago
|
||
Comment on attachment 786699 [details] [diff] [review]
Patch v.2
Review of attachment 786699 [details] [diff] [review]:
-----------------------------------------------------------------
A cleanup idea (although maybe considered less clean) would be to move the Ci/Cc/Cu/Cr SpecialPower alias declarations to pwmgr_common.js. I'll leave it up to you.
r=me. All nits except for understanding how test_basic_form_html5.html could pass as-is.
::: toolkit/components/passwordmgr/test/test_basic_form.html
@@ +8,5 @@
> </head>
> <body>
> Login Manager test: simple form fill
> +
> +<script class="testbody" type="text/javascript">
Nit: you can nuke @type here (and maybe @class?) here
::: toolkit/components/passwordmgr/test/test_basic_form_html5.html
@@ +12,5 @@
> +commonInit();
> +SimpleTest.waitForExplicitFinish();
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
I'm confused about how this can pass without SpecialPowers when the enablePrivilege call is later.
::: toolkit/components/passwordmgr/test/test_basic_form_observer_autocomplete.html
@@ +11,5 @@
> +<script>
> +const Cc = SpecialPowers.Cc;
> +const Ci = SpecialPowers.Ci;
> +netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +const Cu = Components.utils;
Why not SpecialPowers.Cu for consistency? I think we could remove the enablePrivilege's then
::: toolkit/components/passwordmgr/test/test_basic_form_observer_autofillForms.html
@@ +14,5 @@
> +
> +const Cc = SpecialPowers.Cc;
> +const Ci = SpecialPowers.Ci;
> +netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +const Cu = Components.utils;
Ditto
::: toolkit/components/passwordmgr/test/test_basic_form_observer_foundLogins.html
@@ +14,5 @@
> +
> +const Cc = SpecialPowers.Cc;
> +const Ci = SpecialPowers.Ci;
> +netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> +const Cu = Components.utils;
Nit: SpecialPowers.Cu here too but I don't know if Components.Constructor works with SpecialPowers so we might still need the enablePrivilege in this file. Maybe just |new SpecialPower.wrap(Components).Constructor(…)|?
::: toolkit/components/passwordmgr/test/test_basic_form_pwevent.html
@@ +46,5 @@
> .getInterface(SpecialPowers.Ci.nsIWebNavigation)
> .QueryInterface(SpecialPowers.Ci.nsIDocShell)
> .chromeEventHandler.ownerDocument.defaultView;
> chromeWin.gBrowser.addEventListener("DOMFormHasPassword", checkForm);
> + // XXX SpecialPowers.addChromeEventListener instead?
I see that this file comes from bug 355063 so this change would be excluded if we land this first.
::: toolkit/components/passwordmgr/test/test_bug_427033.html
@@ +22,5 @@
> +
> +function startTest() {
> + checkForm(1, "jsuser", "jspass123");
> +
> + netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
Seems like enablePrivilege is no longer needed here?
::: toolkit/components/passwordmgr/test/test_bug_444968.html
@@ +12,5 @@
> +commonInit();
> +SimpleTest.waitForExplicitFinish();
> +
> +const Ci = Components.interfaces;
> +const Cc = Components.classes;
Nit: SpecialPowers could be used and enablePrivilege can probably be removed.
Attachment #786699 -
Flags: review?(mnoorenberghe+bmo) → review+
|
|
Assignee | |
Comment 4•11 years ago
|
||
(In reply to Matthew N. [:MattN] from comment #3)
> ::: toolkit/components/passwordmgr/test/test_basic_form.html
> @@ +8,5 @@
> > </head>
> > <body>
> > Login Manager test: simple form fill
> > +
> > +<script class="testbody" type="text/javascript">
>
> Nit: you can nuke @type here (and maybe @class?) here
Fixed this but left the other 124 alone. :/
>
> ::: toolkit/components/passwordmgr/test/test_basic_form_html5.html
> @@ +12,5 @@
> > +commonInit();
> > +SimpleTest.waitForExplicitFinish();
> > +
> > +const Ci = Components.interfaces;
> > +const Cc = Components.classes;
>
> I'm confused about how this can pass without SpecialPowers when the
> enablePrivilege call is later.
Hmmm. Good question. Maybe because commonInit() enables UniversalXPConnect? Although I thought that was function-scoped. Switched to SpecialPowers just to be safe.
> > +const Cc = SpecialPowers.Cc;
> > +const Ci = SpecialPowers.Ci;
> > +netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
> > +const Cu = Components.utils;
>
> Why not SpecialPowers.Cu for consistency? I think we could remove the
> enablePrivilege's then
I thought it was lacking, but I just checked again and it's not.
However, it doesn't seem to work. Even just replacing only the |Cu| assignment makes usage of XPCOMUtils throw later. So, leaving this as is.
> ::: toolkit/components/passwordmgr/test/test_bug_427033.html
> @@ +22,5 @@
> > +
> > +function startTest() {
> > + checkForm(1, "jsuser", "jspass123");
> > +
> > + netscape.security.PrivilegeManager.enablePrivilege('UniversalXPConnect');
>
> Seems like enablePrivilege is no longer needed here?
Fixed.
> ::: toolkit/components/passwordmgr/test/test_bug_444968.html
> @@ +12,5 @@
> > +commonInit();
> > +SimpleTest.waitForExplicitFinish();
> > +
> > +const Ci = Components.interfaces;
> > +const Cc = Components.classes;
>
> Nit: SpecialPowers could be used and enablePrivilege can probably be removed.
Fixed.
|
|
Assignee | |
Comment 5•11 years ago
|
||
|
||
Comment 6•11 years ago
|
||
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
You need to log in
before you can comment on or make changes to this bug.
Description
•