Port bug 1484496 - Replace use of XPCOMUtils.IterSimpleEnumerator

RESOLVED FIXED in Thunderbird 63.0

Status

enhancement
--
blocker
RESOLVED FIXED
Last year
Last year

People

(Reporter: jorgk, Assigned: jorgk, Mentored)

Tracking

Trunk
Thunderbird 63.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 14 obsolete attachments)

47.65 KB, patch
kmag
: feedback+
Details | Diff | Splinter Review
1.26 KB, patch
Details | Diff | Splinter Review
1.35 KB, patch
aceman
: review+
Details | Diff | Splinter Review
1.50 KB, patch
aceman
: review+
Details | Diff | Splinter Review
9.48 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
1.46 KB, patch
aceman
: review+
Details | Diff | Splinter Review
24.60 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.15 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.42 KB, patch
jorgk
: review+
aceman
: review+
Details | Diff | Splinter Review
1.31 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
2.47 KB, patch
jorgk
: review+
Details | Diff | Splinter Review
37.17 KB, patch
darktrojan
: review+
Details | Diff | Splinter Review
Used in Gloda: mailnews/db/gloda/modules/index_msg.js
Posted patch WIP 1 (obsolete) — Splinter Review
Sadly this needed C++ changes. I did some random tweaks according to
https://hg.mozilla.org/integration/mozilla-inbound/rev/062e4138bfde6fb0f010d3fabb82b052b2a1b301 until it compiled.

But the gloda compaction test is still crashing (it was working with the change in this patch but before applying patches from bug 1484496):
\x07[25350, Main Thread] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file mozilla/obj-x86_64-pc-linux-gnu/dist/include/nsCOMPtr.h, line 436
Could not determine endianness of mozilla/obj-x86_64-pc-linux-gnu/dist/bin/libxul.so
#01: nsMsgBrkMBoxStore::AddSubFolders(nsIMsgFolder*, nsCOMPtr<nsIFile>&, bool) (mozilla/comm/mailnews/local/src/nsMsgBrkMBoxStore.cpp:1038)
#02: nsMsgBrkMBoxStore::DiscoverSubFolders(nsIMsgFolder*, bool) (mozilla/comm/mailnews/local/src/nsMsgBrkMBoxStore.cpp:68)
#03: nsMsgLocalMailFolder::GetSubFolders(nsISimpleEnumerator**) (mozilla/comm/mailnews/local/src/nsLocalMailFolder.cpp:221)
#04: nsMsgAccount::SetIncomingServer(nsIMsgIncomingServer*) (mozilla/comm/mailnews/base/src/nsMsgAccount.cpp:157)
#05: nsMsgAccountManager::CreateLocalMailAccount() (mozilla/comm/mailnews/base/src/nsMsgAccountManager.cpp:2354)

I didn't yet do all the changed from
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8c65dc566057853a19c477e0b30cd9e81d6326b
like setting the DefaultInterface and the new argument NS_NewArrayEnumerator. We have tons of those.
Attachment #9003674 - Flags: feedback?(jorgk)
(In reply to :aceman from comment #1)
> Sadly this needed C++ changes.
:-( - And I thought this was an easy one.

> I didn't yet do all the changed from
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> e8c65dc566057853a19c477e0b30cd9e81d6326b
> like setting the DefaultInterface and the new argument
> NS_NewArrayEnumerator. We have tons of those.
I counted about 30.
Comment on attachment 9003674 [details] [diff] [review]
WIP 1

OK. Will you keep working on it after the M-C merge today?

Where is nsISimpleEnumerator actually removed from M-C. I can see lots of changes replacing it, but I haven't seen the actual removal.
Attachment #9003674 - Flags: feedback?(jorgk) → feedback+
I don't see it removed in the m-c patches. It just gets new properties (like passing compilation and crashing at runtime:)) and C++ base classes, and it is replaced in many spots (but only in JS), but is not actually removed. Maybe that will come soon.

I can't work on it right now for the next few hours.
OK, this patch appears to be compiling, so I'll add the third argument to NS_NewArrayEnumerator() in mailnews/. Talk to me on IRC before continuing.
Posted patch 1485820.patch (obsolete) — Splinter Review
Added third arguments to NS_NewArrayEnumerator() except in nsAbOutlookDirectory.cpp and nsAbOSXDirectory.mm.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ac4e67316e166000d8feffff9c20746a91950e7f
Both tries look equally bad. The debug runs all crashed with
  application crashed [@ mozilla::storage::Service::Observe(nsISupports*, char const*, char16_t const*)]
which may be unrelated.

I tried two Xpcshell tests manually:
mach xpcshell-test comm/mailnews/addrbook/test/unit/test_collection.js
mach xpcshell-test comm/mailnews/addrbook/test/unit/test_nsIAbCard.js

Both fail with: ERROR NS_NOINTERFACE: Component does not have requested interface arg 0 [nsIAbDirectory.childCards]
on these lines:
Assert.ok(!collectChecker.AB.childCards.hasMoreElements());
var childCards = AB.childCards;

So clearly AB.childCards doesn't work.
Kris, looks like there is a whole lot more work than one call site in Gloda :-(

Could you take a look at 1485820.patch and tell us whether there's something obvious we're missing. Looks like most tests fail due to nsAbMDBDirectory::GetChildCards() which looks like this with the patch:

NS_IMETHODIMP nsAbMDBDirectory::GetChildCards(nsISimpleEnumerator* *result)
{
  nsresult rv;

  if (mIsQueryURI)
  {
    rv = StartSearch();
    NS_ENSURE_SUCCESS(rv, rv);

    // TODO
    // Search is synchronous so need to return
    // results after search is complete
    nsCOMPtr<nsIMutableArray> array(do_CreateInstance(NS_ARRAY_CONTRACTID));
    for (auto iter = mSearchCache.Iter(); !iter.Done(); iter.Next()) {
      array->AppendElement(iter.Data());
    }
    return NS_NewArrayEnumerator(result, array, NS_GET_IID(nsIAbCard));
  }

  rv = GetAbDatabase();

  if (NS_FAILED(rv) || !mDatabase)
    return rv;

  return m_IsMailList ? mDatabase->EnumerateListAddresses(this, result) :
                        mDatabase->EnumerateCards(this, result);
}

I tried
    for (auto iter = mSearchCache.Iter(); !iter.Done(); iter.Next()) {
      nsCOMPtr<nsIAbCard> card = do_QueryInterface(iter.Data());
      array->AppendElement(card);
    }
    return NS_NewArrayEnumerator(result, array, NS_GET_IID(nsIAbCard));
and it made no difference.
Flags: needinfo?(kmaglione+bmo)
Added some more of the DefaultInterface() and cleaned up the destructors and other changes according to the m-c patch.
Attachment #9003674 - Attachment is obsolete: true
Attachment #9003755 - Attachment is obsolete: true
Comment on attachment 9003875 [details] [diff] [review]
1485820-2.patch WIP 2 [landed in comment #13]

Looks like the address book tests pass now, but there are still heaps of tests failing:
Opt:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e7f45b58846be00a31a3df1734bc4fc1666a8dc8
Debug:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0b44592835831bf4974f17ae04744794e5b541f1
Attachment #9003875 - Flags: feedback?(kmaglione+bmo)
The opt try has these tests failures:
Xpcshell:
TEST-UNEXPECTED-FAIL | xpcshell-icaljs.ini:comm/calendar/test/unit/test_attendee.js | xpcshell return code: 0

Mozmill:
cloudfile\test-cloudfile-attachment-item.js
cloudfile\test-cloudfile-attachment-urls.js
cloudfile\test-cloudfile-notifications.js
folder-pane\test-folder-names-in-recent-mode.js
im\test-toolbar-buttons.js

I've run
mozmake SOLO_TEST=folder-pane/test-folder-names-in-recent-mode.js mozmill-one
and
mozmake SOLO_TEST=cloudfile/test-cloudfile-attachment-urls.js mozmill-one

Both fail in a similar way:
  EXCEPTION: JavaScript component does not have a method named: "Symbol.iterator"'JavaScript component does not have a method named: "Symbol.iterator"' when calling method: [nsISimpleEnumerator::Symbol.iterator]
    at: nonesuch line 72
       fixIterator/< iteratorUtils.jsm:72 1
       test_folder_names_in_recent_view_mode test-folder-names-in-recent-mode.js:27 5

and

  EXCEPTION: JavaScript component does not have a method named: "Symbol.iterator"'JavaScript component does not have a method named: "Symbol.iterator"' when calling method: [nsISimpleEnumerator::Symbol.iterator]
    at: nonesuch line 72
       fixIterator/< iteratorUtils.jsm:72 1
       attachToCloud/< MsgComposeCommands.js:1743 17

test-folder-names-in-recent-mode.js:27 and MsgComposeCommands.js:1743 both use fixIterator().

Removing these lines related to "Symbol.iterator":
https://searchfox.org/comm-central/rev/39cff66145fdfbfa785202c5b8d1c72a06034034/mailnews/base/util/iteratorUtils.jsm#63-76
fixes the first test and shifts the problem elsewhere in the other.

In summary, bug 1484496 busted C-C in many weird and not so wonderful ways :-(

Oh, the debug try seems completely broken with heaps of
PROCESS-CRASH | message-window | application crashed [@ mozilla::storage::Service::Observe(nsISupports*, char const*, char16_t const*)]
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1cba27a3d3e8
Port bug 1484496 to C-C: Changes to use of nsISimpleEnumerator. rs=jorgk,bustage-fix CLOSED TREE
The crash is this:
Storage connection not closed: blist.sqlite
Hit MOZ_CRASH() at c:/mozilla-source/comm-central/storage/mozStorageService.cpp:805
That's pretty much bug 1437323. So this might be a side effect of something going wrong earlier.
Aceman, this looks wrong:
https://hg.mozilla.org/comm-central/rev/1cba27a3d3e8d6c76f009bcac58ac1569ea2ca03#l21.37
Should the default be nsIMsgDatabase?

Geoff, could to look at the test_attendee.js failure for us.
Flags: needinfo?(geoff)
(In reply to Jorg K (GMT+2) from comment #12)
> Both fail in a similar way:
>   EXCEPTION: JavaScript component does not have a method named:
> "Symbol.iterator"'JavaScript component does not have a method named:
> "Symbol.iterator"' when calling method:
> [nsISimpleEnumerator::Symbol.iterator]

That generally means you have a JS-implemented nsISimpleEnumerator without a
[Symbol.iterator] method. It's only implemented automatically for C++ instances
which inherit from nsSimpleEnumerator. It looks like comm-central has a bunch of
JS-implemented instances, though.

In any case, they're generally pretty trivial to fix.
These ones?
https://searchfox.org/comm-central/search?q=ChromeUtils.generateQI.*nsISimpleEnumerator&case=false&regexp=true&path=*.js
Or how else do we find them? Those ones don't appear to be related to sub-folders or the failure at MsgComposeCommands.js:1743:
  let files = Array.from(fixIterator(fp.files, Ci.nsIFile))

I see you added on method here:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d6c4d5d3097c603e01e366129efccdb667c6254#l33.78
Yes, those ones.

(In reply to Jorg K (GMT+2) from comment #17)
> Or how else do we find them? Those ones don't appear to be related to
> sub-folders or the failure at MsgComposeCommands.js:1743:
>   let files = Array.from(fixIterator(fp.files, Ci.nsIFile))

My guess is it's one of these:

https://searchfox.org/comm-central/source/chat/modules/imXPCOMUtils.jsm#219-246

That said, you probably just want to replace that line with `Array.from(fp.files)`
Flags: needinfo?(kmaglione+bmo)
This does NOT fix the chat test im/test-toolbar-buttons.js which didn't show the Symbol.iterator problem anyway.
Kris, thanks for the suggestions, but the chat code you pointed at seems unrelated to the failures.

Removing the fixIterator() from MsgComposeCommands.js:1743 and using |Array.from(fp.files);| instead shifts the error to:

  EXCEPTION: JavaScript component does not have a method named: "Symbol.iterator"'JavaScript component does not have a method named: "Symbol.iterator"' when calling method: [nsISimpleEnumerator::Symbol.iterator]
    at: nonesuch line 1743
       attachToCloud/< MsgComposeCommands.js:1743 17

So no failure in fixIterator() any more since that call got removed. |Array.from(fp.files);| seems to be pure M-C code.
I also tried |for (let f of fp.files)| and got the same error.

That is used here:
https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/toolkit/mozapps/extensions/content/extensions.js#1245

Are you sure that's still working?
Flags: needinfo?(kmaglione+bmo)
That error means you still have a JS-implemented nsISimpleEnumerator without a [Symbol.iterator] method.
Flags: needinfo?(kmaglione+bmo)
Yes, thanks, you beat me to it, the "mock file picker", of course.
This fixes cloudfile/test-cloudfile-attachment-urls.js. All the tests pass, and then it crashes :-(
Attachment #9003937 - Flags: review?(acelists)
Comment on attachment 9003937 [details] [diff] [review]
1485820-gMockFilePicker.patch [landed in comment #28]

I looked for examples and build this patch based on:
https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/js/src/jit-test/tests/basic/spread-call-funapply.js#15
Attachment #9003937 - Flags: feedback?(kmaglione+bmo)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/b835cfa7443d
add [Symbol.iterator] method to gMockFilePicker.files. rs=bustage-fix CLOSED TREE
I don't understand why this class exists given that Map already has iterators, but whatever.
Flags: needinfo?(geoff)
Attachment #9003951 - Flags: review?(philipp)
Comment on attachment 9003937 [details] [diff] [review]
1485820-gMockFilePicker.patch [landed in comment #28]

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

::: mail/test/mozmill/shared-modules/test-attachment-helpers.js
@@ +77,5 @@
> +              return { value : self.returnFiles[this.i-1], done: false };
> +            else
> +              return { value : undefined, done: true };
> +          }
> +        }

Just:

  [Symbol.iterator]() {
    return self.returnFiles.values();
  }
Attachment #9003937 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 9003875 [details] [diff] [review]
1485820-2.patch WIP 2 [landed in comment #13]

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

Not sure all of these result types are correct. Someone who knows this code better than I do should have a closer look.

::: mail/components/shell/DirectoryProvider.h
@@ -48,2 @@
>    private:
> -    virtual ~AppendingEnumerator() {}

You need to keep this since the refcounting code will call the destructor on a base class.

::: mailnews/addrbook/src/nsAddrDatabase.cpp
@@ +2307,5 @@
> +    NS_DECL_ISUPPORTS_INHERITED
> +
> +    const nsID& DefaultInterface() override
> +    {
> +      return NS_GET_IID(nsIFile);

Pretty sure this is wrong. Maybe `nsIAbCard`, but I'm not sure all elements implement even that.

@@ -2479,4 @@
>      nsListAddressEnumerator(nsAddrDatabase* aDb, mdb_id aRowID);
>  
>  protected:
> -    ~nsListAddressEnumerator() {}

Need to keep this too.

::: mailnews/base/src/nsMailDirProvider.h
@@ -31,5 @@
>  
>      AppendingEnumerator(nsISimpleEnumerator* aBase);
>  
>    private:
> -    ~AppendingEnumerator() {}

Keep.

::: mailnews/db/gloda/modules/index_msg.js
@@ +1160,5 @@
> +    let headerIterator = function* (aEnumerator) {
> +      while (aEnumerator.hasMoreElements())
> +        yield aEnumerator.getNext().QueryInterface(nsIMsgDBHdr);
> +    };
> +    let headerIter = headerIterator(this._indexingEnumerator);

Shouldn't be necessary. Just `let headerIter = this._indexingEnumerator`, and make sure those enumerators are setup to return the correct types.

::: mailnews/db/msgdb/src/nsMsgThread.cpp
@@ -603,5 @@
>    nsMsgThreadEnumeratorFilter filter, void* closure);
>    int32_t MsgKeyFirstChildIndex(nsMsgKey inMsgKey);
>  
>  protected:
> -  virtual ~nsMsgThreadEnumerator();

Keep this. Can just make it `~nsMsgThreadEnumerator() override = default;` though.
Attachment #9003875 - Flags: feedback?(kmaglione+bmo) → feedback+
Thanks Kris!

Here a summary after two patches landed:

Issues:
=======
Comment #15:
https://hg.mozilla.org/comm-central/rev/1cba27a3d3e8d6c76f009bcac58ac1569ea2ca03#l21.37
Should the default be nsIMsgDatabase?

Comment #31:
Various issues in the C++ patch.

Comment #30:
Use:
  [Symbol.iterator]() {
    return self.returnFiles.values();
  }
I believe that's one of the variations I tried around 1 AM since I found it here:
https://searchfox.org/mozilla-central/rev/1410bb760a5e77236b74999807f5500bd285a57d/browser/components/preferences/in-content/main.js#2397
Trying it now, it works. I assume I had: |[Symbol.iterator]: function() {| which might be different.

Test failures:
==============
Xpcshell test: calendar/test/unit/test_attendee.js - addressed by Geoff's patch
Mozmill: folder-pane/test-folder-names-in-recent-mode.js - I think it's a [Symbol.iterator] problem.
  im/test-toolbar-buttons.js | test-toolbar-buttons.js

WIP issues:
===========
- NS_NewArrayEnumerator 3rd argument missing in in nsAbOutlookDirectory.cpp and nsAbOSXDirectory.mm.
- Maybe [Symbol.iterator] missing in for some JS-defined iterators, like in chat,
  see comment #18 and comment #19.

Debug build:
============
Nothing but crashes:
PROCESS-CRASH | ... | application crashed [@ mozilla::storage::Service::Observe(nsISupports*, char const*, char16_t const*)]
See bug 1437323.
Attachment #9003875 - Attachment description: 1485820-2.patch WIP 2 → 1485820-2.patch WIP 2 [landed in comment #13]
Comment on attachment 9003937 [details] [diff] [review]
1485820-gMockFilePicker.patch [landed in comment #28]

I'll fix it according to Kris' suggestion.
Attachment #9003937 - Attachment description: 1485820-gMockFilePicker.patch → 1485820-gMockFilePicker.patch [landed in comment #28]
Attachment #9003937 - Flags: review?(acelists)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/88e66ab7363e
add [Symbol.iterator] method to gMockFilePicker.files, take 2. rs=bustage-fix DONTBUILD CLOSED TREE
Attachment #9003965 - Attachment description: 1485820-gMockFilePicker-take2.patch → 1485820-gMockFilePicker-take2.patch [landed in comment #35]
This could be right, but it doesn't fix im/test-toolbar-buttons.js. The error still is:

SUMMARY-UNEXPECTED-FAIL | c:\mozilla-source\comm-central\comm\mail\test\mozmill\im\test-toolbar-buttons.js | test-toolbar-buttons.js::test_toolbar_and_placeholder
  EXCEPTION: the 'No conversation' placeholder is hidden
    at: test-folder-display-helpers.js line 2913
       assert_true test-folder-display-helpers.js:2913 11
       test_toolbar_and_placeholder test-toolbar-buttons.js:30 3

I've seen this |EXCEPTION: the 'No conversation' placeholder is hidden| in bug 1477636 comment #15 and it was totally unrelated.
Attachment #9003916 - Attachment is obsolete: true
Attachment #9003968 - Flags: feedback?(acelists)
Looking at
  mozmake SOLO_TEST=folder-pane/test-folder-names-in-recent-mode.js mozmill-one

Adding this code:
  for (let acc of fixIterator(MailServices.accounts.accounts, Ci.nsIMsgAccount)) {
    dump("===============================|"+acc.toString()+"|==============\n");
    if (acc.toString() == "[nsIMsgAccount: account3]") continue;
    for (let fld of fixIterator(acc.incomingServer.rootFolder.subFolders,
makes the test pass. The dump prints out three accounts, account1, account2, account3.

So it looks like one of those accounts provides .subFolders via JS. Hmm.
Yes, I ran "Troubleshooting information" while MozMill was going and got:
account3 - IRC - mozmilltest@irc.mozilla.invalid
Looks like I found it:
https://searchfox.org/comm-central/rev/a70a705dd7d87269d5cbbc54549eb1e115b42e16/mail/components/im/imIncomingServer.js#275
get subFolders() { return EmptyEnumerator; },

And that EmptyEnumerator is here, matching Kris' prediction from comment #19:
https://searchfox.org/comm-central/source/chat/modules/imXPCOMUtils.jsm#219-246
Attachment #9003968 - Attachment description: 1485820-Symbol-iterator-chat.patch → 1485820-Symbol-iterator-chat.patch (v2)
Attachment #9003968 - Attachment is obsolete: true
Attachment #9003968 - Flags: feedback?(acelists)
Adding [Symbol.iterator] to EmptyEnumerator fixes folder-pane/test-folder-names-in-recent-mode.js. im/test-toolbar-buttons.js is still not working, I guess the other [Symbol.iterator] I added are wrong. However, an error related to [Symbol.iterator] is not in the log for this test.

Kris, what's the story about .values? When can that be used? And is there a better way to do [Symbol.iterator] for EmptyEnumerator.
Attachment #9003975 - Flags: feedback?(kmaglione+bmo)
Attachment #9003975 - Flags: feedback?(acelists)
Also, when joining a chat:
JavaScript error: chrome://messenger/content/chat/joinchat.js, line 13: NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED: JavaScript component does not have a method named: "next"'JavaScript component does not have a method named: "next"' when calling method: [nsIJSEnumerator::next]
This adds [Symbol.iterator] to imXPCOMUtils.jsm. Tests pass and chat seems to work again. Landing this now.
Attachment #9003980 - Flags: review?(acelists)
Attachment #9003980 - Flags: feedback?(kmaglione+bmo)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/847b0ceab187
add [Symbol.iterator] in chat's imXPCOMUtils.jsm. rs=bustage-fix DONTBUILD
Here an updated summary after three patches landed:

Issues:
=======
Comment #15:
https://hg.mozilla.org/comm-central/rev/1cba27a3d3e8d6c76f009bcac58ac1569ea2ca03#l21.37
Should the default be nsIMsgDatabase?

Comment #31:
Various issues in the C++ patch, also in the Gloda JS snippet.

Test failures:
==============
Xpcshell test: calendar/test/unit/test_attendee.js - addressed by Geoff's patch

WIP issues:
===========
- NS_NewArrayEnumerator 3rd argument missing in in nsAbOutlookDirectory.cpp and nsAbOSXDirectory.mm.
- Maybe [Symbol.iterator] missing in for some JS-defined iterators,
  like in chat's logger.js, see attachment 9003975 [details] [diff] [review]

Debug build:
============
PROCESS-CRASH | ... | application crashed [@ mozilla::storage::Service::Observe(nsISupports*, char const*, char16_t const*)]
See bug 1437323. Maybe solved by the third patch, local MozMill doesn't show it any more.
Attachment #9003980 - Attachment description: 1485820-Symbol-iterator-chat.patch (v4) → 1485820-Symbol-iterator-chat.patch (v4) [landed in comment #43]
Yep, debug crashes all gone :-)
(In reply to Jorg K (GMT+2) from comment #15)
> Aceman, this looks wrong:
> https://hg.mozilla.org/comm-central/rev/
> 1cba27a3d3e8d6c76f009bcac58ac1569ea2ca03#l21.37
> Should the default be nsIMsgDatabase?

Why? It's GetNext() returns mResultHdr, which is nsIMsgDBHdr. The enumerator seemst or return message headers in a single database (it gets one in the constructor), not enumerate databases.
(In reply to Kris Maglione [:kmag] from comment #31)
> Comment on attachment 9003875 [details] [diff] [review]
> 1485820-2.patch WIP 2 [landed in comment #13]
> 
> Review of attachment 9003875 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Not sure all of these result types are correct. Someone who knows this code
> better than I do should have a closer look.
> 
> ::: mail/components/shell/DirectoryProvider.h
> @@ -48,2 @@
> >    private:
> > -    virtual ~AppendingEnumerator() {}
> 
> You need to keep this since the refcounting code will call the destructor on
> a base class.

Why? I went by https://hg.mozilla.org/integration/mozilla-inbound/rev/062e4138bfde6fb0f010d3fabb82b052b2a1b301 where you also removed these destructors that were empty? Won't the base destructor in nsSimpleEnumerator be run in this case?
 
> ::: mailnews/addrbook/src/nsAddrDatabase.cpp
> @@ +2307,5 @@
> > +    NS_DECL_ISUPPORTS_INHERITED
> > +
> > +    const nsID& DefaultInterface() override
> > +    {
> > +      return NS_GET_IID(nsIFile);
> 
> Pretty sure this is wrong. Maybe `nsIAbCard`, but I'm not sure all elements
> implement even that.

Yes, thanks.
 
> ::: mailnews/db/gloda/modules/index_msg.js
> @@ +1160,5 @@
> > +    let headerIterator = function* (aEnumerator) {
> > +      while (aEnumerator.hasMoreElements())
> > +        yield aEnumerator.getNext().QueryInterface(nsIMsgDBHdr);
> > +    };
> > +    let headerIter = headerIterator(this._indexingEnumerator);
> 
> Shouldn't be necessary. Just `let headerIter = this._indexingEnumerator`,
> and make sure those enumerators are setup to return the correct types.

Thanks, I can try.
(In reply to :aceman from comment #47)
> Why? It's GetNext() returns mResultHdr, which is nsIMsgDBHdr.
OK, just ignore what I said ;-)
(In reply to Jorg K (GMT+2) from comment #46)
> Yep, debug crashes all gone :-)
Spoke too early. The MozMill failures are all gone now, but Xpcshell test is full of them. Locally I see:

 0:00.94 pid:1404 [1404, Main Thread] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\\nsCOMPtr.h, line 436
 0:02.14 pid:1404 #01: nsMsgBrkMBoxStore::DiscoverSubFolders (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsMsgBrkMBoxStore.cpp:68)
 0:02.16 pid:1404 #02: nsMsgLocalMailFolder::GetSubFolders (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsLocalMailFolder.cpp:218)
 0:02.16 pid:1404 #03: nsMsgAccount::SetIncomingServer (c:\\mozilla-source\\comm-central\\comm\\mailnews\\base\\src\\nsMsgAccount.cpp:157)
 0:02.17 pid:1404 #04: nsMsgAccountManager::CreateLocalMailAccount (c:\\mozilla-source\\comm-central\\comm\\mailnews\\base\\src\\nsMsgAccountManager.cpp:2354)
> (In reply to Kris Maglione [:kmag] from comment #31)
> > You need to keep this since the refcounting code will call the destructor on
> > a base class.
> 
> Why? I went by
> https://hg.mozilla.org/integration/mozilla-inbound/rev/
> 062e4138bfde6fb0f010d3fabb82b052b2a1b301 where you also removed these
> destructors that were empty? Won't the base destructor in nsSimpleEnumerator
> be run in this case?

The most derived class still needs its own destructor so it can call the
destructors of its member variables. If you don't provide one, the compiler will
generate a default destructor anyway, but having an explicit protected
`override` destructor adds a sanity check that your base classes are still doing
what you think they are.

(In reply to Jorg K (GMT+2) from comment #41)
> Also, when joining a chat:
> JavaScript error: chrome://messenger/content/chat/joinchat.js, line 13:
> NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED: JavaScript component does not
> have a method named: "next"'JavaScript component does not have a method
> named: "next"' when calling method: [nsIJSEnumerator::next]

The object returned by [Symbol.iterator]() needs to have a next() method that does the Right Thing. That means you either need to return an existing JS iterator, or make [Symbol.iterator]() a star generator function.
Posted patch 1485820-fix destructors (obsolete) — Splinter Review
Thanks, is this better or should all of them have the '= default' clause?
Attachment #9004016 - Flags: feedback?(kmaglione+bmo)
(In reply to Kris Maglione [:kmag] from comment #51)
> (In reply to Jorg K (GMT+2) from comment #41)
> > Also, when joining a chat:
> The object returned by [Symbol.iterator]() needs to have a next() method
> that does the Right Thing. That means you either need to return an existing
> JS iterator, or make [Symbol.iterator]() a star generator function.
Fixed here: https://hg.mozilla.org/comm-central/rev/847b0ceab187, there's a f?kmag on the patch.
Comment on attachment 9004016 [details] [diff] [review]
1485820-fix destructors

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

(In reply to :aceman from comment #52)
> Thanks, is this better or should all of them have the '= default' clause?

Ideally, all of the destructors with empty bodies should have `override = default;` The mozilla-central linters will generally require that for any code a patch touches.
Attachment #9004016 - Flags: feedback?(kmaglione+bmo) → feedback+
Comment on attachment 9003980 [details] [diff] [review]
1485820-Symbol-iterator-chat.patch (v4) [landed in comment #43]

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

::: chat/modules/imXPCOMUtils.jsm
@@ +236,5 @@
>  
>      return this._items[this._nextIndex++];
>    },
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> +  [Symbol.iterator]: function () {

[Symbol.iterator]() {
    return this._items.values();
  }

@@ +260,5 @@
> +    return {
> +      next: function () {
> +        return { value : undefined, done: true };
> +      }
> +    }

This would be better as:

  * [Symbol.iterator]() {}

or something like:

  [Symbol.iterator]() {
    return [].values();
  }
Attachment #9003980 - Flags: feedback?(kmaglione+bmo)
Comment on attachment 9003975 [details] [diff] [review]
1485820-Symbol-iterator-chat.patch (v3)

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

::: chat/components/src/logger.js
@@ +470,5 @@
>        _messages: this._messages,
>        hasMoreElements: function() { return this._index < this._messages.length; },
>        getNext: function() { return new LogMessage(this._messages[this._index++], this._conv); },
> +      QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> +      [Symbol.iterator]() { return this._messages.values; }

`.values()`

It needs to be a function call. This just returns a function, which is not an enumerator.

Same for the others.

::: chat/modules/imXPCOMUtils.jsm
@@ +248,5 @@
> +    return {
> +      next: function () {
> +        return { value : undefined, done: true };
> +      }
> +    }

* [Symbol.iterator]() {}
Attachment #9003975 - Flags: feedback?(kmaglione+bmo)
OK, this corrects the suboptimal [Symbol.iterator] methods and adds more, all according to Kris' suggestions. BTW, |[Symbol.iterator]() {}| didn't work (EXCEPTION: [Symbol.iterator]() returned a non-object value), it has to be |[Symbol.iterator]() { return [].values(); }|.
Assignee: nobody → jorgk
Attachment #9003975 - Attachment is obsolete: true
Attachment #9003975 - Flags: feedback?(acelists)
Attachment #9004026 - Flags: review?(acelists)
(In reply to Jorg K (GMT+2) from comment #57)
> Created attachment 9004026 [details] [diff] [review]
> 1485820-Symbol-iterator-chat.patch
> 
> OK, this corrects the suboptimal [Symbol.iterator] methods and adds more,
> all according to Kris' suggestions. BTW, |[Symbol.iterator]() {}| didn't
> work (EXCEPTION: [Symbol.iterator]() returned a non-object value), it has to
> be |[Symbol.iterator]() { return [].values(); }|.

Not |[Symbol.iterator]() {}|, |* [Symbol.iterator]() {}|
Right, that's what you said, I misread this as bullet point/emphasis. I didn't want to ask, but is there any documentation on this. It might be easier to follow the documentation rather than be try/error or BMO dialogue.

While were here, do you think this is right:
@@ -666,33 +667,35 @@ DailyLogEnumerator.prototype = {
   _entries: {},
   _days: [],
   _index: 0,
   hasMoreElements: function() { return this._index < this._days.length; },
   getNext: function() {
     let dayID = this._days[this._index++];
     return new Log(this._entries[dayID]);
   },
-  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator])
+  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
+  [Symbol.iterator]() { return this._days.values(); }
 };

It shouldn't be: |return this._entries.values();| ?
Now using |* [Symbol.iterator]() {}|.
Attachment #9004026 - Attachment is obsolete: true
Attachment #9004026 - Flags: review?(acelists)
Attachment #9004027 - Flags: review?(acelists)
Attachment #9004026 - Attachment description: 1485820-Symbol-iterator-chat.patch → 1485820-Symbol-iterator-chat.patch (v5)
Comparing the Try run with Aceman's destructor fixes
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=cbaff77564c25cac3d1faa88b47b72af0dd94674
to a push without those fixes, there is no visible difference. Adding Linux' X1 and X2 crashes, I counted 99 in both cases.
Status: NEW → ASSIGNED
Attachment #9004016 - Attachment is obsolete: true
Attachment #9003980 - Flags: review?(acelists) → review+
Attachment #9004027 - Flags: review?(acelists) → review+
(In reply to Jorg K (GMT+2) from comment #50)
> (In reply to Jorg K (GMT+2) from comment #46)
> > Yep, debug crashes all gone :-)
> Spoke too early. The MozMill failures are all gone now, but Xpcshell test is
> full of them. Locally I see:
> 
>  0:00.94 pid:1404 [1404, Main Thread] ###!!! ASSERTION: QueryInterface
> needed: 'query_result.get() == mRawPtr', file
> c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\\nsCOMPtr.
> h, line 436
>  0:02.14 pid:1404 #01: nsMsgBrkMBoxStore::DiscoverSubFolders
> (c:\\mozilla-source\\comm-
> central\\comm\\mailnews\\local\\src\\nsMsgBrkMBoxStore.cpp:68)

You're going to need to change your GetDirectoryEntries callers to pass a nsIDirectoryEnumerator** argument rather than nsISimpleEnumerator**. Those enumerators now have 2 nsISimpleEnumerator vtables, and nsIDirectoryEnumerator casts to a different one than QueryInterface returns.

(In reply to Jorg K (GMT+2) from comment #59)
> Right, that's what you said, I misread this as bullet point/emphasis. I
> didn't want to ask, but is there any documentation on this. It might be
> easier to follow the documentation rather than be try/error or BMO dialogue.

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/function*

> While were here, do you think this is right:
> @@ -666,33 +667,35 @@ DailyLogEnumerator.prototype = {
>    _entries: {},
>    _days: [],
>    _index: 0,
>    hasMoreElements: function() { return this._index < this._days.length; },
>    getNext: function() {
>      let dayID = this._days[this._index++];
>      return new Log(this._entries[dayID]);
>    },
> -  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator])
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> +  [Symbol.iterator]() { return this._days.values(); }
>  };
> 
> It shouldn't be: |return this._entries.values();| ?

Presumably it should, yes.
Comment on attachment 9004027 [details] [diff] [review]
1485820-Symbol-iterator-chat.patch (v6)

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

::: chat/components/src/logger.js
@@ +672,5 @@
>      let dayID = this._days[this._index++];
>      return new Log(this._entries[dayID]);
>    },
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> +  [Symbol.iterator]() { return this._days.values(); }

Why not *[Symbol.iterator]() { while (this.hasMoreElements()) yield this.getNext(); } ?
Attachment #9004027 - Flags: review+ → review-
(In reply to :aceman from comment #64)
> Comment on attachment 9004027 [details] [diff] [review]
> 1485820-Symbol-iterator-chat.patch (v6)
> 
> Review of attachment 9004027 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: chat/components/src/logger.js
> @@ +672,5 @@
> >      let dayID = this._days[this._index++];
> >      return new Log(this._entries[dayID]);
> >    },
> > +  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> > +  [Symbol.iterator]() { return this._days.values(); }
> 
> Why not *[Symbol.iterator]() { while (this.hasMoreElements()) yield
> this.getNext(); } ?

Because this._entries.values() is much more efficient.
(In reply to Kris Maglione [:kmag] from comment #65)
Because entries.values() is much more efficient.

But where does it call 'new Log()' on each returned item (see getNext())?
Oh, BMO chat in real time ;-) - But if you look at the code I quoted in comment #59, we have:

   getNext: function() {
     let dayID = this._days[this._index++];
     return new Log(this._entries[dayID]);
   },
-  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator])
+  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
+  [Symbol.iterator]() { return this._days.values(); }

So iterating with getNext() doesn't return anything from the _days array, nor from the _entries object, bug instead |new Log(this._entries[dayID])|. So that's what the [Symbol.iterator] should also return, no?

So should I do one like:
return {
  self = this;
  i: -1,
  next: function () {
    i++;
    if (i < self._days.length)
      return { value: new Log(self._entries[self._days[this.i]]), done: false }
etc.

?
Also it looks like returning values of this._entries directly may change the returning order from this._entries[this._days[...]] .
(In reply to :aceman from comment #66)
> (In reply to Kris Maglione [:kmag] from comment #65)
> Because entries.values() is much more efficient.
> 
> But where does it call 'new Log()' on each returned item (see getNext())?

Ah, good catch. Yeah, your alternative should work, then.
Since we're still discussing logger.js, here's the reduced patch for imXPCOMUtils.jsm.
Attachment #9004027 - Attachment is obsolete: true
Comment on attachment 9004029 [details] [diff] [review]
1485820-fix destructors v1.1 [landed in comment #77]

Compiles for me and the defaults look right.
Attachment #9004029 - Flags: review?(jorgk) → review+
Comment on attachment 9004032 [details] [diff] [review]
1485820-Symbol-iterator-chat.patch (v6, reduced) [landed in comment #77]

This is uncontentious, I'll send another patch for logger.js soon.
Attachment #9004032 - Flags: review?(acelists)
(In reply to Kris Maglione [:kmag] from comment #63)
> (In reply to Jorg K (GMT+2) from comment #50)
> >  0:00.94 pid:1404 [1404, Main Thread] ###!!! ASSERTION: QueryInterface
> > needed: 'query_result.get() == mRawPtr', file
> > c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\\nsCOMPtr.
> > h, line 436
> >  0:02.14 pid:1404 #01: nsMsgBrkMBoxStore::DiscoverSubFolders
> > (c:\\mozilla-source\\comm-
> > central\\comm\\mailnews\\local\\src\\nsMsgBrkMBoxStore.cpp:68)
> 
> You're going to need to change your GetDirectoryEntries callers to pass a
> nsIDirectoryEnumerator** argument rather than nsISimpleEnumerator**. Those
> enumerators now have 2 nsISimpleEnumerator vtables, and
> nsIDirectoryEnumerator casts to a different one than QueryInterface returns.

Thanks, this seems to have helped on some crashes. Patch coming.
Comment on attachment 9004032 [details] [diff] [review]
1485820-Symbol-iterator-chat.patch (v6, reduced) [landed in comment #77]

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

OK, this could work.
Attachment #9004032 - Flags: review?(acelists) → review+
Comment on attachment 9004035 [details] [diff] [review]
1485820-fix GetDirectoryEntries calls [landed in comment #77]

Compiles for me and also fixes mailnews/base/test/unit/test_search.js.

Let's see how we fare.
Attachment #9004035 - Flags: review?(jorgk) → review+
https://hg.mozilla.org/comm-central/rev/12ec908cbece5a1503428ee2a24899044b6af902
add [Symbol.iterator] in chat's imXPCOMUtils.jsm, take 2. r=aceman
https://hg.mozilla.org/comm-central/rev/b21f4fc494c6cfcb25a58727345e0321c9f67044
Port bug 1484496 to C-C: Changes to use of nsISimpleEnumerator - fix destructors. r=jorgk
https://hg.mozilla.org/comm-central/rev/3d2c10e918d29353b29506c635c26e2e5f3cd2bb
Port bug 1484496 to C-C: Changes to use of nsISimpleEnumerator - fix GetDirectoryEntries() calls. r=jorgk

Pulsebot on strike?
Attachment #9004029 - Attachment description: 1485820-fix destructors v1.1 → 1485820-fix destructors v1.1 [landed in comment #77]
Attachment #9004032 - Attachment description: 1485820-Symbol-iterator-chat.patch (v6, reduced) → 1485820-Symbol-iterator-chat.patch (v6, reduced) [landed in comment #77]
Attachment #9004035 - Attachment description: 1485820-fix GetDirectoryEntries calls → 1485820-fix GetDirectoryEntries calls [landed in comment #77]
Attachment #9003965 - Flags: review?(acelists)
Yes, you were right, the previous suggestions were all wrong.
Attachment #9004038 - Flags: review?(acelists)
Comment on attachment 9004038 [details] [diff] [review]
1485820-Symbol-iterator-chat-logger.patch

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

::: chat/components/src/logger.js
@@ +672,5 @@
>      let dayID = this._days[this._index++];
>      return new Log(this._entries[dayID]);
>    },
> +  QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> +  [Symbol.iterator]: function () {

Please just make these |*[Symbol.iterator]() { while (this.hasMoreElements()) yield this.getNext(); }| as aceman suggested.
(In reply to Jorg K (GMT+2) from comment #77)
> Port bug 1484496 to C-C: Changes to use of nsISimpleEnumerator - fix
> GetDirectoryEntries() calls. r=jorgk

Wow, down to two crashes from 99 before :-):
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3MoveFilter.js | xpcshell return code: 1
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3MoveFilter.js | application crashed [@ mozalloc_abort]
TEST-UNEXPECTED-FAIL | comm/mailnews/local/test/unit/test_pop3FilterActions.js | xpcshell return code: 1
PROCESS-CRASH | comm/mailnews/local/test/unit/test_pop3FilterActions.js | application crashed [@ mozalloc_abort]

(In reply to Kris Maglione [:kmag] from comment #79)
> Please just make these |*[Symbol.iterator]() { while
> (this.hasMoreElements()) yield this.getNext(); }| as aceman suggested.
OK, will do. In the previous patch I had |* [Symbol.iterator]() { ...| with a space.
Attachment #9004038 - Attachment is obsolete: true
Attachment #9004038 - Flags: review?(acelists)
Attachment #9004039 - Flags: review?(acelists)
BTW, the two tests crash the same:

 0:01.16 pid:9028 [9028, Main Thread] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\\nsCOMPtr.h, line 436
 0:02.28 pid:9028 #01: nsMsgMaildirStore::RebuildIndex (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsMsgMaildirStore.cpp:1305)
 0:02.28 pid:9028 #02: nsMsgLocalMailFolder::ParseFolder (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsLocalMailFolder.cpp:190)
 0:02.28 pid:9028 #03: nsMsgLocalMailFolder::GetDatabaseWithReparse (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsLocalMailFolder.cpp:387)

Kris, what's you tip for this one?
(In reply to Jorg K (GMT+2) from comment #82)
> BTW, the two tests crash the same:
> 
>  0:01.16 pid:9028 [9028, Main Thread] ###!!! ASSERTION: QueryInterface
> needed: 'query_result.get() == mRawPtr', file
> c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\\nsCOMPtr.
> h, line 436
>  0:02.28 pid:9028 #01: nsMsgMaildirStore::RebuildIndex
> (c:\\mozilla-source\\comm-
> central\\comm\\mailnews\\local\\src\\nsMsgMaildirStore.cpp:1305)
>  0:02.28 pid:9028 #02: nsMsgLocalMailFolder::ParseFolder
> (c:\\mozilla-source\\comm-
> central\\comm\\mailnews\\local\\src\\nsLocalMailFolder.cpp:190)
>  0:02.28 pid:9028 #03: nsMsgLocalMailFolder::GetDatabaseWithReparse
> (c:\\mozilla-source\\comm-
> central\\comm\\mailnews\\local\\src\\nsLocalMailFolder.cpp:387)
> 
> Kris, what's you tip for this one?

Same deal. You need to pass a nsIDirectoryEnumerator** pointer to GetDirectoryEntries.
Yes, I think so too, there is a m_DirectoryEnumerator that I missed to convert.
Comment on attachment 9004039 [details] [diff] [review]
1485820-Symbol-iterator-chat-logger.patch (v2)

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

::: chat/components/src/logger.js
@@ +470,5 @@
>        _messages: this._messages,
>        hasMoreElements: function() { return this._index < this._messages.length; },
>        getNext: function() { return new LogMessage(this._messages[this._index++], this._conv); },
> +      QueryInterface: ChromeUtils.generateQI([Ci.nsISimpleEnumerator]),
> +      [Symbol.iterator]() { return this._messages.values(); }

This seems to miss the LogMessage() on each element, so possibly just use '* [Symbol.iterator]() { while (this.hasMoreElements()) yield this.getNext(); }' again
Attachment #9004039 - Flags: review?(acelists)
Attachment #9003965 - Flags: review?(acelists) → review+
For comment 31, did you mean this? The called methods needed changing for nsISimpleEnumerator instead of JS generator.
Attachment #9004041 - Flags: feedback?(kmaglione+bmo)
Mentor: kmaglione+bmo
Severity: normal → blocker
OS: Unspecified → All
Hardware: Unspecified → All
Comment on attachment 9004041 [details] [diff] [review]
1485820-simplify gloda enumerator

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

::: mailnews/db/gloda/modules/index_msg.js
@@ +1156,5 @@
>      // for GC reasons we need to track the number of headers seen
>      let numHeadersSeen = 0;
>  
>      // We are consuming two lists; our loop structure has to reflect that.
> +    let headerIter = this._indexingEnumerator;

I'd probably go with `let headerIter = this._indexingEnumerator[Symbol.iterator]();` and keep the old structure below so you can avoid the QueryInterface.
Attachment #9004041 - Flags: feedback?(kmaglione+bmo) → feedback+
Comment on attachment 9003951 [details] [diff] [review]
1485820-property-map-iterator-1.diff

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

This was neccesary to make some IDL interfaces happy, e.g. https://searchfox.org/comm-central/source/calendar/base/public/calIAttendee.idl#60 . I believe at the time, [symbol] wasn't part of IDL. While we are making changes here I'd prefer if we could revise all the propertyEnumerators to be able to return mProperties directly, and get rid of PropertyMap.

Either way, I'm wondering why it is neccessary to add [Symbol.iterator] here, given the only way we iterate the properties is through propertyEnumerator or .entries() and it inherits the iterator of Map.
Attachment #9003951 - Flags: review?(philipp) → review-
Comment on attachment 9004042 [details] [diff] [review]
1485820-fix m_directoryEnumerator [landed in comment #92]

Thanks.
Attachment #9004042 - Flags: review?(jorgk) → review+
That should be it then.
Attachment #9004039 - Attachment is obsolete: true
Attachment #9004056 - Flags: review+
https://hg.mozilla.org/comm-central/rev/c954ffb4d86e57efdc9bcdf38ae1fdb4a14ab77d
Bug 1485820 - add [Symbol.iterator] in chat's logger.js. r=aceman
https://hg.mozilla.org/comm-central/rev/0a1a8c41d5fbadeceac3dc6ece0544d9bbf0b042
Port bug 1484496 to C-C: Changes to use of nsISimpleEnumerator - fix GetDirectoryEntries() calls, follow-up. r=jorgk DONTBUILD

Pulsebot still on strike.

Two remarks:
- M-C is inconsistent with the |* [Symbol.iterator]| vs. |*[Symbol.iterator]|,
  but there are more occurrences with the space.
- I caused the test failure in test_imapSearch.js, bug 678322, I'll take care of it.
Attachment #9004042 - Attachment description: 1485820-fix m_directoryEnumerator → 1485820-fix m_directoryEnumerator [landed in comment #92]
Attachment #9004056 - Attachment description: 1485820-Symbol-iterator-chat-logger.patch (v3) → 1485820-Symbol-iterator-chat-logger.patch (v3) [landed in comment #92]
OK, seems to work too.
Attachment #9004041 - Attachment is obsolete: true
Attachment #9004058 - Flags: review?(jorgk)
Attachment #9004056 - Flags: review+
Comment on attachment 9004058 [details] [diff] [review]
1485820-simplify gloda enumerator v2 [landed in comment #107]

I usually get the M-C person who made the suggestion give the r+, like Boris (:bz), Masayuki, Nick (:njn) or others. I don't want to pretend that I have the knowledge for a qualified review here. Yes, I reviewed that you followed the suggestion ;-)
Attachment #9004058 - Flags: review?(jorgk) → review+
https://hg.mozilla.org/comm-central/rev/66296c90046564a6b068ffdfe6bf92dc071464e8
add [Symbol.iterator] in chat's logger.js, fix spurious }. rs=bustage-fix DONTBUILD

Sigh, from the latest push:
SyntaxError: expected expression, got '}' at /Users/cltbld/tasks/task_1535279251/build/tests/xpcshell/tests/comm/chat/components/src/test/test_logger.js -> resource:///components/logger.js:677
(In reply to Jorg K (GMT+2) from comment #92)
> - M-C is inconsistent with the |* [Symbol.iterator]| vs.
> |*[Symbol.iterator]|,
>   but there are more occurrences with the space.

Our ESLint rules require the space, but ESLint isn't enabled everywhere.
Thanks, I landed our patches with the spaces since there are more cases on M-C with the space.
Posted patch 1485820-simplify fixIterator (obsolete) — Splinter Review
Kris, I changed our fixIterator helper to return the new iterator of nsISimpleEnumerator directly instead of open-coding the loop.
Can this be worth anything, may it be faster?

Try run: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7feb54dee957874ee3a819283f95381c93c51baf
Attachment #9004099 - Flags: review?(jorgk)
Attachment #9004099 - Flags: feedback?(kmaglione+bmo)
Jorg, there still seems to be a last crash in comm/mailnews/local/test/unit/test_nsIMsgLocalMailFolder.js test on Windows debug only. Can you confirm it?
Wow, the 100th comment is mine :-)

Confirmed:
 0:01.08 pid:4272 [4272, Main Thread] ###!!! ASSERTION: QueryInterface needed: 'query_result.get() == mRawPtr', file c:/mozilla-source/comm-central/obj-x86_64-pc-mingw32/dist/include\\nsCOMPtr.h, line 436
 0:02.23 pid:4272 #01: nsMsgBrkMBoxStore::RenameFolder (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsMsgBrkMBoxStore.cpp:398)
 0:02.23 pid:4272 #02: nsMsgLocalMailFolder::Rename (c:\\mozilla-source\\comm-central\\comm\\mailnews\\local\\src\\nsLocalMailFolder.cpp:898)

Thanks for checking thoroughly.
I think I know what is the problem, I'll file a m-c bug for it.
The call stack on sever actually is:
#01: nsLocalFile::CopyMove(nsIFile *,nsTSubstring<char16_t> const &,unsigned int) [xpcom/io/nsLocalFileWin.cpp:2049]
#02: nsLocalFile::MoveTo(nsIFile *,nsTSubstring<char16_t> const &) [xpcom/io/nsLocalFileWin.cpp:2160]
#03: nsMsgBrkMBoxStore::RenameFolder(nsIMsgFolder *,nsTSubstring<char16_t> const &,nsIMsgFolder * *) [comm/mailnews/local8]
#04: nsMsgLocalMailFolder::Rename(nsTSubstring<char16_t> const &,nsIMsgWindow *) [comm/mailnews/local/src/nsLocalMailFolder.cpp:898]

And that line is a nsIFile->GetDirectoryEntries(). Filed bug 1486311.
Comment on attachment 9004099 [details] [diff] [review]
1485820-simplify fixIterator

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

(In reply to :aceman from comment #98)
> Kris, I changed our fixIterator helper to return the new iterator of
> nsISimpleEnumerator directly instead of open-coding the loop.
> Can this be worth anything, may it be faster?

It should be faster, yes. Although I'd generally recommend just removing uses of fixIterator for these cases. That's the reason we made this change to begin with.
Attachment #9004099 - Flags: feedback?(kmaglione+bmo) → feedback+
Thanks.
Yes when you know you have a nsISimpleEnumerator, you can now for...of it directly. But we use fixIterator when it is useful to not care what kind of array/iterator you get from the backend.
Next time you could remove properties (this time you added them) so that the object wouldn't be iterable and fixIterator could again abstract that away from the caller without any changed to the caller.
Comment on attachment 9004099 [details] [diff] [review]
1485820-simplify fixIterator

I'm the wrong reviewer here, but I'll add f=kmag to the commit message.
Attachment #9004099 - Flags: review?(jorgk) → review+
Let's lose all the unnecessary parenthesis.
Attachment #9004099 - Attachment is obsolete: true
Attachment #9004103 - Flags: review+
https://hg.mozilla.org/comm-central/rev/1ea73fb2c8074737254c6fad49214e435dd7a6a8
temporarily switch off test_attendee.js. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/c2eb4f61f764ff4b28c83f1ea20858a9777a5c3a
Follow-up: simplify enumeration in GlodaMsgIndexer::_worker_folderCompactionPass. f=kmag, rs=jorgk
https://hg.mozilla.org/comm-central/rev/ce828f6cf25697596ec928ee99dfc07bdf0ce006
Bug 1485820 - Port bug 1484496 to C-C: Simplify nsISimpleEnumerator handling in fixIterator(). f=kmag, rs=jorgk

Sorry Geoff, I switched off the failing test_attendee.js. Please revert that when you submit a new patch.
Attachment #9004058 - Attachment description: 1485820-simplify gloda enumerator v2 → 1485820-simplify gloda enumerator v2 [landed in comment #107]
Attachment #9004103 - Attachment description: 1485820-simplify fixIterator (v1.1) → 1485820-simplify fixIterator (v1.1) [landed in comment #107]
All mail/mailnews work is done here, we're just missing the Calendar patch and bug 1486311.
Keywords: leave-open
Attachment #9003951 - Attachment is obsolete: true
Attachment #9004115 - Flags: review?(philipp)
Depends on: 1486311
Comment on attachment 9004115 [details] [diff] [review]
1485820-property-map-iterator-2.diff

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

Thanks, r+wc. I'm slightly concerned about backwards compat for add-ons, but then again if we document this change it should be fine. I kind of expected the idl to be in a way that |properties| returns something that is or behaves like a Map(), so that you could just have |get properties| return |mProperties|, and the for loop be for (let [name, value] of alarm.properties.entries()) {} but this works as well. 

I think Jörg already started a guide, maybe you could add a note to that?

::: calendar/base/public/calIItemBase.idl
@@ +2,5 @@
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  
>  #include "nsISupports.idl"
> +#include "nsISimpleEnumerator.idl"

I don't think you need to #include, a forward define should be sufficient?
Attachment #9004115 - Flags: review?(philipp) → review+
> I don't think you need to #include, a forward define should be sufficient?

You're right, I don't know what I was thinking there.

Fixed that and a comment which still referred to nsISimpleEnumerator.
Attachment #9004115 - Attachment is obsolete: true
Attachment #9004393 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7c90cbde9cdb
Remove PropertyMap and instead return a simple iterator for item properties. r=philipp
https://hg.mozilla.org/comm-central/rev/22b625dc6af3
Backed out changeset 1ea73fb2c807 to re-enable test. a=backout
Status: ASSIGNED → RESOLVED
Closed: Last year
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 63.0
You need to log in before you can comment on or make changes to this bug.