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

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
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

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

Comment 1

2 years ago
Nothing to port here, I had to adopt nsIStreamTransportService into C-C.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
(Assignee)

Updated

2 years ago
Summary: Port bug 1402888 to mailnews → Port bug 1402888 to mailnews: Fork nsIStreamTransportService
(Assignee)

Comment 2

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

Comment 3

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

Comment 4

2 years ago
Needed to rename two more classes to avoid collision with M-C.
Attachment #8912477 - Attachment is obsolete: true
(Assignee)

Comment 5

2 years ago
Added service registration.
Attachment #8912492 - Attachment is obsolete: true
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.
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

2 years ago
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8747679c08a4
Fork nsIStreamTransportService into mailnews. rs=bustage-fix
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
(Assignee)

Comment 9

2 years ago
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.
Target Milestone: --- → Thunderbird 58.0
(Assignee)

Comment 10

2 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
> OK, I'll back it out and try this tomorrow.

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

If necessary, this should be uint64_t, not int64_t
(source is uint32_t, target is uint64_t)
(Assignee)

Comment 15

2 years ago
Posted patch 1403393-sliced.patch (v1) (obsolete) — Splinter Review
Attachment #8912483 - Attachment is obsolete: true
Attachment #8912532 - Flags: review?(ben.bucksch)
Attachment #8912532 - Flags: review?(Pidgeot18)
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.
Attachment #8912532 - Flags: review?(ben.bucksch) → review+
(Assignee)

Comment 17

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

Updated

2 years ago
Summary: Port bug 1402888 to mailnews: Fork nsIStreamTransportService → Port bug 1402888 to mailnews: use SlicedInputStream() since parameters of CreateInputTransport() changed
(Assignee)

Comment 18

2 years ago
Attachment #8912532 - Attachment is obsolete: true
Attachment #8912532 - Flags: review?(Pidgeot18)
Attachment #8912642 - Flags: review+

Comment 19

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

Updated

2 years ago
Blocks: 1404489
(Assignee)

Updated

2 years ago
Depends on: 1403771
You need to log in before you can comment on or make changes to this bug.