Closed
Bug 1404045
Opened 7 years ago
Closed 7 years ago
Port bug 1402888 to C-C: Remove position and length parameter of nsIInputStreamPump.init
Categories
(Thunderbird :: General, enhancement)
Thunderbird
General
Tracking
(Not tracked)
RESOLVED
FIXED
Thunderbird 58.0
People
(Reporter: jorgk-bmo, Assigned: jorgk-bmo)
References
Details
Attachments
(6 files, 1 obsolete file)
3.59 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
3.90 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
2.41 KB,
patch
|
baku
:
feedback+
|
Details | Diff | Splinter Review |
2.38 KB,
patch
|
jorgk-bmo
:
review+
|
Details | Diff | Splinter Review |
1.71 KB,
patch
|
aceman
:
review+
|
Details | Diff | Splinter Review |
1.36 KB,
patch
|
baku
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•7 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/39f86b4979da port bug 1402888 to C-C: Remove position and length parameter of nsIInputStreamPump.init. rs=bustage-fix
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 3•7 years ago
|
||
Many thanks to Tom Prince who pointed me to the problem!
Target Milestone: --- → Thunderbird 58.0
Assignee | ||
Comment 5•7 years ago
|
||
Comment on attachment 8913345 [details] [diff] [review] nsIInputStreamPump.patch [landed comment #2] Aceman, can you please check whether I missed anything here. This patch at least brought Mozmill back and I fixed all the other spots where input-stream-pump was used in the code. But I'm not 100% certain that I covered everything.
Attachment #8913345 -
Flags: review?(acelists)
Assignee | ||
Comment 6•7 years ago
|
||
OK, I looked a little closer at https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l6.10 and saw that parameters to NS_NewInputStreamPump() have also changed. We didn't get any compile errors, but these also need changing: https://dxr.mozilla.org/comm-central/source/mailnews/base/util/nsMsgProtocol.cpp#471 https://dxr.mozilla.org/comm-central/source/mailnews/imap/src/nsImapProtocol.cpp#9659 https://dxr.mozilla.org/comm-central/source/mailnews/news/src/nsNNTPProtocol.cpp#743 I don't know what to do here, since the parameters have simply gone.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Comment 7•7 years ago
|
||
I forgot to say: Of the eight calls, only three are affected: https://dxr.mozilla.org/comm-central/search?q=NS_NewInputStreamPump&redirect=false The ones where you can't see the |);| on the same line.
Assignee | ||
Comment 8•7 years ago
|
||
This fixes the two test failures from bug 1403771: mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js mozilla/mach xpcshell-test mailnews/imap/test/unit/test_compactOfflineStore.js.
Assignee | ||
Comment 9•7 years ago
|
||
I'm not sure how to deal with the rv = NS_NewInputStreamPump(getter_AddRefs(pump), m_inputStream, -1, m_readCount); in nsMsgProtocol.cpp yet, but the IMAP and NNTP changes are correct according to https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.78 and https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.105 and they fix two test failures, so I'm landing them now.
Attachment #8913871 -
Attachment is obsolete: true
Attachment #8913871 -
Flags: review?(acelists)
Assignee | ||
Updated•7 years ago
|
Keywords: leave-open
Comment 10•7 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/3dfe0c40ba28 follow-up: Use SlicedInputStream() where offset/length for NS_NewInputStreamPump() are necessary. rs=bustage-fix
Assignee | ||
Comment 11•7 years ago
|
||
With that landed, these don't fail any more mozilla/mach xpcshell-test mailnews/db/gloda/test/unit/test_gloda_content_imap_offline.js mozilla/mach xpcshell-test mailnews/imap/test/unit/test_compactOfflineStore.js These two from bug 1403771 still fail: mozilla/mach xpcshell-test mailnews/base/test/unit/test_folderCompact.js mozilla/mach xpcshell-test mailnews/local/test/unit/test_over4GBMailboxes.js So let's get back to nsMsgProtocol.cpp where we have: nsCOMPtr<nsIInputStreamPump> pump; rv = NS_NewInputStreamPump(getter_AddRefs(pump), m_inputStream, -1, m_readCount); if (NS_FAILED(rv)) return rv; This needs to be replace with + RefPtr<SlicedInputStream> slicedStream = + new SlicedInputStream(m_inputStream, ???, uint64_t(m_readCount)); + rv = NS_NewInputStreamPump(getter_AddRefs(pump), slicedStream); but the question is, what to pass to SlicedInputStream() where previously there was a -1 meaning the "current" position. I see three options: Pass 0. In case where the input stream as created above if (!m_inputStream) rv = m_transport->OpenInputStream(0, 0, 0, getter_AddRefs(m_inputStream)); I'm pretty sure that it "current" means 0. Pass -1: uint64_t(-1)==UINT64_MAX and there was some indication that this is what used to happen here: https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.78 QI the stream to nsISeekableStream and do a Tell() to find the current position. Here are the results: Pass 0: test_folderCompact.js fails with testDeleteMessages2 FAIL [testDeleteMessages2 : 208] 2324 == 4956 as per bug 1404489 comment #1. test_over4GBMailboxes.js fails with OnStopRunningUrl FAIL [OnStopRunningUrl : 535] false == true as per bug 1404489 comment #1. Pass -1: test_folderCompact.js and test_over4GBMailboxes.js crash at xpcom/io/nsPipe3.cpp, line 1526 Seek: Same as passing 0. No surprise since the debug output is: Tell says: Offset=0. How I love try and error in software development. Andrea: What's the proper replacement for this code: nsCOMPtr<nsIInputStreamPump> pump; rv = NS_NewInputStreamPump(getter_AddRefs(pump), m_inputStream, -1, m_readCount); if (NS_FAILED(rv)) return rv; https://dxr.mozilla.org/comm-central/rev/e9367f02e475d6487615993f484be0a6b0f85597/mailnews/base/util/nsMsgProtocol.cpp#472
Flags: needinfo?(amarchesini)
Assignee | ||
Updated•7 years ago
|
Attachment #8913345 -
Attachment description: nsIInputStreamPump.patch → nsIInputStreamPump.patch [landed comment #2]
Assignee | ||
Updated•7 years ago
|
Attachment #8913897 -
Attachment description: 1404045-follow-up.patch (v2) → 1404045-follow-up.patch (v2) [landed comment #10]
Assignee | ||
Comment 12•7 years ago
|
||
Here are the three options in a single patch.
Attachment #8913938 -
Flags: feedback?(rkent)
Attachment #8913938 -
Flags: feedback?(amarchesini)
Attachment #8913938 -
Flags: feedback?(acelists)
Attachment #8913938 -
Flags: feedback?(Pidgeot18)
Assignee | ||
Comment 13•7 years ago
|
||
Additionally to the patch I added: if (!m_inputStream) { // open buffered, asynchronous input stream + printf("=== opening new stream\n"); and some more debug in nsMsgProtocol::CloseSocket() where m_inputStream is nulled. Clicking on various messages in a local folder I see this debug: === SlicedInputStream::SlicedInputStream 208399 3210 === opening new stream === nsMsgProtocol::LoadUrl, Tell says: Offset=0 === SlicedInputStream::SlicedInputStream 0 3210 [7408, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 120 === nulling input stream === nulling input stream Doing other operations, like copying messages, I also always see a new stream and offset 0. So some call to SlicedInputStream() appears to position into the mailbox file and then tell always tells 0 since it's a new stream. So either 0 or the tell solution would do the trick here. At the end, m_inputStream is nulled, so it would be interesting to know under which circumstances that's used more than once. If never used more than once, we can just remove m_inputStream and always do a new stream. Kent, thoughts on that? A little disconcerting that I see: [1260, Main Thread] WARNING: 'NS_FAILED(rv)', file c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line 120
No longer blocks: 1404003
Flags: needinfo?(rkent)
Assignee | ||
Comment 14•7 years ago
|
||
Magnus, blocking 1404003 was removed by a mid-air collision, but even with the patches here, bug 1404003 still happens, so I guess it's unrelated.
Comment 15•7 years ago
|
||
Comment on attachment 8913345 [details] [diff] [review] nsIInputStreamPump.patch [landed comment #2] Review of attachment 8913345 [details] [diff] [review]: ----------------------------------------------------------------- Yes, it seems you found all the JS callers of nsIInputStreamPump.init().
Attachment #8913345 -
Flags: review?(acelists) → review+
Comment 16•7 years ago
|
||
While looking at this, I found some unneeded inclusions of nsIInputStreamPump.h.
Attachment #8913992 -
Flags: review?(jorgk)
Assignee | ||
Comment 17•7 years ago
|
||
Comment on attachment 8913992 [details] [diff] [review] 1404045-h [landed comment #22] OK, but that doesn't make me happy ;-(
Attachment #8913992 -
Flags: review?(jorgk) → review+
Comment 18•7 years ago
|
||
Comment on attachment 8913897 [details] [diff] [review] 1404045-follow-up.patch (v2) [landed comment #10] Review of attachment 8913897 [details] [diff] [review]: ----------------------------------------------------------------- Yes, when we pass the proper offsets and lengths just into the another interface, this could work. The guesswork about 0 or -1 default positions is a different matter.
Attachment #8913897 -
Flags: review+
Assignee | ||
Comment 19•7 years ago
|
||
(In reply to :aceman from comment #18) > Yes, when we pass the proper offsets and lengths just into the another > interface, this could work. Please observe that the pump init originally used exactly that interface: https://hg.mozilla.org/mozilla-central/rev/560296786c0c#l15.105 Now all I've done is move the call from inside the pump init to outside. Pump related test failures clear. > The guesswork about 0 or -1 default positions is a different matter. Indeed. I think we might need to wait for Andrea's answer on how to best replace that or on Kent's answer re. re-use of the stream. In all my testing a new stream was created, leading to tell==0, and the stream variable was later nulled, see comment #13. So if it were me, I'd remove m_inputStream completely and always use 0.
Comment 20•7 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #13) > A little disconcerting that I see: > [1260, Main Thread] WARNING: 'NS_FAILED(rv)', file > c:/mozilla-source/comm-central/mozilla/xpcom/io/SlicedInputStream.cpp, line > 120 The new 'cool' syntax of 'if(NS_WARN_IF(NS_FAILED(rv))) return rv;' and it does not even tell the rv value...
Assignee | ||
Comment 21•7 years ago
|
||
I will land this with my next push. Currently we have a big gaping and bleeding wound which we need to close. We're calling NS_NewInputStreamPump() completely incorrectly, so that needs to be fixed. Once we have better insights, we can land a follow up removing the Tell() or m_inputStream.
Comment 22•7 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/b70a05002bb5 remove unneeded inclusions of nsIInputStreamPump.h. r=jorgk https://hg.mozilla.org/comm-central/rev/3c0dde60442e follow-up 2: Use SlicedInputStream() where offset/length for NS_NewInputStreamPump() are necessary. rs=bustage-fix
Assignee | ||
Updated•7 years ago
|
Attachment #8913992 -
Attachment description: 1404045-h → 1404045-h [landed comment #22]
Assignee | ||
Comment 23•7 years ago
|
||
Comment on attachment 8914000 [details] [diff] [review] 1404045-follow-up-2.patch [landed comment #22] This fixes the wrong NS_NewInputStreamPump(getter_AddRefs(pump), slicedStream); call, we can work out later what the best/correct solution is here. My proposal is to remove m_inputStream.
Attachment #8914000 -
Attachment description: 1404045-follow-up-2.patch → 1404045-follow-up-2.patch [landed comment #22]
Comment 24•7 years ago
|
||
Comment on attachment 8914000 [details] [diff] [review] 1404045-follow-up-2.patch [landed comment #22] Review of attachment 8914000 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, looks like the safest option.
Attachment #8914000 -
Flags: review+
Assignee | ||
Comment 25•7 years ago
|
||
(In reply to Jorg K (GMT+2) from comment #23) > My proposal is to remove m_inputStream. Big mistake. I removed it from nsMsgProtocol.h/cpp and got heaps of compilation errors from the subclass nsImapProtocol. So that member can't be removed. Since we don't know about the reuse, the Tell() option is indeed the safest option. So let's assume we're done here unless we get different feedback from Andrea or Kent. Kent, please look at attachment 8914000 [details] [diff] [review] or the landed version https://hg.mozilla.org/comm-central/rev/3c0dde60442e and tell me whether you're happy with it.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
Keywords: leave-open
Resolution: --- → FIXED
Comment 26•7 years ago
|
||
Comment on attachment 8913938 [details] [diff] [review] 1404045-follow-up-2.patch - WIP - not landed, for demonstration purposed only. Review of attachment 8913938 [details] [diff] [review]: ----------------------------------------------------------------- First of all I would like to know what the problem is. Clearly here we are triggering a bug in SlicedInputStream or in how it is used in NS_NewInputStreamPump(). Can you ping me on IRC (baku) and tell me how to reproduce it? Thanks! ::: mailnews/base/util/nsMsgProtocol.cpp @@ +470,5 @@ > +#if 0 > +// Option: pass 0 > + // Note that SlicedInputStream() accepts uint64_t(-1)==UINT64_MAX for the length. > + RefPtr<SlicedInputStream> slicedStream = > + new SlicedInputStream(m_inputStream, 0, uint64_t(m_readCount)); This is correct. @@ +476,5 @@ > +#if 0 > +// Option: pass -1 > + // Note that SlicedInputStream() accepts uint64_t(-1)==UINT64_MAX for the length. > + RefPtr<SlicedInputStream> slicedStream = > + new SlicedInputStream(m_inputStream, uint64_t(-1), uint64_t(m_readCount)); This is wrong. having the starting point == -1, will produce no data to read. @@ +491,5 @@ > + } > + > + // Note that SlicedInputStream() accepts uint64_t(-1)==UINT64_MAX for the length. > + RefPtr<SlicedInputStream> slicedStream = > + new SlicedInputStream(m_inputStream, uint64_t(offset), uint64_t(m_readCount)); This could be right, but if written like this: uint64_t offset = 0; nsCOMPtr<nsISeekableStream> seekable(do_QueryInterface(m_inputStream)); if (seekable && NS_FAILED(seekable->Tell(&offset)) { offset = 0; } ...
Attachment #8913938 -
Flags: feedback?(amarchesini) → feedback+
Assignee | ||
Updated•7 years ago
|
Attachment #8913938 -
Flags: feedback?(rkent)
Attachment #8913938 -
Flags: feedback?(acelists)
Attachment #8913938 -
Flags: feedback?(Pidgeot18)
Assignee | ||
Updated•7 years ago
|
Attachment #8913938 -
Attachment description: 1404045-follow-up-2.patch - WIP → 1404045-follow-up-2.patch - WIP - not landed, for demonstration purposed only.
Assignee | ||
Comment 27•7 years ago
|
||
Thank you Andrea, maybe you can put your seal of approval onto this patch.
Flags: needinfo?(rkent)
Flags: needinfo?(amarchesini)
Attachment #8914279 -
Flags: review?(amarchesini)
Updated•7 years ago
|
Attachment #8914279 -
Flags: review?(amarchesini) → review+
Comment 28•7 years ago
|
||
Comment on attachment 8914279 [details] [diff] [review] 1404045-follow.patch - Last follow-up [landed comment #27] Review of attachment 8914279 [details] [diff] [review]: ----------------------------------------------------------------- ::: mailnews/base/util/nsMsgProtocol.cpp @@ +471,5 @@ > } > > int64_t offset = 0; > nsCOMPtr<nsISeekableStream> seekable(do_QueryInterface(m_inputStream)); > + if (seekable && NS_FAILED(seekable->Tell(&offset))) { or you can also do: if (seekable) { rv = seekable->Tell(&offset); if (NS_WARN_IF(NS_FAILED(rv))) return rv; }
Comment 29•7 years ago
|
||
Pushed by mozilla@jorgk.com: https://hg.mozilla.org/comm-central/rev/20ea7ac93345 Follow-up: Protect against Tell() failure. r=baku
Assignee | ||
Comment 30•7 years ago
|
||
Thanks, I liked your original suggestion better.
Assignee | ||
Updated•7 years ago
|
Attachment #8914279 -
Attachment description: 1404045-follow.patch - Last follow-up → 1404045-follow.patch - Last follow-up [landed comment #27]
You need to log in
before you can comment on or make changes to this bug.
Description
•