Compaction on Windows broken, caused by bug 1403393 / bug 1402888 - Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #13 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045)

RESOLVED FIXED in Thunderbird 58.0

Status

defect
--
critical
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: baku)

Tracking

({regression})

Trunk
Thunderbird 58.0
All
Windows
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [Thunderbird-testfailure: X Windows][Thunderbird-temporary-fix][PLR])

Attachments

(6 attachments, 8 obsolete attachments)

1.67 KB, patch
Details | Diff | Splinter Review
759 bytes, patch
smaug
: review+
Details | Diff | Splinter Review
21.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
9.71 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.36 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.66 KB, patch
Details | Diff | Splinter Review
mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js
and
mailnews/imap/test/unit/test_compactOfflineStore.js

First seen Wed Sep 27, 2017, 7:36:28

M-C last good: bc56729898954e32d3a3731d03d178ed78
M-C first bad: 70158e4e215d784d1391db5e517b18727f

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=bc56729898954e32d3a3731d03d178ed78&tochange=70158e4e215d784d1391db5e517b18727f

Likely related to bug 1403393 / bug 1402888.

Fails locally:
mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js
Looking at
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=6a819eac9f9f49c203104ff1770258d73f954e3b&selectedJob=133801990
there are four test failures:
mailnews/base/test/unit/test_folderCompact.js
mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js
mailnews/imap/test/unit/test_compactOfflineStore.js
mailnews/local/test/unit/test_over4GBMailboxes.js

Running test_gloda_content_imap_offline.js locally I see disconcerting things like:
 0:11.44 PROCESS_OUTPUT: Thread-1 (pid:6424) "The MsgHdrToMimeMessage callback threw an exception: 2147500036"
 0:11.44 PROCESS_OUTPUT: Thread-1 (pid:6424) "JavaScript error: resource://testing-common/mailnews/maild.js, line 200: uncaught exception: 2147500036"
 0:11.48 TEST_STATUS: Thread-1 verify_message_content FAIL [verify_message_content : 160] "robots are nice...\\n\\nexcept for the bloodlust\\nFrom - Thu Sep 28 03:18:07 2017\\nX-Mozilla-Status: 0001\\nX-Mozilla-Status2: 000 == "robots are nice...\\n\\nexcept for the bloodlust"

So something fails retrieving messages and message content is returned incorrect, in fact too long since the From and status headers are not expected.
Severity: normal → major
Whiteboard: [Thunderbird-testfailure: X all]
Tom pointed me at
https://hg.mozilla.org/mozilla-central/rev/560296786c0c
Remove nsIInputStreamPump::Init() offset and limit params

and we use that at least here:
https://dxr.mozilla.org/comm-central/rev/6a819eac9f9f49c203104ff1770258d73f954e3b/mailnews/mime/src/mimeParser.jsm#60
Sadly that doesn't fix the tests but it might fix the Mozmill failure in bug 1403939.
Comment on attachment 8913320 [details] [diff] [review]
1403771-nsIInputStreamPump.patch

I landed that in bug 1404045, it didn't help here, but it fixed bug 1403939.
Attachment #8913320 - Attachment is obsolete: true
Regression bisection:
Before bug 1402888:
M-C: cb604e7830ec
C-C: 3f2554599137
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js
pass.
Regression bisection:
After bug 1402888:
M-C: 7e992ffa004c
C-C: 523f36e9ebc5 with nsIStreamTransportService forked:
mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js fails.
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_compactOfflineStore.js fails.

mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js passes.
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js passes.

The latter two have a different range:
M-C last good: 70158e4e215d784d1391db5e517b18727f
M-C first bad: 5ebe2e8980c6fd3ede2b6617bbbc4073dd
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=70158e4e215d784d1391db5e517b18727f&tochange=5ebe2e8980c6fd3ede2b6617bbbc4073dd

But let's see C-C at 5b7dc947139e with M-C still at 7e992ffa004c, so replacing the fork with the SlicedInputStream() stuff:
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js
now both fail.

So in summary: After M-C bug 1402888 we have two test failures if we make up for the changed code with a fork of the original state. If we use the SlicedInputStream() replacement, we get an additional two test failures.
I wonder what was wrong with the fork, and even more disconcerting, what is wrong with the "replacement" solution using SlicedInputStream().
Summary: mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js → mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js and three others, caused by bug 1402888 (bug 1403393)
Severity: major → critical
The funny thing is that
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js
only fail in Windows, not Linux.

So maybe there's something wrong with the SlicedInputStream() patch:
https://hg.mozilla.org/comm-central/rev/5b7dc947139e10f256ae3ff40877bf6a67d37207

Aceman, can you spot anything wrong there, you have experience with large numbers ;-)
Flags: needinfo?(acelists)
I've logged the data passed to SlicedInputStream() and the number are between 0 and 4956 for test_folderCompact.js.

For test_over4GBMailboxes.js I see "0 -1", printing with %lld, oops, and 4309562542. Printing with I64u I see "0 18446744073709551615", now, that is NOT right. Filed bug 1404489 for that.
OK,
mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_compactOfflineStore.js
are fixed by
https://hg.mozilla.org/comm-central/rev/3dfe0c40ba28093a2c4638126fd1df3bb204561d

so
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js
remain to be fixed.
Summary: mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js and three others, caused by bug 1402888 (bug 1403393) → Fix test_gloda_content_imap_offline.js, test_compactOfflineStore.js, test_folderCompact.js, test_over4GBMailboxes.js, caused by bug 1402888 (bug 1403393)
(In reply to Jorg K (GMT+2) from comment #8)
> Printing with I64u I see "0 18446744073709551615", now, that is
> NOT right. Filed bug 1404489 for that.
18446744073709551615 is actually UINT64_MAX = 2^64-1. So it is OK to pass -1.
Maybe we're seeing two causes here, the two IMAP tests failing due to Necko changes covered in bug 1404045, and the compact/4GB tests caused by something else, since they have a different range, see comment #6.

M-C last good: 70158e4e215d784d1391db5e517b18727f
M-C first bad: 5ebe2e8980c6fd3ede2b6617bbbc4073dd
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=70158e4e215d784d1391db5e517b18727f&tochange=5ebe2e8980c6fd3ede2b6617bbbc4073dd

Summarising failures on Windows only(!!):

mailnews/local/test/unit/test_over4GBMailboxes.js fails like this:
 0:24.19 LOG: Thread-1 INFO "Local inbox size (after compact 2) = 4313201590"
 0:24.19 TEST_STATUS: Thread-1 OnStopRunningUrl PASS [OnStopRunningUrl : 534] 4313201590 == 4313201590
 0:24.19 TEST_STATUS: Thread-1 OnStopRunningUrl FAIL [OnStopRunningUrl : 535] false == true

Line 535 of the test is:
do_check_true(localInboxSize < kSparseBlockSize + 1000);
and var kSparseBlockSize = 102400000;
Obviously    4313201590 is not
smaller than  102400000,
which seems to indicate that at least on Windows the compact hasn't worked.

mailnews/base/test/unit/test_folderCompact.js fails with
 0:05.55 LOG: Thread-1 INFO (xpcshell/head.js) | test testDeleteMessages2 pending (2)
 0:05.57 TEST_STATUS: Thread-1 testDeleteMessages2 FAIL [testDeleteMessages2 : 208] 2324 == 4956
Here's line 208:
  function* testDeleteMessages2() {
    do_check_eq(gExpectedFolderSize, gLocalFolder3.filePath.fileSize); <=== 208.
So the file size is larger than the expected folder size, again an indication that compaction hasn't worked.
Summary: Fix test_gloda_content_imap_offline.js, test_compactOfflineStore.js, test_folderCompact.js, test_over4GBMailboxes.js, caused by bug 1402888 (bug 1403393) → Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #11 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045)
I'm getting confused since I'm dealing with too much bustage at the same time. Essential is comment #6 (repeating):

Regression bisection:
After bug 1402888:
M-C: 7e992ffa004c
C-C: 523f36e9ebc5 with nsIStreamTransportService forked:
mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js fails.
mozilla/mach xpcshell-test mailnews/imap/test/unit/test_compactOfflineStore.js fails.
### Those failure are clear, they are fixed in bug 1404045 which deals with changes to nsIInputStreamPump.init

mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js passes.
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js passes.
### So with 1402888 using the fork of nsIStreamTransportService the two tests above still pass.

But let's see C-C at 5b7dc947139e with M-C still at 7e992ffa004c, so replacing the fork with the SlicedInputStream() stuff:
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js
now both fail.
### So these two start to fail at the same time the fork of nsIStreamTransportService
### is replaced by the "proper" SlicedInputStream() stuff.

Since the best attempts to dealing with changed to nsIInputStreamPump.init in bug 1404045, I conclude that these two test failures are related to some use of nsIStreamTransportService that wasn't covered in bug 1403393.
Now looking at
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js

Aceman pointed out that we see
 WARNING: error renaming compacted folder: 'folderRenameSucceeded', file c:/mozilla-source/comm-central/mailnews/base/src/nsMsgFolderCompactor.cpp, line 569"

So this |rv = m_file->MoveToNative((nsIFile *) nullptr, folderName);| fails.

I traced it down into the bowels of Windows and we fail at
xpcom/io/nsLocalFileWin.cpp:1981 which is
      copyOK = ::MoveFileExW(filePath.get(), destPath.get(),
                           MOVEFILE_REPLACE_EXISTING);
which is the native Windows call resulting in a NS_ERROR_FILE_ACCESS_DENIED.

So where we clearly moved into Chiaki territory who is always worried that streams aren't closed, and files can't be moved or renamed. Looks like he's right.

Replacing nsIStreamTransportService with the the SlicedInputStream() stuff has change the dynamics here and the the file is still open when we come to move it. Linux is different, so the tests doesn't fail there.
Some more debug:
 0:01.97 PROCESS_OUTPUT: Thread-1 (pid:852) "[852, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 119"
 0:01.98 PROCESS_OUTPUT: Thread-1 (pid:852) "=== 1"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder3.msf|"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder3-1.msf|"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== calling MoveToNative"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== 0"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\nstmp|"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder3|"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== 1"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\nstmp.msf|"
 0:01.99 PROCESS_OUTPUT: Thread-1 (pid:852) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder3.msf|"

0 means error, so it fails to move the raw message file. and the .msf is moved back, although it seems to have changed names from folder3-1.msf to nstmp.msf.

So something is still locking the raw message file. Also disconcerting the warning from SlicedInputStream.cpp.
Blocks: 1403393, 1402888
Keywords: regression
Summary: Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #11 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045) → Compation on Windows broken, caused by bug bug 1403393 / bug 1402888 - Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #13 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045)
Whiteboard: [Thunderbird-testfailure: X all] → [Thunderbird-testfailure: X Windows]
Flags: needinfo?(acelists)
Component: General → Database
OS: Unspecified → Windows
Product: Thunderbird → MailNews Core
Hardware: Unspecified → All
Summary: Compation on Windows broken, caused by bug bug 1403393 / bug 1402888 - Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #13 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045) → Compaction on Windows broken, caused by bug bug 1403393 / bug 1402888 - Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #13 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045)
Summary: Compaction on Windows broken, caused by bug bug 1403393 / bug 1402888 - Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #13 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045) → Compaction on Windows broken, caused by bug 1403393 / bug 1402888 - Fix test_folderCompact.js, test_over4GBMailboxes.js, see comment #13 (failing test_gloda_content_imap_offline.js, test_compactOfflineStore.js fixed in bug 1404045)
Andrea, Olli as author and reviewer of bug 1402888, Patrick and Honza as Necko peers:

Bug 1402888, the withdrawal of parameters from  nsIStreamTransportService::CreateInputTransport and nsIInputStreamPump::Init has caused us a big headache.

We have used SlicedInputStream() wherever necessary but still have problems:

We are seeing:
WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 119
frequently without understanding what causes it and whether the condition can be safely ignored.

On Windows, one essential feature of Thunderbird, folder compaction, is broken and our tests detected that. Basically Thunderbird takes a folder mbox file, reads it, ignores all deleted messages and writes a new folder mbox file with the remaining good messages. That file is then moved on top of the existing old file. This stopped working with the Windows system call to move the file failing.

We assume that plugging in the intermediate sliced stream is causing this. Either it's not closing as it should or it causes other timing issues, so the original file isn't released by the time we come to clobber it.

Any hints would be welcome.
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(bugs)
Flags: needinfo?(amarchesini)
Keywords: leave-open
Whiteboard: [Thunderbird-testfailure: X Windows] → [Thunderbird-testfailure: X Windows][Thunderbird-disabled-test]
I would like to reproduce this issue locally. Can you ping me on IRC and tell me more?
As I wrote on bug 1404045, we are triggering a bug in SlicedInputStream or in how it is used in that code.
Having ::Available() returning error is correct if the original stream returns that error. I suspect that the bug is elsewhere.
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #17)
> I would like to reproduce this issue locally. Can you ping me on IRC and
> tell me more?
> As I wrote on bug 1404045, we are triggering a bug in SlicedInputStream or
> in how it is used in that code.
> Having ::Available() returning error is correct if the original stream
> returns that error. I suspect that the bug is elsewhere.
Hi Andrea, thanks for your reply. I can ping you on IRC, in which time zone are you located?

As for triggering the bug, you just need to build Thunderbird. Follow the published guidelines.

Running Thunderbird will show that warning many times, including when running the test. To run the test, simply paste
  mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
into your "DOS"/bash window. Note, Mac currently doesn't build and the test only fails in Windows, but of course, the warnings are always present.
Flags: needinfo?(mcmanus)
Flags: needinfo?(honzab.moz)
Flags: needinfo?(bugs)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1688845160a2
temporarily switch off broken tests test_folderCompact.js, test_over4GBMailboxes.js on Windows. rs=bustage-fix
Andrea, is there anything I can do to assist you? You haven't answered the question re. the time zone.

This bug is very important to us since as a consequence mailbox folder compaction is broken, that is, fails silently.
Flags: needinfo?(amarchesini)
My timezone is GMT. I'll work on it today.
Flags: needinfo?(amarchesini)
We end up having aCount == 0 but we still call the ::Read() to nsFileInputStream. This calls PR_Read() with aCount 0 and we return an error. On linux/mac this is fine. I guess on windows has a different behavior.
Assignee: nobody → amarchesini
Attachment #8915084 - Flags: feedback?(jorgk)
Attachment #8915084 - Flags: review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77427ddd1f47
SlicedInputStream::Read should not call ::Read() of the underlying stream when there is nothing else to read, r=smaug
Thanks Andrea for looking into it. Sadly the patch doesn't fix our test failure in:
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js passes

The debug output is still:

 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "[6932, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 119"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== 1"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== |c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\temp\\xpc-profile-lv3ae0\\Mail\\Local Folders\\folder3.msf|"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== |c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\temp\\xpc-profile-lv3ae0\\Mail\\Local Folders\\folder3-1.msf|"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== 0"  <===== indicates failure to move the file on 
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== |c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\temp\\xpc-profile-lv3ae0\\Mail\\Local Folders\\nstmp|"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== |c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\temp\\xpc-profile-lv3ae0\\Mail\\Local Folders\\folder3|"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== 1"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== |c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\temp\\xpc-profile-lv3ae0\\Mail\\Local Folders\\nstmp.msf|"
 0:08.20 PROCESS_OUTPUT: Thread-3 (pid:6932) "=== |c:\\mozilla-source\\comm-central\\obj-x86_64-pc-mingw32\\temp\\xpc-profile-lv3ae0\\Mail\\Local Folders\\folder3.msf|"
 0:07.94 TEST_STATUS: Thread-3 compactFolder PASS [compactFolder : 63] 0 == 0
 0:07.94 TEST_STATUS: Thread-3 compactFolder PASS [compactFolder : 70] true == true
 0:07.95 TEST_STATUS: Thread-3 compactFolder PASS [compactFolder : 72] 2 == 2
 0:07.95 TEST_STATUS: Thread-3 compactFolder PASS [compactFolder : 74] true == true
 0:08.22 PROCESS_OUTPUT: Thread-3 (pid:6932) "Show messages for folder <folder3> after compact"
 0:08.22 PROCESS_OUTPUT: Thread-3 (pid:6932) "key: 2 offset: 0 ID: 200804111417.m3BEHTk4030129@mrapp51.mozilla.org"
 0:07.95 LOG: Thread-3 INFO (xpcshell/head.js) | test run_next_test 8 pending (2)
 0:07.95 LOG: Thread-3 INFO (xpcshell/head.js) | test compactFolder finished (2)
 0:07.95 LOG: Thread-3 INFO mailnews/base/test/unit/test_folderCompact.js | Starting testDeleteMessages2
 0:07.95 LOG: Thread-3 INFO (xpcshell/head.js) | test testDeleteMessages2 pending (2)
 0:07.96 TEST_STATUS: Thread-3 testDeleteMessages2 FAIL [testDeleteMessages2 : 208] 2324 == 4956

So the test still fails to move the new file on top of the old.

mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js equally still fails.
Comment on attachment 8915084 [details] [diff] [review]
sliced.patch [M-C: landed comment #49]

Maybe a useful fix, but not for addressing that the files we're working are still locked on Windows when they weren't with the original code or the forked code I prepared in bug 1404752.
Attachment #8915084 - Flags: feedback?(jorgk)
Posted patch sliced2.patch (obsolete) — Splinter Review
We don't propagate the close() and CloseWithStatus() call.
Attachment #8915111 - Flags: feedback?(jorgk)
Comment on attachment 8915111 [details] [diff] [review]
sliced2.patch

Thanks, that fixes both tests :-)

(In reply to Andrea Marchesini [:baku] from comment #26)
> We don't propagate the close() and CloseWithStatus() call.
That would be pretty fatal, no? :-(

I still see the ugly warning though, but you said that that's expected:
 0:05.55 PROCESS_OUTPUT: Thread-1 (pid:2040) "[2040, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 119"

NS_WARN_IF(NS_FAILED(rv)) seems to be a pattern that is used an awful lot in M-C, yet it gives no useful information what the error actually was.

Thanks again. Magnus said in bug 1404752 comment #8:
We should probably remember we're kind of the live testing ground for m-c code at large.
Looks like he's right.
Attachment #8915111 - Flags: feedback?(jorgk) → feedback+
> I still see the ugly warning though, but you said that that's expected:
>  0:05.55 PROCESS_OUTPUT: Thread-1 (pid:2040) "[2040, Main Thread] WARNING:
> 'NS_FAILED(rv)', file
> c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line
> 119"

This is fine. the error is CLOSED. I'm writing a gtest and I'll ask a proper review. Sorry for all this mess.
Posted patch sliced2.patch (obsolete) — Splinter Review
gtest included
Attachment #8915111 - Attachment is obsolete: true
Attachment #8915128 - Attachment is obsolete: true
Comment on attachment 8915184 [details] [diff] [review]
sliced2.patch [M-C: landed comment #56]

Could you document in SlicedInputStream.h that the sliced stream takes the ownership and for example Close() does close the internal stream.
Attachment #8915184 - Flags: review+
Hmm, this patch is a whole lot larger. Under which conditions would we have to follow the
-    new SlicedInputStream(stream, mStart, mLength);
+    new SlicedInputStream(stream.forget(), mStart, mLength);

We're using SlicedInputStream() in a few call sites now:
https://dxr.mozilla.org/comm-central/search?q=SlicedInputStream&redirect=false
I can see https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#481 being problematic, since sliced stream takes now ownership.
But I'm sure baku has ideas there.
Mid-air, Olli spotted the same problem.
===
How about this stuff:
https://dxr.mozilla.org/comm-central/search?q=m_multipleMsgMoveCopyStream&redirect=false
FWIW, I think all the SlicedInputStream usage in TB makes me be even stronger against the Close handling without ownership passing (in other words against https://bug1403771.bmoattachments.org/attachment.cgi?id=8915128). One could accidentally close the underlying stream while it is being used elsewhere.
I've added the .forget() identifying two problems.

m_inputStream is actually used in subclasses of nsMsgProtocol:
https://dxr.mozilla.org/comm-central/search?q=m_inputStream&redirect=false

Also, I get a compile error:
 0:39.47 c:/mozilla-source/comm-central/mozilla/dom/file/StreamBlobImpl.cpp(108): error C3861: 'GetInternalStream': identifier not found
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c68498143c13
SlicedInputStream takes ownership of the underlying stream and it propagates the Close() call, r=smaug
> I've added the .forget() identifying two problems.
> 
> m_inputStream is actually used in subclasses of nsMsgProtocol:
> https://dxr.mozilla.org/comm-central/search?q=m_inputStream&redirect=false

Then you must clone the stream. I would do:

  nsCOMPtr<nsIInputStream> clonedStream;
  nsCOMPtr<nsIInputStream> replacemenetStream;
  nsresult rv = NS_CloneInputStream(m_inputStream, getter_AddRefs(clonedStream),
                            getter_AddRefs(replacementStream));
  if (NS_WARN_IF(NS_FAILED(rv))) {
    ...
  }

  if (replacementStream) {
    // If m_InputStream is not cloneable, NS_CloneInputStream will clone it using a pipe. In order to keep the copy alive and working, we have to replace the original stream (m_InputStream) with the replacement.
    m_InputStream = replacementStream.forget(); 
  }

  RefPtr<SlicedInputStream> sliced = new SlicedInputStream(clonedStream, 0, 123);
  ...

> c:/mozilla-source/comm-central/mozilla/dom/file/StreamBlobImpl.cpp(108):
> error C3861: 'GetInternalStream': identifier not found

This as been renamed CreateInputStream.
Flags: needinfo?(jorgk)
Comment on attachment 8915195 [details] [diff] [review]
1403771-sliced-ownership.patch

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

Hi Andrea and Olli, thanks for looking at this.

It is true that we had to encapsulate the underlying stream into SlicedInputStream due to the recent m-c and then just closed the SlicedInputStream as if it were the original underlying stream. If SlicedInputStream didn't forward the close() to the underlying stream, that could understandably cause problems.

Thanks for improving the stream semantics.

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +477,5 @@
>          }
>          // m_readCount can be -1 which means "read as much as we can".
>          // We pass this on as UINT64_MAX, which is in fact uint64_t(-1).
>          RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(m_inputStream.forget(), uint64_t(offset),  // XXX Problem!!

What problem do these comments mark?
Backout by archaeopteryx@coole-files.de:
https://hg.mozilla.org/integration/mozilla-inbound/rev/908add663c5b
Backed out changeset c68498143c13 for bustage at dom/file/StreamBlobImpl.cpp:108: 'GetInternalStream' was not declared in this scope. r=backout on a CLOSED TREE
Backed out for bustage at dom/file/StreamBlobImpl.cpp:108: 'GetInternalStream' was not declared in this scope:

https://hg.mozilla.org/integration/mozilla-inbound/rev/908add663c5be97c5d8aeb9e3bf82ef9c445a73e

Push with bustage: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c68498143c13b8053dcd660ee923bb5d1e5a23e8&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Build log: https://treeherder.mozilla.org/logviewer.html#?job_id=134952186&repo=mozilla-inbound

[task 2017-10-04T17:02:19.464Z] 17:02:19     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dom/file/Unified_cpp_dom_file0.cpp:128:0:
[task 2017-10-04T17:02:19.464Z] 17:02:19     INFO -  /builds/worker/workspace/build/src/dom/file/StreamBlobImpl.cpp: In member function 'virtual already_AddRefed<mozilla::dom::BlobImpl> mozilla::dom::StreamBlobImpl::CreateSlice(uint64_t, uint64_t, const nsAString&, mozilla::ErrorResult&)':
[task 2017-10-04T17:02:19.464Z] 17:02:19     INFO -  /builds/worker/workspace/build/src/dom/file/StreamBlobImpl.cpp:108:56: error: 'GetInternalStream' was not declared in this scope
[task 2017-10-04T17:02:19.464Z] 17:02:19     INFO -       GetInternalStream(getter_AddRefs(clonedStream), aRv);
[task 2017-10-04T17:02:19.464Z] 17:02:19     INFO -                                                          ^
[task 2017-10-04T17:02:19.464Z] 17:02:19     INFO -  /builds/worker/workspace/build/src/config/rules.mk:1075: recipe for target 'Unified_cpp_dom_file0.o' failed
[task 2017-10-04T17:02:19.465Z] 17:02:19     INFO -  gmake[5]: *** [Unified_cpp_dom_file0.o] Error 1
Flags: needinfo?(amarchesini)
I think (In reply to Andrea Marchesini [:baku] from comment #38)
> Then you must clone the stream. I would do:
I think we have to just through a whole lot of hoops here to get the original nsIStreamTransportService with it's additional parameters back. Maybe a 350-line fork is the simpler way to go.

This all comes at a very inopportune time since I will be offline for large parts of tomorrow due to travel, when this will be merged to M-C, oops, it just got backed out:
https://hg.mozilla.org/integration/mozilla-inbound/rev/908add663c5b

> > c:/mozilla-source/comm-central/mozilla/dom/file/StreamBlobImpl.cpp(108):
> > error C3861: 'GetInternalStream': identifier not found
> This as been renamed CreateInputStream.
Not in the copy I have and apparently you made a mistake:
Backed out changeset c68498143c13 (bug 1403771) for bustage at dom/file/StreamBlobImpl.cpp:108: 'GetInternalStream' was not declared in this scope. r=backout on a CLOSED TREE

If you want to re-land it, please wait until tomorrow afternoon.
(In reply to :aceman from comment #39)
> >          RefPtr<SlicedInputStream> slicedStream =
> > +          new SlicedInputStream(m_inputStream.forget(), uint64_t(offset),  // XXX Problem!!
> What problem do these comments mark?
Well, of you release ownership of the stream in m_inputStream to the sliced stream, then you can't access it anywhere else any more.

Hence you need to clone it as suggestion in comment #38. I just wanted to get the code compiled and mark the problems. As we can see, it doesn't compile for other reasons.
Flags: needinfo?(jorgk)
Untested since I can't even compile :-(
Attachment #8915195 - Attachment is obsolete: true
Attachment #8915219 - Flags: review?(amarchesini)
Now without trailing spaces.
Attachment #8915219 - Attachment is obsolete: true
Attachment #8915219 - Flags: review?(amarchesini)
Attachment #8915220 - Flags: review?(amarchesini)
Comment on attachment 8915220 [details] [diff] [review]
1403771-sliced-ownership.patch (v2) [landed comment #57]

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

::: mailnews/local/src/nsMailboxProtocol.cpp
@@ +76,5 @@
> +                           getter_AddRefs(clonedStream),
> +                           getter_AddRefs(replacementStream));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  if (replacementStream) {
> +    // If m_inputStream is not cloneable, NS_CloneInputStream will clone

Copy/paste error here, I'll fix it before landing.
BTW, I fixed the compile error replacing GetInternalStream with CreateInputStream to run my test and it fails as originally:

 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== 1"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder2.msf|"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder2-1.msf|"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== 0"  <==== Failure to move the file on Windows.
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\nstmp|"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder2|"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== 1"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\nstmp.msf|"
 0:06.97 PROCESS_OUTPUT: Thread-1 (pid:4492) "=== |c:\\users\\jorgk\\appdata\\local\\temp\\firefox\\xpcshellprofile\\Mail\\Local Folders\\folder2.msf|"
Here's a sightly more adventurous patch decoupling the use if m_inputStream in various source files to avoid stream cloning once.

mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
still fails, I guess the problem is with m_multipleMsgMoveCopyStream which is still cloned and then maybe not closed properly.
Flags: needinfo?(amarchesini)
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ad8bee818a39
SlicedInputStream takes ownership of the underlying stream and it propagates the Close() call, r=smaug
Attachment #8915220 - Flags: review?(amarchesini) → review+
Andrea, thanks for the r+. I'm just thinking about why our test
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
still fails with the latest set of patches. I think it's related to this hunk:

+  nsCOMPtr<nsIInputStream> clonedStream;
+  nsCOMPtr<nsIInputStream> replacementStream;
+  rv = NS_CloneInputStream(m_multipleMsgMoveCopyStream,
+                           getter_AddRefs(clonedStream),
+                           getter_AddRefs(replacementStream));
+  NS_ENSURE_SUCCESS(rv, rv);
+  if (replacementStream) {
+    // If m_multipleMsgMoveCopyStream is not cloneable, NS_CloneInputStream
+    // will clone it using a pipe. In order to keep the copy alive and working,
+    // we have to replace the original stream with the replacement.
+    m_multipleMsgMoveCopyStream = replacementStream.forget();
+  }
   // XXX 64-bit
   // This can be called with size == -1 which means "read as much as we can".
   // We pass this on as UINT64_MAX, which is in fact uint64_t(-1).
   RefPtr<SlicedInputStream> slicedStream =
-    new SlicedInputStream(m_multipleMsgMoveCopyStream, offset,
+    new SlicedInputStream(clonedStream.forget(), offset,
                           size == -1 ? UINT64_MAX : uint64_t(size));
   rv = serv->CreateInputTransport(slicedStream, false,
                                   getter_AddRefs(m_transport));

As you can see, we pass 'false' to CreateInputTransport which means that we'll have to close the stream ourselves once we're done.

But we can only close the original which we hold in 'multipleMsgMoveCopyStream'. How would the clone whose ownership is handed over to the sliced stream be closed?

If you look at comment #47, you can see that with the latest set of patches we still fail to move the new file on top of the old. With your patch that just added promoting the Close() to the original stream, it worked.

I'm also new to the concept of cloning a stream. What does that entail exactly? Does that clone the open status so you'd have to close two streams at the end?

Any hint would be much appreciated.
Flags: needinfo?(amarchesini)
> As you can see, we pass 'false' to CreateInputTransport which means that
> we'll have to close the stream ourselves once we're done.

I would do it.

> But we can only close the original which we hold in
> 'multipleMsgMoveCopyStream'. How would the clone whose ownership is handed
> over to the sliced stream be closed?

> I'm also new to the concept of cloning a stream. What does that entail
> exactly? Does that clone the open status so you'd have to close two streams
> at the end?

When a nsFileInputStream is cloned, we create a new nsFileInputStream pointing to the same nsIFile.
The new nsFileInputStream will receive the same behavior flags of the original stream.

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsFileStreams.cpp#715-721

Why the bug is still there? Because when Thunderbird creates a nsFileInputStream, it doesn't pass DEFER_OPEN flag.

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIFileStreams.idl#74-84

When this flag is set, the file is not immediately opened but it's opened only at the first read/available/seek/.. call.
Basically, the original stream will not have a FileDescriptor pointing to the file, and windows will be able to move it.

I would change this:
https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1940
    rv = NS_NewLocalFileInputStream(getter_AddRefs(inputStream), aFile, -1, -1, 
           nsIFileInputStream::CLOSE_ON_EOF | nsIFileInputStream::REOPEN_ON_REWIND |
           nsIFileInputStream::DEFER_OPEN | nsIFileInputStream::SHARE_DELETE);

same here: https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgBrkMBoxStore.cpp#756
and here: https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#208
Flags: needinfo?(amarchesini)
(In reply to Andrea Marchesini [:baku] from comment #52)
> > As you can see, we pass 'false' to CreateInputTransport which means that
> > we'll have to close the stream ourselves once we're done.
> I would do it.
Can you please clarify. What would you do? Close the stream? I believe we do that otherwise the test wouldn't have worked in the first place.

My question was re. the clone whose control we've handed over to a sliced stream which we then pass to CreateInputTransport with 'false'.

> I would change this:
> https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsLocalMailFolder.cpp#1940
> same here:
> https://dxr.mozilla.org/comm-central/source/mailnews/local/src/nsMsgBrkMBoxStore.cpp#756
> and here:
> https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#208
How did you pick those out of the many we have?

Particularly nsLocalMailFolder.cpp#1940 doesn't appear to be a candidate for deferring since there is a Read() call a few lines further down. However, I tried your suggestion and the test still fails.

I believe the problem is in the code I pointed out in nsMailboxProtocol.cpp since when I use my fork of nsIStreamTransportService::CreateInputTransport, no slicing is used there and the test passes.

As far as I can see, the slicing (and cloning) causes problems when used to make up for the removed parameter of that function, not for the removed parameters of the input stream's init function.
Comment on attachment 8915324 [details] [diff] [review]
1403771-sliced-ownership2.patch

I'll prepare a new patch showing the concept of removing shared use of m_inputStream.
Attachment #8915324 - Attachment is obsolete: true
Andrea, could you please briefly address comment #53.
Flags: needinfo?(amarchesini)
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3a970f41a84d
SlicedInputStream now takes ownership of the stream. r=baku
Attachment #8914216 - Attachment description: 1403771-switch-off-tests.patch → 1403771-switch-off-tests.patch [landed comment #19]
Attachment #8915084 - Attachment description: sliced.patch → sliced.patch [M-C: landed comment #49]
Attachment #8915184 - Attachment description: sliced2.patch → sliced2.patch [M-C: landed comment #56]
Attachment #8915220 - Attachment description: 1403771-sliced-ownership.patch (v2) → 1403771-sliced-ownership.patch (v2) [landed comment #57]
With the latest patches on M-C and C-C the situation is this:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&revision=55159bf4efc9b3960577e5c6f55e334a061a3723

mailnews/base/test/unit/test_folderCompact.js and mailnews/local/test/unit/test_over4GBMailboxes.js fail on Windows locally, not visible on the tree since they are switched off for Windows.

The 4GB test also fails on Linux now, but it's not a file locking issue any more but an assert:
"[3876, Main Thread] ###!!! ASSERTION: unknown error, but don't alert user.: 'errorString', file c:/mozilla-source/comm-central/mailnews/base/util/nsMsgProtocol.cpp, line 364"
"#01: nsMsgProtocol::ShowAlertMessage (c:\\mozilla-source\\comm-central\\mailnews\\base\\util\\nsmsgprotocol.cpp:364)"
"#02: nsMsgProtocol::OnStopRequest (c:\\mozilla-source\\comm-central\\mailnews\\base\\util\\nsmsgprotocol.cpp:414)"
"#03: nsMailboxProtocol::OnStopRequest (c:\\mozilla-source\\comm-central\\mailnews\\local\\src\\nsmailboxprotocol.cpp:400)"
"#04: nsInputStreamPump::OnStateStop (c:\\mozilla-source\\comm-central\\mozilla\\netwerk\\base\\nsinputstreampump.cpp:705)"
"#05: nsInputStreamPump::OnInputStreamReady (c:\\mozilla-source\\comm-central\\mozilla\\netwerk\\base\\nsinputstreampump.cpp:428)"
"#06: SlicedInputStream::RunAsyncWaitCallback (c:\\mozilla-source\\comm-central\\mozilla\\xpcom\\io\\slicedinputstream.cpp:340)"
"#07: SlicedInputStream::OnInputStreamReady (c:\\mozilla-source\\comm-central\\mozilla\\xpcom\\io\\slicedinputstream.cpp:330)"
"#08: nsInputStreamReadyEvent::Run (c:\\mozilla-source\\comm-central\\mozilla\\xpcom\\io\\nsstreamutils.cpp:99)"
"#09: nsThread::ProcessNextEvent (c:\\mozilla-source\\comm-central\\mozilla\\xpcom\\threads\\nsthread.cpp:1040)"

Aceman, could you take a look. Please also refer to the experiments described in bug 1404752 comment #14 and attachment 8915684 [details] [diff] [review] which fixes the 4GB crash on all platforms.
Flags: needinfo?(acelists)
Actually, you can see it on the tree that nsMsgProtocol::OnStopRequest() gets 0x80004005 (2147500037), NS_ERROR_FAILURE, and that's unexpected when showing the error to the user, hence the assert. In any case, something went wrong there.
OK, I've dug through the application logic.

We hold a stream in m_multipleMsgMoveCopyStream onto the mailbox, which we need to clone each time we want to access an individual message.

The clone needs to be closed by that operation, the original is closed in nsMailboxProtocol::OnStopRequest().

This fixes test_folderCompact.js.

test_over4GBMailboxes.js still fails, that has a different issue with another globally held stream 'm_inputStream'. Maybe attachment 8915684 [details] [diff] [review] is the best way to address this. I'm still thinking about it.

Andreas, I hope you can take the review here, I've explained the application logic behind the patch.
Flags: needinfo?(amarchesini)
Flags: needinfo?(acelists)
Attachment #8916219 - Flags: review?(amarchesini)
OK, I moved this from bug 1404752.
Attachment #8916222 - Flags: feedback?(rkent)
Attachment #8916222 - Flags: feedback?(acelists)
Duplicate of this bug: 1406621
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/63b02690e4f5
always close the sliced stream. rs=bustage-fix
https://hg.mozilla.org/comm-central/rev/86b8752316ff
Backed out changeset 1688845160a2 to re-enable temporarily disabled tests. a=jorgk
Attachment #8914216 - Attachment description: 1403771-switch-off-tests.patch [landed comment #19] → 1403771-switch-off-tests.patch [landed comment #19][backed out comment #63]
Comment on attachment 8916219 [details] [diff] [review]
1403771-close-sliced-stream.patch [landed comment #63]

I've landed this as a bustage fix to also fix bug 1406621 as Daily would have been quite affected if it couldn't access mbox files correctly any more causing possible corruption.

We'll have to do a post-landing review on this one, sorry.
Attachment #8916219 - Attachment description: 1403771-close-sliced-stream.patch → 1403771-close-sliced-stream.patch [landed comment #63]
With this landed, the only remaining failing test is test_over4GBMailboxes.js:
TEST-UNEXPECTED-FAIL | mailnews/local/test/unit/test_over4GBMailboxes.js | OnStopRunningUrl - [OnStopRunningUrl : 490] 2147942414 == 0

0x8007000E (2147942414), NS_ERROR_OUT_OF_MEMORY, strange.
OK, here is a less drastic solution that doesn't eliminate m_inputStream from the base class. However, like the other solution it does not clone the stream in nsMsgProtocol::LoadUrl() since the original would never be closed.

On Windows, that fixes test_over4GBMailboxes.js.

Here's a try:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=45c4129bc98275889bb7178a2b6fe798925b7702

I guess our first approach "stream in member variable, sounds like trouble, let's clone it" wasn't right without understanding the application logic.

It appears that m_inputStream is not referred to later and we can just forget it at the point when we need to do the slicing.
Attachment #8916278 - Flags: feedback?(rkent)
Attachment #8916278 - Flags: feedback?(acelists)
Attachment #8916219 - Flags: review?(amarchesini) → review+
Comment on attachment 8916278 [details] [diff] [review]
1403771-dont-clone-m_inputStream.patch [landed comment #70]

Hi Andrea, you're around on a Saturday night. What do you think about this one? Hard to say if you don't know the application logic.
Attachment #8916278 - Flags: feedback?(amarchesini)
Comment on attachment 8916222 [details] [diff] [review]
1403771-decouple_m_inputStream.patch

I'm going to use the other patch for now.
Attachment #8916222 - Attachment is obsolete: true
Attachment #8916222 - Flags: feedback?(rkent)
Attachment #8916222 - Flags: feedback?(acelists)
Keywords: leave-open
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3d4d669c0557
don't clone m_inputStream since it will never close. rs=bustage-fix
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
Comment on attachment 8916278 [details] [diff] [review]
1403771-dont-clone-m_inputStream.patch [landed comment #70]

I've landed this to finish the ongoing Necko-caused bustage that started with bug on 2017-09-27, 10 days ago. Since then Dailies weren't stable.

Post-landing review/feedback welcome. We could still eliminate m_inputStream from nsMsgProtocol.h and add it to other files using it, I'll add another patch for that.

I'm finally done here until further notice.
Attachment #8916278 - Attachment description: 1403771-dont-clone-m_inputStream.patch → 1403771-dont-clone-m_inputStream.patch [landed comment #70]
Attachment #8916278 - Flags: review?(rkent)
Attachment #8916278 - Flags: feedback?(rkent)
Attachment #8916278 - Flags: feedback?(amarchesini)
Attachment #8916278 - Flags: feedback?(acelists)
I don't know why m_inputStream is declared in nsMsgProtocol.h and used in classes derived from nsMsgProtocol. To me, the super-classes never set a value for the base-class to use. So to stop the confusion, I suggest removing it.
Attachment #8916306 - Flags: review?(rkent)
Whiteboard: [Thunderbird-testfailure: X Windows][Thunderbird-disabled-test] → [Thunderbird-testfailure: X Windows][Thunderbird-disabled-test][PLR]
Whiteboard: [Thunderbird-testfailure: X Windows][Thunderbird-disabled-test][PLR] → [Thunderbird-testfailure: X Windows][Thunderbird-temporary-fix][PLR]
Comment on attachment 8916306 [details] [diff] [review]
1403771-decouple_m_inputStream.patch - optional patch

This will be landed in bug 1419955 now.
Attachment #8916306 - Attachment is obsolete: true
Attachment #8916306 - Flags: review?(rkent)
Comment on attachment 8916278 [details] [diff] [review]
1403771-dont-clone-m_inputStream.patch [landed comment #70]

Forget the review. This has long been landed and caused no problems.
Attachment #8916278 - Flags: review?(rkent)
You need to log in before you can comment on or make changes to this bug.