1297133 - Port |Bug 1295825 - Add a [must_use] attribute to XPIDL| to Mailnews

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

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

Comment 1

[Jorg K (CEST = GMT+2)]

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.

Comment 2

[Richard Marti (:Paenglab)]

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

Comment 3

[aleth [:aleth]]

(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.

Comment 4

[Jorg K (CEST = GMT+2)]

Yes, that's more precise ;-)

Comment 5

[Jorg K (CEST = GMT+2)]

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

Comment 6

[Jorg K (CEST = GMT+2)]

Attachment: v1a.

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

Comment 7

[Jorg K (CEST = GMT+2)]

Attachment: v1a.

Oops, wrong patch.

Comment 8

[aleth [:aleth]]

Attachment: WIP - compiles locally on OSX

Comment 9

[Jorg K (CEST = GMT+2)]

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

Comment 10

[aleth [:aleth]]

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.

Comment 11

[aleth [:aleth]]

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

Comment 12

[Jorg K (CEST = GMT+2)]

Attachment: v2a.

Comment 13

[Jorg K (CEST = GMT+2)]

Attachment: v2b.

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

Comment 14

[Kent James (:rkent)]

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.

Comment 16

[Jorg K (CEST = GMT+2)]

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