Closed Bug 1317284 Opened 7 years ago Closed 7 years ago

Intermittent toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html | Test timed out.

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: intermittent-bug-filer, Assigned: selee)

References

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;
58 bytes, text/x-review-board-request
MattN
: review+
jcristau
: approval-mozilla-beta+
Details
Blocks: 1289913
Flags: needinfo?(selee)
Assignee: nobody → selee
Status: NEW → ASSIGNED
Priority: -- → P1
I am trying to reproduce this at my local Linux x64 debug build now.
After talking with Sean I suggest he try to listen to this observer within restoreForm(), which re-construct the forms with innerHTML and triggers form fill.

http://searchfox.org/mozilla-central/rev/886d5186f0598ab2f8a9953eb5a4dab9750ef834/toolkit/components/passwordmgr/test/pwmgr_common.js#184-195
Thanks for Tim's suggestion. Here is the try link with the patch of "passwordmgr-processed-form" observer solution.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=478b2e0545282ea2b87695a922f109d1ac1bd1b6
Flags: needinfo?(selee)
Whiteboard: [test disabled on Linux]
Pushed by philringnalda@gmail.com:
https://hg.mozilla.org/mozilla-central/rev/14b5800c284e
disable test_password_field_autocomplete.html on Linux for constant failures, a=nnoyance
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

https://reviewboard.mozilla.org/r/93354/#review94422

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 1)
> -  Services.prefs.addObserver("security.insecure_field_warning.contextual.enabled",
> -                             this.updateWithPrefChange.bind(this), false);
> -
> -  Services.prefs.addObserver("signon.autofillForms.http",
> -                             this.updateWithPrefChange.bind(this), false);

It's unfortunate that this will leave a function (updateWithPrefChange) with one caller when it wasn't a function before.

::: toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html:114
(Diff revision 1)
>    let temp = form.innerHTML;
>    form.innerHTML = "";
>    form.innerHTML = temp;
> +
> +  yield new Promise(resolve => {
> +    var observer = SpecialPowers.wrapCallback(() => {

Nit: use `let`
Attachment #8811126 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

https://reviewboard.mozilla.org/r/93354/#review94424

::: toolkit/components/passwordmgr/test/mochitest/test_password_field_autocomplete.html:96
(Diff revision 1)
>  
>  </div>
>  
>  <pre id="test">
>  <script class="testbody" type="text/javascript">
>  

Don't forget to revert https://hg.mozilla.org/mozilla-central/rev/14b5800c284e too
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

Approval Request Comment
[Feature/Bug causing the regression]: Regression, bug 1289913.
[User impact if declined]: None.
[Is this code covered by automated tests?]: Yes.
[Has the fix been verified in Nightly?]: Yes.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: See dependency.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: This is a test script only fix that would fix a test that got merged into Aurora.
[String changes made/needed]: No.
Attachment #8811126 - Flags: approval-mozilla-aurora?
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

Hi MattN,

This patch is with the following changes:
1. Add pushPrefEnv for readonly and disabled cases.
2. Move readonly and disabled cases from the bottom to after test_form1_initial_empty. This can resolve another intermittent failed.

Could you review it again? Thanks.
Attachment #8811126 - Flags: review+ → review?(MattN+bmo)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #19)
> [Has the fix been verified in Nightly?]: Yes.

I'm confused.  This patch doesn't seem to have landed in nightly?
Flags: needinfo?(timdream)
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

My mistake. I was trying to fill the form in advanced so this don't get blocked. Let's re-request again only after it gets landed.
Flags: needinfo?(timdream)
Attachment #8811126 - Flags: approval-mozilla-aurora?
Is this ready to land Sean?  Or are you waiting on Matt's review?
Flags: needinfo?(selee)
Sorry, wrong bug.
Keywords: regressionwindow-wanted
Hi Tanvi, yes, I am waiting for MattN's review.

Hi MattN, could you help to review the patch again? There are some changes to fix another test failed since the last R+ you gave. Thanks.
Flags: needinfo?(selee) → needinfo?(MattN+bmo)
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

https://reviewboard.mozilla.org/r/93354/#review106502

::: toolkit/components/passwordmgr/LoginManagerContent.jsm
(Diff revision 4)
> -  this.searchString = aSearchString;
> -
> -  this._stringBundle = Services.strings.createBundle("chrome://passwordmgr/locale/passwordmgr.properties");
> -  this._dateAndTimeFormatter = new Intl.DateTimeFormat(undefined,
> -                              { day: "numeric", month: "short", year: "numeric" });
> -
> -  this._messageManager = messageManager;
> -  this._matchingLogins = matchingLogins;

This unnecessary code movement made the patch really hard to review and is why it took so long. I kept looking at the patch and not wanting to waste the time untangling what was happening here. In the future use a separate commit for significant code movement that's unrelated to the bug.
Attachment #8811126 - Flags: review?(MattN+bmo) → review+
Flags: needinfo?(MattN+bmo)
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [test disabled on Linux]
https://hg.mozilla.org/mozilla-central/rev/fabf199ceca6
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Please request Aurora approval on this when you get a chance. Also note that the patch will need rebasing for uplift.
Flags: needinfo?(selee)
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

Approval Request Comment
[Feature/Bug causing the regression]: bug 1289913
[User impact if declined]: Blocks uplift of other insecure password warning fixes/tests
[Is this code covered by automated tests?]: This is a test
[Has the fix been verified in Nightly?]: Not directly but it's mostly a test change and QA is regularly testing this feature now.
[Needs manual test from QE? If yes, steps to reproduce]:  No
[List of other uplifts needed for the feature/fix]: This blocks uplift of bug 1329351
[Is the change risky?]: No, 
[Why is the change risky/not risky?]: Non-test change is mostly refactoring to got back to an older method
[String changes made/needed]: None
Flags: needinfo?(selee)
Attachment #8811126 - Flags: approval-mozilla-aurora?
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

This now needs beta approval due to the merge.
Attachment #8811126 - Flags: approval-mozilla-beta?
Attachment #8811126 - Flags: approval-mozilla-aurora?
Comment on attachment 8811126 [details]
Bug 1317284 - Fix the intermittent failed in test_password_field_autocomplete.html.;

fix an intermittent in beta52, should be in b2
Attachment #8811126 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
As mentioned in comment 35, this needs rebasing for Beta uplift.
Flags: needinfo?(selee)
I'm rebasing now
Rebasing was only due to eslint changes.
Flags: needinfo?(selee)
You need to log in before you can comment on or make changes to this bug.