1810082 - During xpcshell test of C-C TB DEBUG version, I get WARNING: 'mIndex >= Count()', file /mozilla/xpcom/ds/nsStringEnumerator.cpp:202

Status: NEW

(Thunderbird :: General, defect)

Description

[ISHIKAWA, Chiaki]

Attachment: comm/mail/base/test/unit/test_mailGlue_distribution.js mochitest log.

During the xpcshell testing of DEBUG version of C-C TB, I get the folloing errors.
One needs to pass this option to |mach xpcshell-test| invocation to look at the errors/warnings.
--sequential --verbose

00:08.64 pid:52252 [Parent 52252, Main Thread] WARNING: 'mIndex >= Count()', file /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/ds/nsStringEnumerator.cpp:202

The warning is printed during the testing of comm/mail/base/test/unit/test_mailGlue_distribution.js
and I think something is wrong considering the line
https://searchfox.org/mozilla-central/source/xpcom/ds/nsStringEnumerator.cpp#202

nsStringEnumerator::GetNext(nsAString& aResult) {
  if (NS_WARN_IF(mIndex >= Count())) {   <==== This is 202.;
    return NS_ERROR_UNEXPECTED;
  }

We are passing something out of bound. Bad.

Comment 1

[ISHIKAWA, Chiaki]

Attachment: gdb session to see what is going on when the warning is printed. Nothing extraordinary.

Interesting.
I checked to see when the error started to occur.
I found that the local log on Feb 26, 2021 have these warnings.
But earlier log, say, in January, 2021 does not.
Something changed in between.

Given that the |NS_WARN_IF()| on relevant lines seem to have been placed much earlier (earlier in 2014), I think then the caller of the said function began behaving differently.

I checked some cases of the said warnings I see when I start up debug version of C-C TB under gdb.

Attachment is the gdb session log (from the start to a point where I have seen a few warnings.)

Note that the |mIndex| value is part of |*this| when the |GetNext()| is reached.

There is nothing unusual about the looping. |mIndex| is 0 when the GetNext() is first called.
Each next call to |GetnNext()|, |mIndex| is incremented until |mIndex| is too large.
When the warning is printed.

In the last enumeration loop for looping over

$50 = const nsTArray<nsTString<char> > = {"app.releaseNotesURL", 
  "mailnews.start_page.welcome_url", "test.setting.locale"}

I dumped JavaStack and showed the stacktrace of C++. C++ stack shows that compiled javascript byte code calls
|JSStringEnumerator::Next()|
I am not sure if |Next()| should check for |HasMore()|.

(gdb) where
#0  nsStringEnumerator::GetNext(nsTSubstring<char16_t>&)
    (this=0x555556ce8b70, aResult=u"")
    at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/ds/nsStringEnumerator.cpp:201
#1  0x00007fffe7ebfea2 in (anonymous namespace)::JSStringEnumerator::Next(JSContext*, JS::MutableHandleValue)
    (this=0x555556ce89a0, aCx=0x555555943db0, aResult=$JS::UndefinedValue())
    at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/ds/nsStringEnumerator.cpp:50
#2  0x00007fffe7fd2f46 in NS_InvokeByIndex ()
    at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/xpcom/reflect/xptcall/md/unix/xptcinvoke_asm_x86_64_unix.S:101
#3  0x00007fffe8bd95fd in CallMethodHelper::Invoke() (this=0x7fffffffadf8)
    at /NEW-SSD/NREF-COMM-CENTRAL/mozilla/js/xpconnect/src/XPCWrappedNative.cpp:1626
#4  CallMethodHelper::Call() (this=this@entry=0x7fffffffadf8)

I think whatever is causing the enumeration loop does not check |HasMore()| before it calls |GetNext()| and seems to decide to terminate the loop processing by checking the error code returned by |GetNext()|.

Is there a hand-crafted open loop for emulating enumeration loop in JavaScript code?
That is hard to believe. But given that the warning suddenly appears at the beginning of 2021...

Or are we maybe doing something similar in C++ code in C-C TB code tree?

A C++ code snippet in M-C portion would have been caught IMHO
(however, I have not checked tryserver M-C mozilla test log yet.)

OK, actually I see a lot of hand-crafted open loop when I search for |Next()|.
https://searchfox.org/mozilla-central/search?q=Next%28%29&path=&case=false&regexp=false
Some of these open loops may have forgotten to check for |HasMore()| in advance.

I think putting the check for |HasMore| in ||Next()| may be a good idea.
Better than removing the |NS_WARN_IF| from the if-expressions since
there CAN be buggy change in the future in the fundamental part of the code, and so
such warning may have merit of being there.