1403393 - Port bug 1402888 to mailnews: use SlicedInputStream() since parameters of CreateInputTransport() changed

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[Jorg K (CEST = GMT+2)]

https://hg.mozilla.org/mozilla-central/rev/d434da3de9ea#l4.26

     nsITransport createInputTransport(in nsIInputStream aStream,
-                                      in long long aStartOffset,
-                                      in long long aReadLimit,
                                       in boolean aCloseWhenDone);

Used in

mailnews/base/util/nsMsgProtocol.cpp
215 rv = sts->CreateInputTransport(stream, int64_t(aStartPosition),
mailnews/local/src/nsMailboxProtocol.cpp
72 rv = serv->CreateInputTransport(m_multipleMsgMoveCopyStream, int64_t(offset),
161 rv = sts->CreateInputTransport(stream, offset,
316 rv = sts->CreateInputTransport(stream, msgOffset,

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-nsIStreamTransportService.patch

Nothing to port here, I had to adopt nsIStreamTransportService into C-C.

Comment 2

[Jorg K (CEST = GMT+2)]

Attachment: Failed attempt to use SlicedInputStream() - WIP: does not compile.

This doesn't compile. Also, SlicedInputStream() is missing the aCloseWhenDone parameter we need. I attach the patch here for reference purposes.

The fork will land once my compile has finished.

Comment 3

[Jorg K (CEST = GMT+2)]

Comment on attachment 8912483 [details] [diff] [review]
Failed attempt to use SlicedInputStream() - WIP: does not compile.

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

::: mailnews/local/src/nsMailboxProtocol.cpp
@@ +292,5 @@
>  
>                    if (NS_SUCCEEDED(rv))
>                    {
> +                    m_readCount = msgSize;
> +                    stream = new SlicedInputStream(stream, msgOffset, int64_t(msgSize));

That should be m_transport = ...

Comment 4

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-nsIStreamTransportService.patch (v2)

Needed to rename two more classes to avoid collision with M-C.

Comment 5

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-nsIStreamTransportService.patch (v3)

Added service registration.

Comment 6

[Ben Bucksch (:BenB)]

Jörg, am I understanding correctly that using SlicedInputStream is an alternative to forking nsIStreamTransportService?

If so, why it is failing to compile? Could you fix that, instead?

Forking nsIStreamTransportService and its implementation, or other core interfaces, will lead to bigger maintenance problems later on.

Comment 7

[Ben Bucksch (:BenB)]

Joshua Cranmer wrote on maildev (literal, entire quote):
=============
Big decisions should not be taken hastily. We don't have the resources to maintain core bits of Gecko or Necko.

Looking really quickly at the bug, I believe the intended fix would be to replace

rv = sts->CreateInputTransport(stream, int64_t(aStartPosition), int64_t(aReadCount), true, getter_AddRefs(m_transport));

with

RefPtr<SlicedInputStream> sliceStream = new SlicedInputStream(stream, aStartPosition, aReadCount);
rv = sts->CreateInputTransport(sliceStream, true, getter_AddRefs(m_transport));

No need for a fork or any m-c changes at all in that case.
=============

I think that's what your first patch is attempting. Even if that patch doesn't work yet, I don't see why it can't be made to work with some more effort.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8747679c08a4
Fork nsIStreamTransportService into mailnews. rs=bustage-fix

Comment 9

[Jorg K (CEST = GMT+2)]

We can back this out if M-C add the missing parameter to SlicedInputStream(), see bug 1402888 comment #14. I can't afford to have C-C broken.

Comment 10

[Jorg K (CEST = GMT+2)]

Oops, I didn't look at this carefully:
RefPtr<SlicedInputStream> sliceStream = new SlicedInputStream(stream, aStartPosition, aReadCount);
rv = sts->CreateInputTransport(sliceStream, true, getter_AddRefs(m_transport));

The second call takes the 'true' argument. OK, I'll back it out and try this tomorrow.

Comment 11

[Ben Bucksch (:BenB)]

Jörg, you had 2 objections to this patch, including one from the current module owner. Furthermore, you had no review. Please adhere to Mozilla review rules. Please leave us time to react and don't just push bad patches.

Having the tree broken just means "all hands on deck!" to fix it.

I was working on the SlicedInputStream and I'm compiling my patch, when you pushed it.

At the very least, it means that this bug here is not "FIXED" properly and should stay open.

Another alternative is to subclass the Mozilla class that misses a feature you need, instead of copying and modifying it.

Comment 12

[Ben Bucksch (:BenB)]

> OK, I'll back it out and try this tomorrow.

Thanks

Comment 13

[Ben Bucksch (:BenB)]

> +                    stream = new SlicedInputStream(stream, msgOffset, int64_t(msgSize))

If necessary, this should be uint64_t, not int64_t

Comment 14

[Ben Bucksch (:BenB)]

(source is uint32_t, target is uint64_t)

Comment 15

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced.patch (v1)

Comment 16

[Ben Bucksch (:BenB)]

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

Jörg, I was working on this as well, as I mentioned in comment 11. I was just compiling the change to verify when you posted yours. :-(

My patch is almost the same as yours.
review+, therefore.

Comment 17

[Jorg K (CEST = GMT+2)]

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

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

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

Thanks for the r+, but you didn't notice that this code above is actually wrong.

Comment 18

[Jorg K (CEST = GMT+2)]

Attachment: 1403393-sliced.patch (v1b)

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/2bb34fe36525
Backed out changeset 8747679c08a4. a=jorgk
https://hg.mozilla.org/comm-central/rev/5b7dc947139e
Port bug 1402888 to mailnews: use SlicedInputStream() since arguments of CreateInputTransport() changed. r=BenB DONTBUILD