Closed Bug 1404489 Opened 4 years ago Closed 4 years ago

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

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 58.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

Details

Attachments

(2 files, 6 obsolete files)

+++ This bug was initially created as a clone of Bug #1403393 +++
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.
Attachment #8913857 - Flags: feedback?(acelists)
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 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?
Attachment #8913857 - Flags: feedback?(amarchesini)
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.
Attachment #8913857 - Flags: feedback?(amarchesini)
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.
Attachment #8913857 - Attachment is obsolete: true
Attachment #8913857 - Flags: feedback?(acelists)
Attachment #8914035 - Flags: review?(acelists)
White-space change, sorry about the noise.
Attachment #8914035 - Attachment is obsolete: true
Attachment #8914035 - Flags: review?(acelists)
Attachment #8914036 - Flags: review?(acelists)
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.
Attachment #8914036 - Flags: feedback+
(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 ;-)
(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?
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 :)
Attached patch 1404489.patch (obsolete) — Splinter Review
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() .
Attachment #8914103 - Flags: review?(jorgk)
And we still haven't found the reason for
WARNING: 'NS_FAILED(rv)', file /mozilla/xpcom/io/SlicedInputStream.cpp, line 119"
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 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.
Attached patch 1404489.patch v1.1 (obsolete) — Splinter Review
Counter-proposal ;-)
Attachment #8914105 - Flags: review+
Attachment #8914105 - Flags: feedback?(acelists)
Rebased.
Attachment #8914036 - Attachment is obsolete: true
Attachment #8914036 - Flags: review?(acelists)
Attachment #8914106 - Flags: review?(acelists)
(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 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 :)
Attachment #8914105 - Flags: feedback?(acelists) → feedback+
Put the "cast" back in the IMAP file.
Attachment #8914105 - Attachment is obsolete: true
Attachment #8914109 - Flags: review+
Attachment #8914109 - Flags: feedback?(acelists)
Attachment #8914109 - Flags: feedback?(acelists) → feedback+
Added comment. Note that in the entire file there is not a single "proper description comment block".
Attachment #8914106 - Attachment is obsolete: true
Attachment #8914106 - Flags: review?(acelists)
Attachment #8914110 - Flags: review?(acelists)
Attachment #8914103 - Attachment is obsolete: true
Attachment #8914103 - Flags: review?(jorgk)
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.
Attachment #8914110 - Flags: review?(acelists) → review+
(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 :)
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Pushed by mozilla@jorgk.com:
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
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 58.0
You need to log in before you can comment on or make changes to this bug.