Hi all,

today I had to fork nsIStreamTransportService since M-C rendered it useless.

https://bugzilla.mozilla.org/show_bug.cgi?id=1403393

Jörg.

On 27/09/2017 05:57, Joshua Cranmer 🐧 wrote:

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));

SlicedInputStream() is missing parameter: https://bugzilla.mozilla.org/show_bug.cgi?id=1402888#c14

We can back out the forked code once that has been added. Volunteers, step forward.

Jörg.

On 27/09/2017 07:45, Jörg Knobloch wrote:

On 27/09/2017 05:57, Joshua Cranmer 🐧 wrote:

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));

SlicedInputStream() is missing parameter: https://bugzilla.mozilla.org/show_bug.cgi?id=1402888#c14

We can back out the forked code once that has been added. Volunteers, step forward.

Sorry, I missed the second line. I've backed out the forked code and landed a solution as per Joshua's suggestion.

Sadly, we're busted again by something else, the next bustage came with the next M-C merge.

Jörg.

On 27/09/2017 05:57, Joshua Cranmer 🐧 wrote:

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));

After recovering from unrelated bustage I had to revisit this.

As we know, https://bugzilla.mozilla.org/show_bug.cgi?id=1403393 made changes to nsIStreamTransportService.

My first approach was to fork the code and create nsIStreamTransportService2 in C-C. That caused two test failures. Maybe I made a mistake forking or perhaps there was some interaction with the rest of Necko that the fork didn't handle.

By popular demand I backed out the forked code again and used SlicedInputStream() as had been suggested. Result:

Four test failures, the original two and two more: Details in https://bugzilla.mozilla.org/show_bug.cgi?id=1403771.

Comments welcome. Any volunteers willing to help debug this are also welcome. Ben? All hands on deck?

This seems to have caused bug https://bugzilla.mozilla.org/show_bug.cgi?id=1404003 (Daily from 2017-09-28 re-downloads and indexes messages at every start) which makes TB Daily pretty useless for IMAP use.

Jörg.

On 27/09/2017 05:57, Joshua Cranmer 🐧 wrote:

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));

As we know, https://bugzilla.mozilla.org/show_bug.cgi?id=1403393 made changes to nsIStreamTransportService.

My first approach was to fork the code and create nsIStreamTransportService2 in C-C.

By popular demand I backed out the forked code again and used SlicedInputStream() as had been suggested.

After all the other crises have been mostly cleared, I had to come back to the remaining test failures originating from replacing the original code in its forked form with calls to SlicedInputStream() - https://bugzilla.mozilla.org/show_bug.cgi?id=1403771#c13.

It turns out that compaction on Windows stopped working. Basically the Windows system call to move the compacted raw message file to replace the original message file fails with "access denied" since due to the new processing the file is still locked.

I can see that this could take many hours do debug, including Necko code. I also see some disconcerting messages come from Necko, like: WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 119

So decision we need to take here is:

• Use the working forked code with the maintenance burden or
• use the so-called non-working "proper" solution and run the risk that we lose compaction for the foreseeable future.

I vote for the fork. However wants to investigate the compaction failure can simply remove the fork and apply the SlicedInputStream() patches again.

With current staff levels we run the risk of not having compaction in TB 59 ESR, which wouldn't be a good place to be.

Jörg.