1404045 - Port bug 1402888 to C-C: Remove position and length parameter of nsIInputStreamPump.init

Status: RESOLVED FIXED

(Thunderbird :: General, enhancement)

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: nsIInputStreamPump.patch [landed comment #2]

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/39f86b4979da
port bug 1402888 to C-C: Remove position and length parameter of nsIInputStreamPump.init. rs=bustage-fix

Comment 3

[Jorg K (CEST = GMT+2)]

Many thanks to Tom Prince who pointed me to the problem!

Comment 5

[Jorg K (CEST = GMT+2)]

Comment on attachment 8913345 [details] [diff] [review]
nsIInputStreamPump.patch [landed comment #2]

Aceman, can you please check whether I missed anything here. This patch at least brought Mozmill back and I fixed all the other spots where input-stream-pump was used in the code. But I'm not 100% certain that I covered everything.

Comment 6

[Jorg K (CEST = GMT+2)]

OK, I looked a little closer at
https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l6.10
and saw that parameters to NS_NewInputStreamPump() have also changed.

We didn't get any compile errors, but these also need changing:
https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#471
https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#9659
https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp#743

I don't know what to do here, since the parameters have simply gone.

Comment 7

[Jorg K (CEST = GMT+2)]

I forgot to say: Of the eight calls, only three are affected:
https://dxr.mozilla.org/comm-central/search?q=NS_NewInputStreamPump&redirect=false
The ones where you can't see the |);| on the same line.

Comment 8

[Jorg K (CEST = GMT+2)]

Attachment: 1404045-follow-up.patch (v1)

This fixes the two test failures from bug 1403771:
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.

Comment 9

[Jorg K (CEST = GMT+2)]

Attachment: 1404045-follow-up.patch (v2) [landed comment #10]

I'm not sure how to deal with the
        rv = NS_NewInputStreamPump(getter_AddRefs(pump),
          m_inputStream, -1, m_readCount);
in nsMsgProtocol.cpp yet, but the IMAP and NNTP changes are correct according to
https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.78 and 
https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.105
and they fix two test failures, so I'm landing them now.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/3dfe0c40ba28
follow-up: Use SlicedInputStream() where offset/length for NS_NewInputStreamPump() are necessary. rs=bustage-fix

Comment 11

[Jorg K (CEST = GMT+2)]

With that landed, these don't fail any more
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

These two from bug 1403771 still fail:
mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js
mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js

So let's get back to nsMsgProtocol.cpp where we have:
        nsCOMPtr<nsIInputStreamPump> pump;
        rv = NS_NewInputStreamPump(getter_AddRefs(pump),
          m_inputStream, -1, m_readCount);
        if (NS_FAILED(rv)) return rv;

This needs to be replace with
+          RefPtr<SlicedInputStream> slicedStream =
+            new SlicedInputStream(m_inputStream, ???, uint64_t(m_readCount));
+          rv = NS_NewInputStreamPump(getter_AddRefs(pump), slicedStream);

but the question is, what to pass to SlicedInputStream() where previously there was a -1 meaning the "current" position.

I see three options:
Pass 0.  In case where the input stream as created above
         if (!m_inputStream)
           rv = m_transport->OpenInputStream(0, 0, 0, getter_AddRefs(m_inputStream));
         I'm pretty sure that it "current" means 0.
Pass -1: uint64_t(-1)==UINT64_MAX and there was some indication that this is what used to happen here:
         https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.78
QI the stream to nsISeekableStream and do a Tell() to find the current position.

Here are the results:
Pass 0:
test_folderCompact.js fails with
  testDeleteMessages2 FAIL [testDeleteMessages2 : 208] 2324 == 4956
as per bug 1404489 comment #1.
test_over4GBMailboxes.js fails with
  OnStopRunningUrl FAIL [OnStopRunningUrl : 535] false == true
as per bug 1404489 comment #1.

Pass -1:
test_folderCompact.js and test_over4GBMailboxes.js crash at xpcom/io/nsPipe3.cpp, line 1526

Seek:
Same as passing 0. No surprise since the debug output is: Tell says: Offset=0.

How I love try and error in software development.

Andrea: What's the proper replacement for this code:
        nsCOMPtr<nsIInputStreamPump> pump;
        rv = NS_NewInputStreamPump(getter_AddRefs(pump),
          m_inputStream, -1, m_readCount);
        if (NS_FAILED(rv)) return rv;
https://dxr.mozilla.org/comm-central/rev/e9367f02e475d6487615993f484be0a6b0f85597/mailnews/base/util/nsMsgProtocol.cpp#472

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: 1404045-follow-up-2.patch - WIP - not landed, for demonstration purposed only.

Here are the three options in a single patch.

Comment 13

[Jorg K (CEST = GMT+2)]

Additionally to the patch I added:
         if (!m_inputStream)
         {
           // open buffered, asynchronous input stream
+          printf("=== opening new stream\n");
and some more debug in nsMsgProtocol::CloseSocket() where m_inputStream is nulled.

Clicking on various messages in a local folder I see this debug:
=== SlicedInputStream::SlicedInputStream 208399 3210
=== opening new stream
=== nsMsgProtocol::LoadUrl, Tell says: Offset=0
=== SlicedInputStream::SlicedInputStream 0 3210
[7408, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 120
=== nulling input stream
=== nulling input stream

Doing other operations, like copying messages, I also always see a new stream and offset 0.

So some call to SlicedInputStream() appears to position into the mailbox file and then tell always tells 0 since it's a new stream. So either 0 or the tell solution would do the trick here.

At the end, m_inputStream is nulled, so it would be interesting to know under which circumstances that's used more than once. If never used more than once, we can just remove m_inputStream and always do a new stream.

Kent, thoughts on that?

A little disconcerting that I see:
[1260, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 120

Comment 14

[Jorg K (CEST = GMT+2)]

Magnus, blocking 1404003 was removed by a mid-air collision, but even with the patches here, bug 1404003 still happens, so I guess it's unrelated.

Comment 15

[:aceman]

Comment on attachment 8913345 [details] [diff] [review]
nsIInputStreamPump.patch [landed comment #2]

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

Yes, it seems you found all the JS callers of nsIInputStreamPump.init().

Comment 16

[:aceman]

Attachment: 1404045-h [landed comment #22]

While looking at this, I found some unneeded inclusions of nsIInputStreamPump.h.

Comment 17

[Jorg K (CEST = GMT+2)]

Comment on attachment 8913992 [details] [diff] [review]
1404045-h [landed comment #22]

OK, but that doesn't make me happy ;-(

Comment 18

[:aceman]

Comment on attachment 8913897 [details] [diff] [review]
1404045-follow-up.patch (v2) [landed comment #10]

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

Yes, when we pass the proper offsets and lengths just into the another interface, this could work.
The guesswork about 0 or -1 default positions is a different matter.

Comment 19

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #18)
> Yes, when we pass the proper offsets and lengths just into the another
> interface, this could work.
Please observe that the pump init originally used exactly that interface:
https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.105
Now all I've done is move the call from inside the pump init to outside. Pump related test failures clear.

> The guesswork about 0 or -1 default positions is a different matter.
Indeed.

I think we might need to wait for Andrea's answer on how to best replace that or on Kent's answer re. re-use of the stream. In all my testing a new stream was created, leading to tell==0, and the stream variable was later nulled, see comment #13. So if it were me, I'd remove m_inputStream completely and always use 0.

Comment 20

[:aceman]

(In reply to Jorg K (GMT+2) from comment #13)
> A little disconcerting that I see:
> [1260, Main Thread] WARNING: 'NS_FAILED(rv)', file
> c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line
> 120

The new 'cool' syntax of 'if(NS_WARN_IF(NS_FAILED(rv))) return rv;' and it does not even tell the rv value...

Comment 21

[Jorg K (CEST = GMT+2)]

Attachment: 1404045-follow-up-2.patch [landed comment #22]

I will land this with my next push. Currently we have a big gaping and bleeding wound which we need to close. We're calling NS_NewInputStreamPump() completely incorrectly, so that needs to be fixed. Once we have better insights, we can land a follow up removing the Tell() or m_inputStream.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b70a05002bb5
remove unneeded inclusions of nsIInputStreamPump.h. r=jorgk
https://hg.mozilla.org/comm-central/rev/3c0dde60442e
follow-up 2: Use SlicedInputStream() where offset/length for NS_NewInputStreamPump() are necessary. rs=bustage-fix

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8914000 [details] [diff] [review]
1404045-follow-up-2.patch [landed comment #22]

This fixes the wrong NS_NewInputStreamPump(getter_AddRefs(pump), slicedStream); call, we can work out later what the best/correct solution is here.

My proposal is to remove m_inputStream.

Comment 24

[:aceman]

Comment on attachment 8914000 [details] [diff] [review]
1404045-follow-up-2.patch [landed comment #22]

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

Thanks, looks like the safest option.

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to Jorg K (GMT+2) from comment #23)
> My proposal is to remove m_inputStream.
Big mistake. I removed it from nsMsgProtocol.h/cpp and got heaps of compilation errors from the subclass nsImapProtocol.

So that member can't be removed. Since we don't know about the reuse, the Tell() option is indeed the safest option.

So let's assume we're done here unless we get different feedback from Andrea or Kent.

Kent, please look at attachment 8914000 [details] [diff] [review] or the landed version
https://hg.mozilla.org/comm-central/rev/3c0dde60442e
and tell me whether you're happy with it.

Comment 26

[Andrea Marchesini [:baku]]

Comment on attachment 8913938 [details] [diff] [review]
1404045-follow-up-2.patch - WIP - not landed, for demonstration purposed only.

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

First of all I would like to know what the problem is.
Clearly here we are triggering a bug in SlicedInputStream or in how it is used in NS_NewInputStreamPump().
Can you ping me on IRC (baku) and tell me how to reproduce it? Thanks!

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +470,5 @@
> +#if 0
> +// Option: pass 0
> +        // Note that SlicedInputStream() accepts uint64_t(-1)==UINT64_MAX for the length.
> +        RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(m_inputStream, 0, uint64_t(m_readCount));

This is correct.

@@ +476,5 @@
> +#if 0
> +// Option: pass -1
> +        // Note that SlicedInputStream() accepts uint64_t(-1)==UINT64_MAX for the length.
> +        RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(m_inputStream, uint64_t(-1), uint64_t(m_readCount));

This is wrong. having the starting point == -1, will produce no data to read.

@@ +491,5 @@
> +        }
> +
> +        // Note that SlicedInputStream() accepts uint64_t(-1)==UINT64_MAX for the length.
> +        RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(m_inputStream, uint64_t(offset), uint64_t(m_readCount));

This could be right, but if written like this:

uint64_t offset = 0;
nsCOMPtr<nsISeekableStream> seekable(do_QueryInterface(m_inputStream));
if (seekable && NS_FAILED(seekable->Tell(&offset)) {
  offset = 0;
}

...

Comment 27

[Jorg K (CEST = GMT+2)]

Attachment: 1404045-follow.patch - Last follow-up [landed comment #27]

Thank you Andrea, maybe you can put your seal of approval onto this patch.

Comment 28

[Andrea Marchesini [:baku]]

Comment on attachment 8914279 [details] [diff] [review]
1404045-follow.patch - Last follow-up [landed comment #27]

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

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +471,5 @@
>          }
>  
>          int64_t offset = 0;
>          nsCOMPtr<nsISeekableStream> seekable(do_QueryInterface(m_inputStream));
> +        if (seekable && NS_FAILED(seekable->Tell(&offset))) {

or you can also do:

if (seekable) {
  rv = seekable->Tell(&offset);
  if (NS_WARN_IF(NS_FAILED(rv))) return rv;
}

Comment 29

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/20ea7ac93345
Follow-up: Protect against Tell() failure. r=baku

Comment 30

[Jorg K (CEST = GMT+2)]

Thanks, I liked your original suggestion better.