Fix misuse of fixIterator

RESOLVED FIXED in Thunderbird 40.0

Status

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: hiro, Assigned: hiro)

Tracking

Trunk
Thunderbird 40.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

There are a lots of:

for each (let element in fixIterator(xx))

This is mis-use actually, should be:

for (let element in fixIterator())

Now we got lots of test failures. 
See https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=1a581f0a2e36 for example.

Updated

4 years ago
Assignee: nobody → hiikezoe
There was a still misuse in mail/test/mozmill/cloudfile/test-cloudfile-add-account-dialog.js.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c953e2bd80d3
Created attachment 8538958 [details] [diff] [review]
Fix for calendar [checked in comment 31]
Attachment #8538468 - Attachment is obsolete: true
Attachment #8538958 - Flags: review?(philipp)
Created attachment 8538959 [details] [diff] [review]
Fix for mail and mailnews [checked in comment 31]
Attachment #8538469 - Attachment is obsolete: true
Attachment #8538959 - Flags: review?(mkmelin+mozilla)
Created attachment 8538961 [details] [diff] [review]
Fix for suite
Attachment #8538470 - Attachment is obsolete: true
Attachment #8538961 - Flags: review?(neil)
https://treeherder.mozilla.org/ui/#/jobs?repo=try-comm-central&revision=c953e2bd80d3
On windows debug build all xpcshell tests and all mozmill tests failed but I think it's another issue.
Comment on attachment 8538958 [details] [diff] [review]
Fix for calendar [checked in comment 31]

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

Thanks for the patch!
Attachment #8538958 - Flags: review?(philipp) → review+
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)
Comment hidden (Treeherder Robot)

Comment 30

4 years ago
Comment on attachment 8538959 [details] [diff] [review]
Fix for mail and mailnews [checked in comment 31]

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

::: mail/base/content/FilterListDialog.js
@@ +763,5 @@
>      // If it cannot, check all accounts to find a server
>      // that can have filters.
>      let allServers = MailServices.accounts.allServers;
> +    for (currentServer in fixIterator(allServers,
> +                                      Components.interfaces.nsIMsgIncomingServer))

let currentServer, while we're here
Attachment #8538959 - Flags: review?(mkmelin+mozilla) → review+

Comment 31

4 years ago
https://hg.mozilla.org/comm-central/rev/ddea495220c7
https://hg.mozilla.org/comm-central/rev/7a80103c70c5
Status: NEW → ASSIGNED
Summary: Fix mis use of fixIterator → Fix misuse of fixIterator
Target Milestone: --- → Thunderbird 37.0

Comment 33

4 years ago
So something must have changed in the JS core.

Comment 34

4 years ago
(In reply to :aceman from comment #33)
> So something must have changed in the JS core.
Probably Bug 783829

Comment 35

4 years ago
(From comment #32)
> Missed one instance in /calendar/
> http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/
> calendar-dialog-utils.js?rev=fa839fc34b66&mark=318-321#318
> 
> Missed one instance in /mail.
> http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.
> js?rev=7a80103c70c5&mark=2483-2484#2483
Flags: needinfo?(hiikezoe)
Created attachment 8539839 [details] [diff] [review]
Overlooked part [checked-in Comment 41]
Flags: needinfo?(hiikezoe)
Attachment #8539839 - Flags: review?(philip.chee)
(In reply to Philip Chee from comment #35)
> (From comment #32)
> > Missed one instance in /calendar/
> > http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/
> > calendar-dialog-utils.js?rev=fa839fc34b66&mark=318-321#318
> > 
> > Missed one instance in /mail.
> > http://mxr.mozilla.org/comm-central/source/mail/base/content/folderPane.
> > js?rev=7a80103c70c5&mark=2483-2484#2483

Great! I did check in mailnews, mail and calendar again, and found another one. I hope there is no misuse now.

Comment 38

4 years ago
Comment on attachment 8539839 [details] [diff] [review]
Overlooked part [checked-in Comment 41]

> Great! I did check in mailnews, mail and calendar again, and found another
> one. I hope there is no misuse now.
Good catch!
Attachment #8539839 - Flags: review?(philip.chee) → review+
Actually for (let x in fixIterator()) is also wrong, or, rather, it will be when bug 1123117 is fixed.

Updated

3 years ago
Attachment #8538958 - Attachment description: Fix for calendar → Fix for calendar [checked in comment 31]

Updated

3 years ago
Attachment #8538959 - Attachment description: Fix for mail and mailnews → Fix for mail and mailnews [checked in comment 31]

Comment 40

3 years ago
I'd say this bug is done if the failures are fixed by the patches.
Arguing whether for..of or for..in is better for fixIterator has its own bugs.
Keywords: checkin-needed
Whiteboard: [check in 2 remaining patches]
Version: unspecified → Trunk

Updated

3 years ago
Attachment #8539839 - Attachment description: Overlooked part → Overlooked part [checked-in Comment 41]

Updated

3 years ago
Whiteboard: [check in 2 remaining patches] → [check in 1remaining patch]

Updated

3 years ago
Whiteboard: [check in 1remaining patch] → [check in 1 remaining patch]

Updated

3 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [check in 1 remaining patch]
Target Milestone: Thunderbird 37.0 → Thunderbird 40.0
You need to log in before you can comment on or make changes to this bug.