Closed Bug 1272849 Opened 8 years ago Closed 5 years ago

Intermittent Main action button is disabled - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger_empty_password.js :: test_empty_password

Categories

(Toolkit :: Password Manager, defect, P1)

Product:
Toolkit
Component:
Password Manager
Platform:
Desktop
All
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla72
Tracking Flags:
Tracking Status
firefox72 --- fixed

People

(Reporter: philor, Assigned: sfoster)

References

Details

(Keywords: intermittent-failure, Whiteboard: [stockwell disabled])

Attachments

(3 files, 1 obsolete file)

Bug 1272849 - skip browser_notification_2 test on linux to avoid intermittent;
58 bytes, text/x-review-board-request
MattN
: review+
Details
Bug 1272849 - disabled browser_notifications_2.js until a fix is available
47 bytes, text/x-phabricator-request
Details | Review
Bug 1272849 - Enable empty-password test and tighten up with shared helpers. r?MattN
47 bytes, text/x-phabricator-request
Details | Review
Blocks: 1217134
Flags: needinfo?(gasolin)
dup of bug 1275100
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(gasolin)
Resolution: --- → DUPLICATE
This is about an intermittent failure. Bug 1275100 is about the fact that the test will fail on merge day because of the signon.rememberSignons.visibilityToggle pref only being true on Nightly: https://mxr.mozilla.org/mozilla-central/source/modules/libpref/init/all.js?rev=cfda20b697dd#4196
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Can you work on this? This is marked as a dependency of one of your fixed bugs.
Flags: needinfo?(gasolin)
Assignee: nobody → gasolin
Flags: needinfo?(gasolin)
The bug seems related to bug 1227652
See Also: → 1227652
Since pyGTK error on linux is not resolved anytime soon, I'd like to disable the test on linux to avoid intermittent
Comment on attachment 8779190 [details]
Bug 1272849 - skip browser_notification_2 test on linux to avoid intermittent;

https://reviewboard.mozilla.org/r/70224/#review67510

The pyGTK issue is harmless (as the bug summary says) and isn't what's causing the orange. The issue is the "Main action button is disabled - false == true…" from the description.
Attachment #8779190 - Flags: review?(MattN+bmo) → review-
Can you please spend some time on this?
Flags: needinfo?(gasolin)
The origin browser_notification_2 related tests are fully skipped on linux, 
in patch bug 1217134 I separated browser_notification tests to browser_notification_1 and browser_notification_2, then removed the skip sentence because I can run test successfully both at local and treeherder.

Though now the browser_notification_2 still shows intermittent issues, I think a practical way is to add skipped="linux" back if we can't simply find the root cause which make test fail on linux.
Flags: needinfo?(gasolin) → needinfo?(MattN+bmo)
(In reply to Fred Lin [:gasolin] from comment #18)
> Though now the browser_notification_2 still shows intermittent issues, I
> think a practical way is to add skipped="linux" back if we can't simply find
> the root cause which make test fail on linux.

Fine with me though it's not 100% Linux it will at least reduce the volume.
Flags: needinfo?(MattN+bmo)
Comment on attachment 8779190 [details]
Bug 1272849 - skip browser_notification_2 test on linux to avoid intermittent;

Ask for review again based on comment 18
Attachment #8779190 - Flags: review- → review?(MattN+bmo)
Comment on attachment 8779190 [details]
Bug 1272849 - skip browser_notification_2 test on linux to avoid intermittent;

https://reviewboard.mozilla.org/r/70224/#review73222

::: toolkit/components/passwordmgr/test/browser/browser.ini:41
(Diff revision 1)
>    subtst_notifications_11_popup.html
>  [browser_username_select_dialog.js]
>  support-files =
>    subtst_notifications_change_p.html
>  [browser_DOMFormHasPassword.js]
>  [browser_DOMInputPasswordAdded.js]

Nit: You have a typo ("testt") in the commit message.

Thanks
Attachment #8779190 - Flags: review?(MattN+bmo) → review+
fixed commit message, thanks!
Keywords: checkin-needed
Fred, could you leave a comment after the skip-if statement and refer to this bug?

Given that this bug is not really being fixed, we should leave it open as well.
Flags: needinfo?(gasolin)
Keywords: leave-open
Thanks for remind, added a comment after skip-if sentence
Flags: needinfo?(gasolin)
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/ada110cf62d8
Skip browser_notification_2 test on linux to avoid intermittent. r=MattN
Keywords: checkin-needed
Bulk assigning P3 to all open intermittent bugs without a priority set in Firefox components per bug 1298978.
Priority: -- → P3
Assignee: gasolin → nobody

I guess we fixed it

Status: REOPENED → RESOLVED
Closed: 8 years ago → 5 years ago
Keywords: leave-open
Resolution: --- → FIXED

Still disabled from what I can see.

status-firefox49: affected → ---
Keywords: leave-open
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Over the last 7 days there are 76 failures present on this bug. These happen on osx-10-10, osx-10-10-shippable, windows10-64, windows10-64-ccov, windows10-64-shippable. windows10-aarch64, windows7-32, windows7-32-shippable.

Here is the most recent log example: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=241646343&repo=mozilla-inbound&lineNumber=7483

Flags: needinfo?(MattN+bmo)

Can you please disable the test for now? I thought my fix in https://hg.mozilla.org/integration/mozilla-inbound/rev/33f6d42d7fa92c72b4adb78cf1e6f19932266bf6 was the solution because it worked locally.

Flags: needinfo?(MattN+bmo) → needinfo?(shindli)

:mattn - submitted a patch for disabling this test on affected platforms.

:jmaher, sorry in advance for requesting a "data-review" but the simple "review" field did not have "?" :)

Assignee: nobody → shindli
Flags: needinfo?(shindli)
Attachment #9059694 - Flags: data-review?(jmaher)
Comment on attachment 9059694 [details] [diff] [review]
disable browser_notifications_2.js per mattn's request until he fixes it.

># HG changeset patch
># User Stefan Hindli <shindli@mozilla.com>
># Parent  72be82c6809e2cc187e5cffdff0c3e686c564a57
>|Bug 1272849 - disable browser_notifications_2.js per mattn's request until he fixes it r=jmaher|
>
>diff --git a/toolkit/components/passwordmgr/test/browser/browser.ini b/toolkit/components/passwordmgr/test/browser/browser.ini
>--- a/toolkit/components/passwordmgr/test/browser/browser.ini
>+++ b/toolkit/components/passwordmgr/test/browser/browser.ini
>@@ -73,6 +73,7 @@ skip-if = verify
> [browser_notifications_password.js]
> [browser_notifications_2.js]
> skip-if = os == "linux" # Bug 1272849 Main action button disabled state intermittent
>+skip-if = (os == "win" && os_version == 10.0 && bits = 64) || (os == "win" && os_version == 6.1 && !debug) (os == "osx" && !debug)
> [browser_openPasswordManager.js]
> [browser_passwordmgr_editing.js]
> skip-if = os == "linux"
Attachment #9059694 - Attachment is obsolete: true
Attachment #9059694 - Flags: data-review?(jmaher)
Assignee: shindli → nobody
Whiteboard: [stockwell disable-recommended] → [stockwell disabled]
Pushed by apavel@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ae52b1712e8d
disabled browser_notifications_2.js until a fix is available r=jmaher
Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7701a0d0fb8b
browser_notifications_2.js: Properly clear the password field in the doorhanger. r=intermittent
Status: REOPENED → NEW
Priority: P3 → P1

Here's some of the stuff I've tried to fix or at least diagnose what is going on here. In each case I've run the test using --verify several times with no local failures (Ubuntu / artifact build) before pushing to try:

This last one is especially mystifying as up to this point I had assumed we were dealing with a focus issue, with the keys being sent to the wrong window or element. But with this implementation password field value should be cleared and the test should pass with or without focus. So either waiting for the input event is incorrect and racy, or there's something else entirely going on here. Is waiting for popupshown really a definitive way to know the doorhanger is visible and interactive? Is the password input .value property read-only at some point?

Pushed by mozilla@noorenberghe.ca:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b29a467b6c66
Move test_empty_password to its own file. r=intermittent

I moved the one test task to browser_capture_doorhanger_empty_password.js so I could re-enable the rest of the file.

Summary: Intermittent browser_notifications_2.js | Main action button is disabled - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_notifications_2.js :: test_empty_password/< :: line 36 etc. → Intermittent Main action button is disabled - false == true - JS frame :: chrome://mochitests/content/browser/toolkit/components/passwordmgr/test/browser/browser_capture_doorhanger_empty_password.js :: test_empty_password
OS: Unspecified → All
Priority: P1 → P2
Hardware: Unspecified → Desktop
Priority: P2 → P1
Flags: qe-verify-

I have a patch that seems like it fixes this, but the try runs I've done all run into intermittent failures in test-verify (TV).
Such as: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13fc4f5a67bf9ea741b0b6f4b02139c1310a0bb2

I can get bug 1580326 to trigger using --verify locally (weird, the <login-filter> is just not there when it fails, some race in its dependencies?) I can't reproduce the failure in toolkit/components/passwordmgr/test/browser/browser_autocomplete_master_password.js. So I'm not sure yet if this is a real regression my patch might introduce or some other thing.

One of the patterns I see is popups being left in an indeterminate state between tests, which is an issue particularly when a test is waiting for a "showing" event but the popup is already shown. Sharing helpers and tidying up popups at the end of each task should help, but its a lot of changes and hard to measure their impact.

Assignee: nobody → sfoster
Status: NEW → ASSIGNED

(In reply to Sam Foster [:sfoster] (he/him) from comment #63)

One of the patterns I see is popups being left in an indeterminate state between tests, which is an issue particularly when a test is waiting for a "showing" event but the popup is already shown. Sharing helpers and tidying up popups at the end of each task should help, but its a lot of changes and hard to measure their impact.

I would support doing this in head.js' registerCleanupFunction for all relevant popups (autocomplete, doorhanger, context menu, etc.) to ensure that each test starts with an expected state.

(In reply to Sam Foster [:sfoster] (he/him) from comment #63)

I have a patch that seems like it fixes this, but the try runs I've done all run into intermittent failures in test-verify (TV).
Such as: https://treeherder.mozilla.org/#/jobs?repo=try&revision=13fc4f5a67bf9ea741b0b6f4b02139c1310a0bb2

I can get bug 1580326 to trigger using --verify locally (weird, the <login-filter> is just not there when it fails, some race in its dependencies?) I can't reproduce the failure in toolkit/components/passwordmgr/test/browser/browser_autocomplete_master_password.js. So I'm not sure yet if this is a real regression my patch might introduce or some other thing.

I believe TV only runs tests that were changed in the commit so I think you're only seeing those Tier 2 TV failures because they are changed and it's possible those issues already existed. You would have to do a TV try push without your patches to confirm. I would probably just land the patches as-is since they are tier 2 and won't cause a backout then we can fix any high-frequency failures after.

Pushed by sfoster@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4ebff107956a
Enable empty-password test and tighten up with shared helpers. r=MattN

Bugbug thinks this bug is a regression, but please revert this change in case of error.

Keywords: regression

Not a regression. The patch that landed should have fixed and re-enabled the test

Status: ASSIGNED → RESOLVED
Closed: 5 years ago → 5 years ago
Keywords: leave-open, regression
Resolution: --- → FIXED
status-firefox72: --- → fixed
Target Milestone: --- → mozilla72
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: