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

RESOLVED FIXED in Thunderbird 58.0

Status

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jorgk, Assigned: jorgk)

Tracking

Trunk
Thunderbird 58.0

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(6 attachments, 1 obsolete attachment)

Comment hidden (empty)

Comment 2

2 years ago
Pushed by mozilla@jorgk.com:
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
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 3

2 years ago
Many thanks to Tom Prince who pointed me to the problem!
Target Milestone: --- → Thunderbird 58.0
(Assignee)

Updated

2 years ago
Duplicate of this bug: 1403939
(Assignee)

Comment 5

2 years ago
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.
Attachment #8913345 - Flags: review?(acelists)
(Assignee)

Comment 6

2 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 7

2 years ago
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.
(Assignee)

Comment 8

2 years ago
Posted patch 1404045-follow-up.patch (v1) (obsolete) — Splinter Review
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.
Assignee: nobody → jorgk
Status: REOPENED → ASSIGNED
Attachment #8913871 - Flags: review?(acelists)
(Assignee)

Comment 9

2 years ago
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.
Attachment #8913871 - Attachment is obsolete: true
Attachment #8913871 - Flags: review?(acelists)
(Assignee)

Updated

2 years ago
Keywords: leave-open

Comment 10

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3dfe0c40ba28
follow-up: Use SlicedInputStream() where offset/length for NS_NewInputStreamPump() are necessary. rs=bustage-fix
(Assignee)

Comment 11

2 years ago
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
Flags: needinfo?(amarchesini)
(Assignee)

Updated

2 years ago
Attachment #8913345 - Attachment description: nsIInputStreamPump.patch → nsIInputStreamPump.patch [landed comment #2]
(Assignee)

Updated

2 years ago
Attachment #8913897 - Attachment description: 1404045-follow-up.patch (v2) → 1404045-follow-up.patch (v2) [landed comment #10]
(Assignee)

Comment 12

2 years ago
Here are the three options in a single patch.
Attachment #8913938 - Flags: feedback?(rkent)
Attachment #8913938 - Flags: feedback?(amarchesini)
Attachment #8913938 - Flags: feedback?(acelists)
Attachment #8913938 - Flags: feedback?(Pidgeot18)
Blocks: 1404003
(Assignee)

Comment 13

2 years ago
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
No longer blocks: 1404003
Flags: needinfo?(rkent)
(Assignee)

Comment 14

2 years ago
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

2 years ago
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().
Attachment #8913345 - Flags: review?(acelists) → review+

Comment 16

2 years ago
While looking at this, I found some unneeded inclusions of nsIInputStreamPump.h.
Attachment #8913992 - Flags: review?(jorgk)
(Assignee)

Comment 17

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

OK, but that doesn't make me happy ;-(
Attachment #8913992 - Flags: review?(jorgk) → review+

Comment 18

2 years ago
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.
Attachment #8913897 - Flags: review+
(Assignee)

Comment 19

2 years ago
(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

2 years ago
(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...
(Assignee)

Comment 21

2 years ago
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

2 years ago
Pushed by mozilla@jorgk.com:
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
(Assignee)

Updated

2 years ago
Attachment #8913992 - Attachment description: 1404045-h → 1404045-h [landed comment #22]
(Assignee)

Comment 23

2 years ago
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.
Attachment #8914000 - Attachment description: 1404045-follow-up-2.patch → 1404045-follow-up-2.patch [landed comment #22]

Comment 24

2 years ago
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.
Attachment #8914000 - Flags: review+
(Assignee)

Comment 25

2 years ago
(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.
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago2 years ago
Keywords: leave-open
Resolution: --- → FIXED
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;
}

...
Attachment #8913938 - Flags: feedback?(amarchesini) → feedback+
(Assignee)

Updated

2 years ago
Attachment #8913938 - Flags: feedback?(rkent)
Attachment #8913938 - Flags: feedback?(acelists)
Attachment #8913938 - Flags: feedback?(Pidgeot18)
(Assignee)

Updated

2 years ago
Attachment #8913938 - Attachment description: 1404045-follow-up-2.patch - WIP → 1404045-follow-up-2.patch - WIP - not landed, for demonstration purposed only.
(Assignee)

Comment 27

2 years ago
Thank you Andrea, maybe you can put your seal of approval onto this patch.
Flags: needinfo?(rkent)
Flags: needinfo?(amarchesini)
Attachment #8914279 - Flags: review?(amarchesini)
Attachment #8914279 - Flags: review?(amarchesini) → review+
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

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/20ea7ac93345
Follow-up: Protect against Tell() failure. r=baku
(Assignee)

Comment 30

2 years ago
Thanks, I liked your original suggestion better.
(Assignee)

Updated

2 years ago
Attachment #8914279 - Attachment description: 1404045-follow.patch - Last follow-up → 1404045-follow.patch - Last follow-up [landed comment #27]
You need to log in before you can comment on or make changes to this bug.