1293212 - Use MOZ_MUST_USE in uriloader/

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Add MOZ_MUST_USE to prevent potential bugs.

Comment 1

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Ongoing try build:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=29e70283d74e

Comment 2

[Nicholas Nethercote [inactive]]

Thank you for doing this!

You have a few places where you've added MOZ_MUST_USE to an NS_IMETHOD function. I'm in the process of including MOZ_MUST_USE in the NS_IMETHOD macro itself, in bug 1292440, so those changes won't be necessary.

Comment 3

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Attachment: Bug 1293212 - Add MOZ_MUST_USE to prevent potential bugs.

Review commit: https://reviewboard.mozilla.org/r/70822/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/70822/

Comment 4

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a51020d5d193

(In reply to Nicholas Nethercote [:njn] from comment #2)
> Thank you for doing this!
> 
> You have a few places where you've added MOZ_MUST_USE to an NS_IMETHOD
> function. I'm in the process of including MOZ_MUST_USE in the NS_IMETHOD
> macro itself, in bug 1292440, so those changes won't be necessary.

Thanks, I've updated the patch.

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8779991 [details]
Bug 1293212 - Add MOZ_MUST_USE to prevent potential bugs.

https://reviewboard.mozilla.org/r/70822/#review68288

those fixed

::: uriloader/exthandler/ExternalHelperAppParent.h:53
(Diff revision 1)
>      NS_DECL_NSIMULTIPARTCHANNEL
>      NS_DECL_NSIRESUMABLECHANNEL
>      NS_DECL_NSISTREAMLISTENER
>      NS_DECL_NSIREQUESTOBSERVER
>  
> -    bool RecvOnStartRequest(const nsCString& entityID) override;
> +    MOZ_MUST_USE bool RecvOnStartRequest(const nsCString& entityID) override;

Why the changes to Recv*. IPC machinery will kill the child process if false is returned, and
do we really want to annotate all the IPC stuff with MOZ_MUST_USE?

So I wouldn't add the annotation to any Recv*

::: uriloader/exthandler/HandlerServiceParent.h:20
(Diff revision 1)
>   private:
>    virtual ~HandlerServiceParent();
>    virtual void ActorDestroy(ActorDestroyReason aWhy) override;
>  
>  
> -  virtual bool RecvFillHandlerInfo(const HandlerInfo& aHandlerInfoData,
> +  virtual MOZ_MUST_USE bool RecvFillHandlerInfo(const HandlerInfo& aHandlerInfoData,

also these recv

::: uriloader/exthandler/mac/nsOSHelperAppService.h:41
(Diff revision 1)
>    // platformAppPath --> a platform specific path to an application that we got out of the 
>    //                     rdf data source. This can be a mac file spec, a unix path or a windows path depending on the platform
>    // aFile --> an nsIFile representation of that platform application path.
> -  virtual nsresult GetFileTokenForPath(const char16_t * platformAppPath, nsIFile ** aFile);
> +  virtual MOZ_MUST_USE nsresult GetFileTokenForPath(const char16_t * platformAppPath, nsIFile ** aFile);
>  
> -  nsresult OSProtocolHandlerExists(const char * aScheme,
> +  MOZ_MUST_USE nsresult OSProtocolHandlerExists(const char * aScheme,

now the params don't align anymore. Please fix

Comment 6

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8779991 [details]
Bug 1293212 - Add MOZ_MUST_USE to prevent potential bugs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70822/diff/1-2/

Comment 7

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Ongoing try:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=71ff13f86047

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/26a015091dbc
Add MOZ_MUST_USE to prevent potential bugs. r=smaug

Comment 9

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/09c45bad0ba5 for OS X failures in the web-platform-test /html/webappapis/system-state-and-capabilities/the-navigator-object/protocol.html like https://treeherder.mozilla.org/logviewer.html#?job_id=25620789&repo=try

Comment 10

[Nicholas Nethercote [inactive]]

> You have a few places where you've added MOZ_MUST_USE to an NS_IMETHOD
> function. I'm in the process of including MOZ_MUST_USE in the NS_IMETHOD
> macro itself, in bug 1292440, so those changes won't be necessary.

I ended up abandoning bug 1292440 because it wasn't working well. The alternative is to use the new [must_use] property in IDL, which I added in bug 1296164.

Comment 11

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8779991 [details]
Bug 1293212 - Add MOZ_MUST_USE to prevent potential bugs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70822/diff/2-3/

Comment 12

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Fixed the failed test:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f84db78823b5

Only changed one line.

Comment 13

[Wei-Cheng Pan [:wcpan] [:wcp] [:legnaleurc] (left Mozilla)]

Comment on attachment 8779991 [details]
Bug 1293212 - Add MOZ_MUST_USE to prevent potential bugs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/70822/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/18c67e7044be
Add MOZ_MUST_USE to prevent potential bugs. r=smaug

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/18c67e7044be