Closed Bug 1293212 Opened 3 years ago Closed 3 years ago

Use MOZ_MUST_USE in uriloader/

Categories

(Core :: Networking, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: wcpan, Assigned: wcpan)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-backlog])

Attachments

(1 file)

Add MOZ_MUST_USE to prevent potential bugs.
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.
Whiteboard: [necko-backlog]
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 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
Attachment #8779991 - Flags: review?(bugs) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/26a015091dbc
Add MOZ_MUST_USE to prevent potential bugs. r=smaug
Keywords: checkin-needed
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
Assignee: nobody → wpan
> 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.
Pushed by wpan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/18c67e7044be
Add MOZ_MUST_USE to prevent potential bugs. r=smaug
https://hg.mozilla.org/mozilla-central/rev/18c67e7044be
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.