Closed Bug 1403393 Opened 3 years ago Closed 3 years ago

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

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, 4 obsolete files)

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,
Nothing to port here, I had to adopt nsIStreamTransportService into C-C.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Summary: Port bug 1402888 to mailnews → Port bug 1402888 to mailnews: Fork nsIStreamTransportService
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 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 = ...
Needed to rename two more classes to avoid collision with M-C.
Attachment #8912477 - Attachment is obsolete: true
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.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8747679c08a4
Fork nsIStreamTransportService into mailnews. rs=bustage-fix
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
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
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)
Attached 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+
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.
Summary: Port bug 1402888 to mailnews: Fork nsIStreamTransportService → Port bug 1402888 to mailnews: use SlicedInputStream() since parameters of CreateInputTransport() changed
Attachment #8912532 - Attachment is obsolete: true
Attachment #8912532 - Flags: review?(Pidgeot18)
Attachment #8912642 - Flags: review+
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
Closed: 3 years ago3 years ago
Resolution: --- → FIXED
Blocks: 1404489
Depends on: 1403771
You need to log in before you can comment on or make changes to this bug.