1404489 - Bug 1403393 follow-up: Don't pass -1 to SlicedInputStream()

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

+++ This bug was initially created as a clone of Bug #1403393 +++

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced-minus-one.patch (v1)

I've come here from bug 1403771 comment #8.

Basically I'm trying to fix the mess done in:
https://hg.mozilla.org/comm-central/rev/5b7dc947139e10f256ae3ff40877bf6a67d37207

Whilst this certainly avoids passing -1 to SlicedInputStream(), it doesn't fix the two additional test failures on Windows:

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.

Comment 2

[Jorg K (CEST = GMT+2)]

This patch might not be necessary. uint64_t(-1)==UINT64_MAX and that's what was set here:
mStreamLength(UINT64_MAX):
https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.30

and that was passed to SlicedInputStream() here:
https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.105

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8913857 [details] [diff] [review]
1403393-sliced-minus-one.patch (v1)

Andrea, perhaps you can give me your opinion here, pass -1 or rather avoid it?

Comment 4

[Andrea Marchesini [:baku]]

Comment on attachment 8913857 [details] [diff] [review]
1403393-sliced-minus-one.patch (v1)

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

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +219,5 @@
> +    rv = sts->CreateInputTransport(slicedStream, true, getter_AddRefs(m_transport));
> +  } else {
> +    MOZ_ASSERT(aStartPosition == 0, "length <= 0 and start > 0??");
> +    rv = sts->CreateInputTransport(stream, true, getter_AddRefs(m_transport));
> +  }

I suggest to do:

RefPtr<SlicedInputStream> slicedStream =
  new SlicedInputStream(stream, uint64_t(aStartPosition),
                        aReadCount == -1 ? UINT64_MAX : uint64_t(aReadCount));
rv = sts->CreateInputTransport(stream, true, getter_AddRefs(m_transport));

::: mailnews/local/src/nsMailboxProtocol.cpp
@@ +78,5 @@
> +  } else {
> +    MOZ_ASSERT(offset == 0, "length <= 0 and start > 0??");
> +    rv = serv->CreateInputTransport(m_multipleMsgMoveCopyStream, false,
> +                                    getter_AddRefs(m_transport));
> +  }

same here.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced-minus-one.patch (v2)

Thank you very much, Andrea. I updated the patch accordingly, including another call site that wasn't in the original patch.

I hope we agree that the original code worked, uint64_t(-1), but this is clearer.

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced-minus-one.patch (v2b)

White-space change, sorry about the noise.

Comment 7

[:aceman]

Comment on attachment 8914036 [details] [diff] [review]
1403393-sliced-minus-one.patch (v2b)

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

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +214,5 @@
>    if (NS_FAILED(rv)) return rv;
>  
>    RefPtr<SlicedInputStream> slicedStream =
> +    new SlicedInputStream(stream, uint64_t(aStartPosition),
> +                          aReadCount == -1 ? UINT64_MAX : uint64_t(aReadCount));

Please document this magic value in the proper description comment block to this function (it is already mentioned at https://dxr.mozilla.org/comm-central/rev/079662f3683788017ec3c4f605863681c5e2d0e2/mailnews/local/src/nsMailboxProtocol.cpp#113).

You do this magic directly in the call to SlicedInputStream here but set m_readCount to -1 and then need to do this same conversion in nsMsgProtocol::LoadUrl below. May I suggest you do 
m_readCount = (aReadCount == -1) ? UINT64_MAX : uint64_t(aReadCount)

Also make m_readCount uint64_t and aReadCount int64_t (we pass in a uint32_t so the current int32_t is wrong too). We aim to support single messages up to 4GB in size.

@@ +476,2 @@
>          RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(m_inputStream, uint64_t(offset),

Can the offset be negative? Do we need the cast when passing int64_t to uint64_t?

@@ +476,3 @@
>          RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(m_inputStream, uint64_t(offset),
> +                                m_readCount == -1 ? UINT64_MAX : uint64_t(m_readCount));

You could just use m_readCount directly and without cast if you do what I say above in nsMsgProtocol::OpenFileSocket.

::: mailnews/local/src/nsMailboxProtocol.cpp
@@ +71,5 @@
>  
>    // XXX 64-bit
>    RefPtr<SlicedInputStream> slicedStream =
> +    new SlicedInputStream(m_multipleMsgMoveCopyStream, offset,
> +                          size == -1 ? UINT64_MAX : uint64_t(size));

Can we receive a size == -1 from the single caller we have here?
I'd rather leave this and I try a follow up with checking all the callers of SlicedInputStream and up them to 64bit as the comments request. At some cases even the offset we send is 32bit which is suspicious if the stream may represent the mbox file which may be above 4GB in size and we need to read a single message from it. But I don't know if this SlicedInputStream is used in such scenario, but I can't imagine what the offset would otherwise be.

Comment 8

[Jorg K (CEST = GMT+2)]

(In reply to :aceman from comment #7)
> Also make m_readCount uint64_t and aReadCount int64_t (we pass in a uint32_t
> so the current int32_t is wrong too). We aim to support single messages up
> to 4GB in size.
Widening the scope here, hmm? :-(

I'll see what I can do. Thank goodness this isn't busted and works as-is ;-)

Comment 9

[:aceman]

(In reply to Jorg K (GMT+2) from comment #8)
> (In reply to :aceman from comment #7)
> > Also make m_readCount uint64_t and aReadCount int64_t (we pass in a uint32_t
> > so the current int32_t is wrong too). We aim to support single messages up
> > to 4GB in size.
> Widening the scope here, hmm? :-(

I do not say to touch the callers, just make these changes inside the function you touch. But without this change you can't store what I say in m_readCount. Or do you not agree with the proposal?

Comment 10

[:aceman]

I think even aStartPosition should be uint64_t (as it is an offset which we want 64bit everywhere) but you can leave it for me if you want :)

Comment 11

[:aceman]

Attachment: 1404489.patch

I propose this cleanup of the arguments (offset int64_t or uint64_t and size at least uint32_t (int32_t is not enough).

The you could make your patch on top just setting m_readCount using specialcasing of -1 .

Also check the one settings of m_readCount = -1 in nsMsgProtocol::OpenNetworkSocketWithInfo() .

Comment 12

[:aceman]

And we still haven't found the reason for
WARNING: 'NS_FAILED(rv)', file /mozilla/xpcom/io/SlicedInputStream.cpp, line 119"

Comment 13

[Jorg K (CEST = GMT+2)]

Comment on attachment 8914103 [details] [diff] [review]
1404489.patch

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

::: mailnews/base/util/nsMsgProtocol.h
@@ +133,5 @@
>    bool          m_socketIsOpen; // mscott: we should look into keeping this state in the nsSocketTransport...
>                                    // I'm using it to make sure I open the socket the first time a URL is loaded into the connection
>    uint32_t      m_flags; // used to store flag information
>    //uint32_t  m_startPosition;
> +  uint64_t       m_readCount;

This should be int64_t.

You have m_readCount = aReadCount; above where aReadCount is of that type.

You also have m_readCount = 1; as you pointed out yourself. So why make that unsigned?

::: mailnews/local/src/nsMailboxProtocol.cpp
@@ +60,5 @@
>    // free our local state
>    delete m_lineStreamBuffer;
>  }
>  
> +nsresult nsMailboxProtocol::OpenMultipleMsgTransport(uint64_t offset, uint32_t size)

Making that unsigned is OK since that is one caller with uin32_t. However, I'd prefer to keep the interfaces consistent, unsigned for offset, signed for size with the possibility to pass -1 meaning "everything".

So how about 
uint64_t offset, int64_t size
everywhere?

Comment 14

[Jorg K (CEST = GMT+2)]

Comment on attachment 8914103 [details] [diff] [review]
1404489.patch

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

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +740,5 @@
>  
>          // create a stream pump that will async read the specified amount of data.
>          // XXX make size 64-bit int
>          RefPtr<SlicedInputStream> slicedStream =
> +          new SlicedInputStream(fileStream, offset, uint64_t(size));

why remove that? offset formally was signed.

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1404489.patch v1.1

Counter-proposal ;-)

Comment 16

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced-minus-one.patch (v3)

Rebased.

Comment 17

[:aceman]

(In reply to Jorg K (GMT+2) from comment #13)
> > +  uint64_t       m_readCount;
> 
> This should be int64_t.
 
> You have m_readCount = aReadCount; above where aReadCount is of that type.
> 
> You also have m_readCount = 1; as you pointed out yourself. So why make that
> unsigned?

It is a preparation for your where you make it never take a signed value :)
I can make it signed and then you make it unsigned immediately.
 
> ::: mailnews/local/src/nsMailboxProtocol.cpp
> @@ +60,5 @@
> >    // free our local state
> >    delete m_lineStreamBuffer;
> >  }
> >  
> > +nsresult nsMailboxProtocol::OpenMultipleMsgTransport(uint64_t offset, uint32_t size)
> 
> Making that unsigned is OK since that is one caller with uin32_t. However,
> I'd prefer to keep the interfaces consistent, unsigned for offset, signed
> for size with the possibility to pass -1 meaning "everything".
> 
> So how about 
> uint64_t offset, int64_t size
> everywhere?

I looked over the affected files and it seems to me the majority of message size variables is uint32_t. A signed size seems to be an exception so we only left it as signed in this nsMsgProtocol::OpenFileSocket if you want to keep the feature of passing in -1. But all the other interfaces seem to pass a proper size of unsigned size.

(In reply to Jorg K (GMT+2) from comment #14)
> > +          new SlicedInputStream(fileStream, offset, uint64_t(size));
> 
> why remove that? offset formally was signed.

Yes, but does it need the cast?

Comment 18

[:aceman]

Comment on attachment 8914105 [details] [diff] [review]
1404489.patch v1.1

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

Thanks, when this increases the uint32_t up to int64_t, that still fulfills my idea of fixing the few int32_t :)

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1404489.patch v1.2

Put the "cast" back in the IMAP file.

Comment 20

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced-minus-one.patch (v3b)

Added comment. Note that in the entire file there is not a single "proper description comment block".

Comment 21

[:aceman]

Comment on attachment 8914110 [details] [diff] [review]
1403393-sliced-minus-one.patch (v3b)

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

OK, so if we kept m_readCount signed, we can't move the logic around as I proposed in comment 7.
So hopefully we are done here.

Comment 22

[:aceman]

(In reply to Jorg K (GMT+2) from comment #20)
> Added comment. Note that in the entire file there is not a single "proper
> description comment block".

Then it would be a good time to start the tradition :)

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/1689cd570ccf
always use uint64_t as offset and int64_t as size for functions containing SlicedInputStream(). r=jorgk
https://hg.mozilla.org/comm-central/rev/534589b4cfaf
Bug 1403393 follow-up: Don't pass -1 as length to SlicedInputStream(). r=aceman