Jorg K (CEST = GMT+2) Assignee
	Description • 8 years ago

Looks like more warnings got turned into errors, so we're busted.

	Jorg K (CEST = GMT+2) Assignee
	Comment 1 • 8 years ago

Aleth or Richard, I don't have a Mac, so I can only do this very painfully a few errors at a time, all I can see on treeherder. Right now, I saw eight which I fixed:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=5d376f556adc

We can speed things up if you can give me a full list of errors from a local compile.

You can just place (void) in front of the call for now and keep going. I can add error handling later. Or Aleth, you can take this bug.

	Richard Marti (:Paenglab)
	Comment 2 • 8 years ago

My Mac Mini is a really slow one from 2009. If aleth isn't faster I can try it tomorrow evening.

	aleth [:aleth]
	Comment 3 • 8 years ago

(In reply to Jorg K (GMT+2, PTO during summer) from comment #0)
> Looks like more warnings got turned into errors, so we're busted.

Looking at the regressing bug, it's not that these warnings weren't enabled before, it's that the certain interfaces got a must_use added, to make sure the return value is checked.

	Jorg K (CEST = GMT+2) Assignee
	Comment 4 • 8 years ago

Yes, that's more precise ;-)

	Jorg K (CEST = GMT+2) Assignee
	Comment 5 • 8 years ago

Here is another one:
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0a862425193c

With some luck that could be all:
https://dxr.mozilla.org/comm-central/search?q=pipe-%3EGetInput&redirect=false

	Jorg K (CEST = GMT+2) Assignee
	Comment 6 • 8 years ago

Assuming that I covered all the call sites, let's get some review here.

	Jorg K (CEST = GMT+2) Assignee
	Comment 7 • 8 years ago

Oops, wrong patch.

	Jorg K (CEST = GMT+2) Assignee
	Comment 9 • 8 years ago

Thanks, that's the six files I have. But I actually check the errors now.

	aleth [:aleth]
	Comment 10 • 8 years ago

Comment on attachment 8783713 [details] [diff] [review]
v1a.

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

This basically looks r+ to me, but see what you think about these comments:

::: mailnews/addrbook/src/nsAddbookProtocolHandler.cpp
@@ +162,5 @@
>  
> +      rv = pipe->GetInputStream(getter_AddRefs(pipeIn));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = pipe->GetOutputStream(getter_AddRefs(pipeOut));
> +      NS_ENSURE_SUCCESS(rv, rv);

I think you can use MOZ_ALWAYS_SUCCEEDS here together with a comment, following https://hg.mozilla.org/mozilla-central/rev/44f472638f79#l2.51

Not sure if it matters though.

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +1425,5 @@
>      mInStream = dont_AddRef(static_cast<nsIInputStream *>(inputStream));
>  
>      nsIAsyncOutputStream *outputStream = nullptr;
> +    rv = pipe->GetOutputStream(&outputStream);
> +    NS_ENSURE_SUCCESS(rv, rv);

Same here.

::: mailnews/imap/src/nsImapProtocol.cpp
@@ +3027,5 @@
>        // we create an "infinite" pipe in case we get extremely long lines from the imap server,
>        // and the consumer is waiting for a whole line
>        nsCOMPtr<nsIPipe> pipe = do_CreateInstance("@mozilla.org/pipe;1");
>        rv = pipe->Init(false, false, 4096, PR_UINT32_MAX);
>        NS_ASSERTION(NS_SUCCEEDED(rv), "nsIPipe->Init failed!");

Not sure why this is an assertion - that won't help in opt builds.

@@ +3032,5 @@
>  
> +      rv = pipe->GetInputStream(getter_AddRefs(m_channelInputStream));
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      rv = pipe->GetOutputStream(getter_AddRefs(m_channelOutputStream));
> +      NS_ENSURE_SUCCESS(rv, rv);

Could be MOZ_ALWAYS_SUCCEEDS if the pipe->Init rv check was made reliable.

::: mailnews/mime/src/nsStreamConverter.cpp
@@ +628,5 @@
>    {
> +    rv = pipe->GetInputStream(getter_AddRefs(mInputStream));
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    rv = pipe->GetOutputStream(getter_AddRefs(mOutputStream));
> +    NS_ENSURE_SUCCESS(rv, rv);

see above

::: mailnews/news/src/nsNNTPProtocol.cpp
@@ +2086,5 @@
>    //
>    if (m_channelListener) {
>        nsCOMPtr<nsIPipe> pipe = do_CreateInstance("@mozilla.org/pipe;1");
>        mozilla::DebugOnly<nsresult> rv = pipe->Init(false, false, 4096, PR_UINT32_MAX);
>        NS_ASSERTION(NS_SUCCEEDED(rv), "failed to create pipe");

Removing the NS_ASSERTION here in a way that doesn't cause a mess is what the //TODO comment was about I think.

	aleth [:aleth]
	Comment 11 • 8 years ago

Also interesting to see what the pipe->Init failure handler looks like here, compared to the c-c code: https://hg.mozilla.org/mozilla-central/rev/44f472638f79#l2.43

	Jorg K (CEST = GMT+2) Assignee
	Comment 13 • 8 years ago

Did a little more clean-up. Why create a pipe in nsStreamConverter.cpp which we don't use?

	Kent James (:rkent)
	Comment 14 • 8 years ago

Comment on attachment 8783731 [details] [diff] [review]
v2b.

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

I can't confirm the issues on OSX and Linux, as my builders are now all obsolete for comm-central on those platforms. But the changes make sense, others (and try) have confirmed the changes there, so this is fine.

	Jorg K (CEST = GMT+2) Assignee
	Comment 16 • 8 years ago

https://hg.mozilla.org/comm-central/rev/fca207401880