1485820 - Port bug 1484496 - Replace use of XPCOMUtils.IterSimpleEnumerator

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[Jorg K (CEST = GMT+2)]

Used in Gloda: mailnews/db/gloda/modules/index_msg.js

Comment 1

[:aceman]

Attachment: WIP 1

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.

Comment 2

[Jorg K (CEST = GMT+2)]

(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 3

[Jorg K (CEST = GMT+2)]

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.

Comment 4

[:aceman]

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.

Comment 5

[Jorg K (CEST = GMT+2)]

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.

Comment 6

[Jorg K (CEST = GMT+2)]

With the patch I can already compile, so we'll see:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=d43cb4dc77c471cf0182d337a4db2f018fda9693

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: 1485820.patch

Added third arguments to NS_NewArrayEnumerator() except in nsAbOutlookDirectory.cpp and nsAbOSXDirectory.mm.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=ac4e67316e166000d8feffff9c20746a91950e7f

Comment 8

[Jorg K (CEST = GMT+2)]

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.

Comment 9

[Jorg K (CEST = GMT+2)]

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.

Comment 10

[:aceman]

Attachment: 1485820-2.patch WIP 2 [landed in comment #13]

Added some more of the DefaultInterface() and cleaned up the destructors and other changes according to the m-c patch.

Comment 11

[Jorg K (CEST = GMT+2)]

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

Comment 12

[Jorg K (CEST = GMT+2)]

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*)]

Comment 13

[Pulsebot]

Pushed by [e-mail]:
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

Comment 14

[Jorg K (CEST = GMT+2)]

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.

Comment 15

[Jorg K (CEST = GMT+2)]

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.

Comment 16

[Kris Maglione [:kmag]]

(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.

Comment 17

[Jorg K (CEST = GMT+2)]

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

Comment 18

[Jorg K (CEST = GMT+2)]

https://searchfox.org/comm-central/search?q=ChromeUtils.generateQI.*nsISimpleEnumerator&regexp=true&path=*.js
What happened there. This links works.

Comment 19

[Kris Maglione [:kmag]]

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)`

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch

This does NOT fix the chat test im/test-toolbar-buttons.js which didn't show the Symbol.iterator problem anyway.

Comment 21

[Jorg K (CEST = GMT+2)]

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.

Comment 22

[Jorg K (CEST = GMT+2)]

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?

Comment 23

[Kris Maglione [:kmag]]

That error means you still have a JS-implemented nsISimpleEnumerator without a [Symbol.iterator] method.

Comment 24

[Kris Maglione [:kmag]]

Presumably this one:

https://dxr.mozilla.org/comm-central/source/mail/test/mozmill/shared-modules/test-attachment-helpers.js#62

Comment 25

[Jorg K (CEST = GMT+2)]

Yes, thanks, you beat me to it, the "mock file picker", of course.

Comment 26

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-gMockFilePicker.patch [landed in comment #28]

This fixes cloudfile/test-cloudfile-attachment-urls.js. All the tests pass, and then it crashes :-(

Comment 27

[Jorg K (CEST = GMT+2)]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b835cfa7443d
add [Symbol.iterator] method to gMockFilePicker.files. rs=bustage-fix CLOSED TREE

Comment 29

[Geoff Lankow (:darktrojan)]

Attachment: 1485820-property-map-iterator-1.diff

I don't understand why this class exists given that Map already has iterators, but whatever.

Comment 30

[Kris Maglione [:kmag]]

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();
  }

Comment 31

[Kris Maglione [:kmag]]

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.

Comment 32

[Jorg K (CEST = GMT+2)]

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.

Comment 33

[Jorg K (CEST = GMT+2)]

Comment on attachment 9003937 [details] [diff] [review]
1485820-gMockFilePicker.patch [landed in comment #28]

I'll fix it according to Kris' suggestion.

Comment 34

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-gMockFilePicker-take2.patch [landed in comment #35]

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/88e66ab7363e
add [Symbol.iterator] method to gMockFilePicker.files, take 2. rs=bustage-fix DONTBUILD CLOSED TREE

Comment 36

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch (v2)

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.

Comment 37

[Jorg K (CEST = GMT+2)]

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.

Comment 38

[Jorg K (CEST = GMT+2)]

Yes, I ran "Troubleshooting information" while MozMill was going and got:
account3 - IRC - [e-mail]

Comment 39

[Jorg K (CEST = GMT+2)]

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

Comment 40

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch (v3)

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.

Comment 41

[Jorg K (CEST = GMT+2)]

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]

Comment 42

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch (v4) [landed in comment #43]

This adds [Symbol.iterator] to imXPCOMUtils.jsm. Tests pass and chat seems to work again. Landing this now.

Comment 43

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/847b0ceab187
add [Symbol.iterator] in chat's imXPCOMUtils.jsm. rs=bustage-fix DONTBUILD

Comment 44

[Jorg K (CEST = GMT+2)]

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.

Comment 45

[Jorg K (CEST = GMT+2)]

Debug try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=be1f095aa4bd7031afd001902994f3f1ede2dbbd

Comment 46

[Jorg K (CEST = GMT+2)]

Yep, debug crashes all gone :-)

Comment 47

[:aceman]

(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.

Comment 48

[:aceman]

(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.

Comment 49

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #47)
> Why? It's GetNext() returns mResultHdr, which is nsIMsgDBHdr.
OK, just ignore what I said ;-)

Comment 50

[Jorg K (CEST = GMT+2)]

(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)

Comment 51

[Kris Maglione [:kmag]]

> (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.

Comment 52

[:aceman]

Attachment: 1485820-fix destructors

Thanks, is this better or should all of them have the '= default' clause?

Comment 53

[Jorg K (CEST = GMT+2)]

(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 54

[Kris Maglione [:kmag]]

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.

Comment 55

[Kris Maglione [:kmag]]

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();
  }

Comment 56

[Kris Maglione [:kmag]]

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]() {}

Comment 57

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch (v5)

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(); }|.

Comment 58

[Kris Maglione [:kmag]]

(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]() {}|

Comment 59

[Jorg K (CEST = GMT+2)]

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();| ?

Comment 60

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch (v6)

Now using |* [Symbol.iterator]() {}|.

Comment 61

[Jorg K (CEST = GMT+2)]

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.

Comment 62

[:aceman]

Attachment: 1485820-fix destructors v1.1 [landed in comment #77]

Comment 63

[Kris Maglione [:kmag]]

(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 64

[:aceman]

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(); } ?

Comment 65

[Kris Maglione [:kmag]]

(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.

Comment 66

[:aceman]

(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())?

Comment 67

[Jorg K (CEST = GMT+2)]

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.

?

Comment 68

[:aceman]

Also it looks like returning values of this._entries directly may change the returning order from this._entries[this._days[...]] .

Comment 69

[Kris Maglione [:kmag]]

(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.

Comment 70

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat.patch (v6, reduced) [landed in comment #77]

Since we're still discussing logger.js, here's the reduced patch for imXPCOMUtils.jsm.

Comment 71

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004029 [details] [diff] [review]
1485820-fix destructors v1.1 [landed in comment #77]

Compiles for me and the defaults look right.

Comment 72

[Jorg K (CEST = GMT+2)]

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.

Comment 73

[:aceman]

(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 74

[:aceman]

Attachment: 1485820-fix GetDirectoryEntries calls [landed in comment #77]

Comment 75

[:aceman]

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.

Comment 76

[Jorg K (CEST = GMT+2)]

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.

Comment 77

[Jorg K (CEST = GMT+2)]

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?

Comment 78

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat-logger.patch

Yes, you were right, the previous suggestions were all wrong.

Comment 79

[Kris Maglione [:kmag]]

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.

Comment 80

[Jorg K (CEST = GMT+2)]

(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.

Comment 81

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat-logger.patch (v2)

Comment 82

[Jorg K (CEST = GMT+2)]

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?

Comment 83

[Kris Maglione [:kmag]]

(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.

Comment 84

[:aceman]

Yes, I think so too, there is a m_DirectoryEnumerator that I missed to convert.

Comment 85

[:aceman]

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

Comment 86

[:aceman]

Attachment: 1485820-simplify gloda enumerator

For comment 31, did you mean this? The called methods needed changing for nsISimpleEnumerator instead of JS generator.

Comment 87

[:aceman]

Attachment: 1485820-fix m_directoryEnumerator [landed in comment #92]

Fixes the 2 pop3 tests for me locally. Try run:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=7feb54dee957874ee3a819283f95381c93c51baf

Comment 88

[Kris Maglione [:kmag]]

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.

Comment 89

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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.

Comment 90

[Jorg K (CEST = GMT+2)]

Comment on attachment 9004042 [details] [diff] [review]
1485820-fix m_directoryEnumerator [landed in comment #92]

Thanks.

Comment 91

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-Symbol-iterator-chat-logger.patch (v3) [landed in comment #92]

That should be it then.

Comment 92

[Jorg K (CEST = GMT+2)]

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.

Comment 93

[:aceman]

Attachment: 1485820-simplify gloda enumerator v2 [landed in comment #107]

OK, seems to work too.

Comment 94

[Jorg K (CEST = GMT+2)]

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 ;-)

Comment 95

[Jorg K (CEST = GMT+2)]

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

Comment 96

[Kris Maglione [:kmag]]

(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.

Comment 97

[Jorg K (CEST = GMT+2)]

Thanks, I landed our patches with the spaces since there are more cases on M-C with the space.

Comment 98

[:aceman]

Attachment: 1485820-simplify fixIterator

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

Comment 99

[:aceman]

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?

Comment 100

[Jorg K (CEST = GMT+2)]

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.

Comment 101

[:aceman]

I think I know what is the problem, I'll file a m-c bug for it.

Comment 102

[:aceman]

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 103

[Kris Maglione [:kmag]]

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.

Comment 104

[:aceman]

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 105

[Jorg K (CEST = GMT+2)]

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.

Comment 106

[Jorg K (CEST = GMT+2)]

Attachment: 1485820-simplify fixIterator (v1.1) [landed in comment #107]

Let's lose all the unnecessary parenthesis.

Comment 107

[Jorg K (CEST = GMT+2)]

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.

Comment 108

[Jorg K (CEST = GMT+2)]

All mail/mailnews work is done here, we're just missing the Calendar patch and bug 1486311.

Comment 109

[Geoff Lankow (:darktrojan)]

Attachment: 1485820-property-map-iterator-2.diff

Comment 110

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

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?

Comment 111

[Geoff Lankow (:darktrojan)]

Attachment: 1485820-property-map-iterator-3.diff

> 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.

Comment 112

[Pulsebot]

Pushed by [e-mail]:
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

Comment 113

[Magnus Melin [:mkmelin]]

Filed bug 1594000 to clean up the call sites following this bug. I think a lot of them could be much simpler and nicer now!