Closed Bug 1202039 Opened 8 years ago Closed 8 years ago

Intermittent test_IE7_passwords.js | xpcshell return code: 0

Categories

(Firefox :: Migration, defect)

Product:
Firefox
Component:
Migration
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 43
Tracking Flags:
Tracking Status
firefox43 --- fixed

People

(Reporter: KWierso, Unassigned)

Details

(Keywords: intermittent-failure)

Attachments

(1 file)

Do not pretend to support IE7FormPasswords on Windows 8 and above
2.93 KB, patch
MattN
: review+
Details | Diff | Splinter Review 
This is not intermittent!  It's perma-fail on Windows 8, because of this code in the test:

288   if (AppConstants.isPlatformAndVersionAtLeast("win", "6.2")) {
289     Assert.throws(() => getFirstResourceOfType(MigrationUtils.resourceTypes.PASSWORDS),
290                   "The migrator doesn't exist for win8+");
291     return;
292   }

However, in IEProfileMigrator.js, we register IE7FormPasswords() unconditionally, so the test is checking for something that the code is not doing.
Comment on attachment 8660460 [details] [diff] [review]
Do not pretend to support IE7FormPasswords on Windows 8 and above

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

::: browser/components/migration/IEProfileMigrator.js
@@ +528,5 @@
>    , new Settings()
>    ];
> +  // Only support the password migrator for Windows XP to 7.
> +  if (AppConstants.isPlatformAndVersionAtMost("win", "6.1")) {
> +    resources.push(new IE7FormPasswords());

This comment is going to be inaccurate ("the password migrator") in the next few days as we're landing the Win8+ password migrator and then eventually an HTTP Auth password migrator. Can you rephrase this to remove the implication that there is only one password migrator?
Attachment #8660460 - Flags: review?(MattN+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/010b34c54ee2
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox43: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
(In reply to Ehsan Akhgari (don't ask for review please) from comment #89)
> This is not intermittent!  It's perma-fail on Windows 8

And yet, it is intermittent, in the sense that not every single "Windows 8 xpcshell" run has this test failing in it.

In fact, every instance of this bug has happened on one of fourteen out of our 172 active Win8 test slaves. So, are those fourteen the only ones which are actually identifying themselves as Win8, and the rest are running tests as though they were Win7?
(In reply to Phil Ringnalda (:philor) from comment #106)
> (In reply to Ehsan Akhgari (don't ask for review please) from comment #89)
> > This is not intermittent!  It's perma-fail on Windows 8
> 
> And yet, it is intermittent, in the sense that not every single "Windows 8
> xpcshell" run has this test failing in it.
> 
> In fact, every instance of this bug has happened on one of fourteen out of
> our 172 active Win8 test slaves. So, are those fourteen the only ones which
> are actually identifying themselves as Win8, and the rest are running tests
> as though they were Win7?

Could some machines be on 8.0 vs 8.1 (or 8.1u1 or 8.1u2)?
nthomas looked at t-w864-ix-022 which was hitting this, and t-w864-ix-023 which was not, and both claim a HKLM\SOFTWARE\Microsoft\Windows NT\CurrentVersion\CurrentVersion, which I think is what http://mxr.mozilla.org/mozilla-central/source/xpcom/base/nsSystemInfo.cpp#178 reads, of "6.2".

The good news for this bug is that the patch did fix the test for the affected slaves, one of them ran xpcshell after it landed and was green. The bad news is, the next person who does whatever the specific problem here was, some or all of "Services.sysinfo.version from xpcshell" I guess, is going to get bit by it only sometimes being 6.2 for Win8, unless we get rid of Win8 before the next person comes along.
You need to log in before you can comment on or make changes to this bug.