1635639 - TB xpcshell-test InternalError: too much recursion File: resource://testing-common/mailnews/Nntpd.jsm Line: 433

Status: RESOLVED FIXED

(Thunderbird :: Mail Window Front End, defect)

Description

[ISHIKAWA, Chiaki]

During local xpcshell-test run with FULL DEBUG version of TB, I noticed the following error.

These errors are only visible when you add --verbose flag to |mach xpcshell-test| command because the error is SILENTLY IGNORED BY XPCSHELL-TEST HARNESS and IS NOT REPORTED to unsuspecting tryserver submitters. (and better still with --sequential so that the test results won't get mixed with parallel execution of subtests.)

 0:07.33 pid:1703764 InternalError: too much recursion
 0:07.33 pid:1703764 File: resource://testing-common/mailnews/Nntpd.jsm Line: 433
 0:07.33 pid:1703764 Stack trace:
 0:07.33 pid:1703764 get@resource://testing-common/mailnews/Nntpd.jsm:433:20
 0:07.33 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:5
 0:07.33 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
 0:07.33 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
 0:07.33 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
... many ...
 0:07.34 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
 0:07.34 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
 0:07.34 pid:1703764 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
 0:07.34 pid:1703764 [1703764, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057 (NS_ERROR_ILLEGAL_VALUE): file /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNNTPProtocol.cpp, line 1859

The code in question is

https://searchfox.org/comm-central/source/mailnews/test/fakeserver/Nntpd.jsm#433
A symlink is created during build to the following file.:
${MOZ_OBJ}/_tests/modules/mailnews/Nntpd.jsm

and https://searchfox.org/comm-central/source/mailnews/test/fakeserver/Nntpd.jsm#456

The errors happen in the following tests.

 egrep
"(TEST_START|TEST_END|Leaving test|Entering test|Internal)"
/FF-NEW/log1210-xpcshell.txt
... 
 0:06.75 TEST_START: comm/mailnews/news/test/unit/test_nntpPassword.js
 0:07.33 pid:1703764 InternalError: too much recursion
 0:07.58 TEST_END: Test PASS. Subtests passed 1/1. Unexpected 0
 0:07.59 TEST_START: comm/mailnews/news/test/unit/test_nntpPassword2.js
 0:08.20 pid:1703794 InternalError: too much recursion
 0:08.47 TEST_END: Test PASS. Subtests passed 1/1. Unexpected 0

What irks me is that both tests SUCCEEDED (Notce Test PASS 1/1.)

The test harness/infrastructure should fail a test that produces
JavaScript errors or InternalErrors.

TIA

Comment 1

[ISHIKAWA, Chiaki]

Attachment: Recent log of the same error.

The "too much recurson " error still persists.

However, the appearance of the stack trace has changed a bit.
``
0:20.46 pid:674299 InternalError: too much recursion
0:20.46 pid:674299 File: resource://testing-common/mailnews/Nntpd.jsm Line: 456
0:20.46 pid:674299 Stack trace:
0:20.47 pid:674299 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:5
0:20.47 pid:674299 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
0:20.47 pid:674299 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24

... many ...

0:20.47 pid:674299 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
0:20.47 pid:674299 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
0:20.47 pid:674299 LIST@resource://testing-common/mailnews/Nntpd.jsm:456:24
0:20.47 pid:674299 [674299, Main Thread] WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80070057 (NS_ERROR_ILLEGAL_VALUE): file /NEW-SSD/NREF-COMM-CENTRAL/mozilla/comm/mailnews/news/src/nsNNTPProtocol.cpp:1841
0:20.47 pid:674299 NS_NewBufferedOutputStream: outputStream (= std::move(aOutputputStream)) =0x607000074748
0:20.47 PASS - "MODE READER,LIST,AUTHINFO user testnews,AUTHINFO pass newstest,LIST" == "MODE READER,LIST,AUTHINFO user testnews,AUTHINFO pass newstest,LIST"
0:20.48 pid:674299 Passed test news:*

I am attaching a somewhat verbose (with  dump from local patches) for the test that caused this error.

Comment 2

[Magnus Melin [:mkmelin]]

Attachment: bug1635639_nntpd_recursion.patch

With this ./mach xpcshell-test comm/mailnews/news/test/unit/test_nntpPassword.js works, let's hope nothing else broke. Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=41d8b3e997ab6c2382947e24f5a136d1113a27c7

Comment 3

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #2)

Created attachment 9188770 [details] [diff] [review]
bug1635639_nntpd_recursion.patch

With this ./mach xpcshell-test comm/mailnews/news/test/unit/test_nntpPassword.js works, let's hope nothing else broke. Try: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=41d8b3e997ab6c2382947e24f5a136d1113a27c7

Great. Thank you for the patch.

Just curious. What is the |parent| when the the connection is authenticated (|this.authenticated|) ?
My asking is based on my observation below:.
Usually if a list structure is created, there is a mechanism to traverse it and a function to tell us when we reached the end of the list.
In this case, it seems that traversing is done by looking at |this.parent|.
To detect the end of the list, I would think that there is a terminating condition of the list structure, it could be |list.parent| being nil or some other attribute having a particular value.

I wonder if this list structure convention for this list created via |this.parent| in Nntpd.jsm is documented or not.
If documented, I think we should stick to the |end of list| condition to stop the recursion as well.
Is that condition |this.authenticated| being false?
From the look of the recursion, I suspect at the end of the list, this.parent actually refers to ITSELF.
This may be one way to stop the recursion. But I may be way off.

Comment 4

[ISHIKAWA, Chiaki]

Can bug 1667962 be fixed in a similar manner? However, the calling sequence seems to be very different

Comment 5

[Magnus Melin [:mkmelin]]

I think there's much confusion in these classes and now that js has classes the code should ideally be rewritten to use that instead of these home-grown class simulations.

Parent seems to be an NNTP_RFC977_handler and "this" is an NNTP_RFC4643_extension.
I don't really don't know much about these, and reasoning is difficult due to the above...

Comment 6

[ISHIKAWA, Chiaki]

(In reply to Magnus Melin [:mkmelin] from comment #5)

I think there's much confusion in these classes and now that js has classes the code should ideally be rewritten to use that instead of these home-grown class simulations.

Parent seems to be an NNTP_RFC977_handler and "this" is an NNTP_RFC4643_extension.
I don't really don't know much about these, and reasoning is difficult due to the above...

I see. I wonder if it makes sense to use a simplified server written in python or other popularly available language on windows and linux.
(But I am not sure why these simulators were done in JavaScript to begin with.)

I think I should give r+ to the patch in comment 2.

Comment 7

[ISHIKAWA, Chiaki]

Comment on attachment 9188770 [details] [diff] [review]
bug1635639_nntpd_recursion.patch

I think we should land this for investigating if there would be hidden issues with nntpd simulator.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/4bf90269ae49
fix Nntpd not to go into infinite recursion for LIST without arguments. r=ishikawa DONTBUILD