The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in Thunderbird 51.0

Status

Thunderbird
General
RESOLVED FIXED
7 months ago
7 months ago

People

(Reporter: Jorg K (GMT+1), Assigned: Jorg K (GMT+1))

Tracking

unspecified
Thunderbird 51.0
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Assignee)

Description

7 months ago
Looks like more warnings got turned into errors, so we're busted.
(Assignee)

Comment 1

7 months 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.
Flags: needinfo?(richard.marti)
Flags: needinfo?(aleth)
My Mac Mini is a really slow one from 2009. If aleth isn't faster I can try it tomorrow evening.
Flags: needinfo?(richard.marti)

Comment 3

7 months 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.
(Assignee)

Comment 4

7 months ago
Yes, that's more precise ;-)
(Assignee)

Comment 5

7 months 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

Updated

7 months ago
Blocks: 1295825
Flags: needinfo?(aleth)
(Assignee)

Comment 6

7 months ago
Created attachment 8783710 [details] [diff] [review]
v1a.

Assuming that I covered all the call sites, let's get some review here.
Assignee: nobody → jorgk
Status: NEW → ASSIGNED
Attachment #8783710 - Flags: review?(rkent)
Attachment #8783710 - Flags: review?(aleth)
(Assignee)

Comment 7

7 months ago
Created attachment 8783713 [details] [diff] [review]
v1a.

Oops, wrong patch.
Attachment #8783710 - Attachment is obsolete: true
Attachment #8783710 - Flags: review?(rkent)
Attachment #8783710 - Flags: review?(aleth)
Attachment #8783713 - Flags: review?(rkent)
Attachment #8783713 - Flags: review?(aleth)

Comment 8

7 months ago
Created attachment 8783715 [details] [diff] [review]
WIP - compiles locally on OSX
(Assignee)

Comment 9

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

Comment 10

7 months 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.
Attachment #8783713 - Flags: review?(aleth) → feedback+

Comment 11

7 months 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
(Assignee)

Updated

7 months ago
Attachment #8783715 - Attachment is obsolete: true
(Assignee)

Comment 12

7 months ago
Created attachment 8783729 [details] [diff] [review]
v2a.
Attachment #8783713 - Attachment is obsolete: true
Attachment #8783713 - Flags: review?(rkent)
Attachment #8783729 - Flags: review?(rkent)
(Assignee)

Comment 13

7 months ago
Created attachment 8783731 [details] [diff] [review]
v2b.

Did a little more clean-up. Why create a pipe in nsStreamConverter.cpp which we don't use?
Attachment #8783729 - Attachment is obsolete: true
Attachment #8783729 - Flags: review?(rkent)
Attachment #8783731 - Flags: review?(rkent)

Comment 14

7 months 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.
Attachment #8783731 - Flags: review?(rkent) → review+

Updated

7 months ago
Duplicate of this bug: 1297242
(Assignee)

Comment 16

7 months ago
https://hg.mozilla.org/comm-central/rev/fca207401880
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 51.0

Updated

7 months ago
Depends on: 1297460
You need to log in before you can comment on or make changes to this bug.