Open Bug 1308037 Opened 4 years ago Updated 4 months ago

Use MOZ_MUST_USE in image/

Categories

(Core :: ImageLib, defect, P3)

defect

Tracking

()

People

(Reporter: dmajor, Unassigned, NeedInfo)

References

(Blocks 1 open bug)

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

No description provided.
I had intended to hack on this during spare cycles, but I'm doing a bad job of getting around to it. Putting this up for grabs if anyone wants to do this before me.
Assignee: dmajor → nobody
Sounds like something I can do.

I'm guessing you mean apply the macro[1] to all functions in image/ source files, attempt to compile and see what results arnt being checked?
Please fill me in if its something I can do.

[1] https://dxr.mozilla.org/mozilla-central/source/mfbt/Attributes.h?q=MOZ_MUST_USE&redirect_type=direct#320
(In reply to Samathy Barratt from comment #2)
Feel free to give it a shot! Bug 1268766 comment 5 has some helpful advice, especially the last part about dealing with errors.
The above adds MOZ_MUST_USE throughout the header files in image/.

I just realised I've pushed that without doing the idl files yet, so I'll do them next.

However, since I'm here.
The above compiles fine on a linux build.

I have a couple of specific questions:
imgRequestProxy::SyncClone's result isnt checked in a lot of places.
The places where it isnt checked are methods that have a void return type, so can't really return anything and I'm not sure how, or if those methods need to do anything should SyncClone fail.
Currently I've just left them be.

Similarly for:
imgLoader::RemoveFromCache
imgLoader::ClearCache
imgLoader::EvictEntries

Those all seem to get used in destructors or methods with void return type.
Oops, I noticed I somehow managed to copy a whole file into another one in that earlier commit.
Frankly, I'm surprised that compiled.

Anyway, above is a commit including IDL file changes too.

Noted that some of the tests in the image/gtest compile with warnings because they don't check the return values.
I'm not sure what to do about those either.
Thanks for the patch! You can find reviewers at https://wiki.mozilla.org/Modules/All by looking up the directories you modified. In this case let's try tn.
Attachment #8936164 - Flags: review?(tnikkel)
(In reply to Samathy Barratt from comment #8)
> Noted that some of the tests in the image/gtest compile with warnings
> because they don't check the return values.
> I'm not sure what to do about those either.

Which ones are they? Because we'll need to fix them before landing.
Comment on attachment 8936164 [details]
Bug 1308037 - Use MOZ_MUST_USE in image/

https://reviewboard.mozilla.org/r/206924/#review212922

::: image/ClippedImage.h:41
(Diff revision 3)
>    NS_IMETHOD GetHeight(int32_t* aHeight) override;
>    NS_IMETHOD GetIntrinsicSize(nsSize* aSize) override;
>    NS_IMETHOD GetIntrinsicRatio(nsSize* aRatio) override;
>    NS_IMETHOD_(already_AddRefed<SourceSurface>)
>      GetFrame(uint32_t aWhichFrame, uint32_t aFlags) override;
> -  NS_IMETHOD_(already_AddRefed<SourceSurface>)
> +  MOZ_MUST_USE NS_IMETHOD_(already_AddRefed<SourceSurface>)

These five functions (GetFrameAtSize, GetImageContainer, GetImageContainerAtSize, Draw, RequestDiscard) are all defined in imgIContainer.idl, and almost all calls to them are virtual. So I think we should convert them all at once (or none at all) by changing imgIContainer.idl, otherwise we are left with a confusing state where some implementations are "must use" and some are not.

::: image/ClippedImage.h:67
(Diff revision 3)
> -                               uint32_t aWhichFrame,
> +                                            uint32_t aWhichFrame,
> -                               gfx::SamplingFilter aSamplingFilter,
> +                                            gfx::SamplingFilter aSamplingFilter,
> -                               const Maybe<SVGImageContext>& aSVGContext,
> +                                            const Maybe<SVGImageContext>& aSVGContext,
> -                               uint32_t aFlags,
> +                                            uint32_t aFlags,
> -                               float aOpacity) override;
> +                                            float aOpacity) override;
> -  NS_IMETHOD RequestDiscard() override;
> +  MOZ_MUST_USE NS_IMETHOD RequestDiscard() override;

RequestDiscard can only ever return NS_OK I think. I tend to think that we should not be checking this return value.

::: image/DecodePool.h:68
(Diff revision 3)
>     * itself to make this decision; @see IDecodingTask::ShouldPreferSyncRun(). If
>     * @aTask doesn't prefer it, just run @aTask asynchronously and return
>     * immediately.
>     * @return true if the task was run sync, false otherwise.
>     */
> -  bool SyncRunIfPreferred(IDecodingTask* aTask, const nsCString& aURI);
> +  MOZ_MUST_USE bool SyncRunIfPreferred(IDecodingTask* aTask, const nsCString& aURI);

Some callers of this just don't care about the return value, and that's fine. Either static analysis isn't catching that or it hasn't been checked yet. For example, this LaunchDecodingTask

https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/image/RasterImage.cpp#1301

doesn't use the return value of this function after it is returned.

::: image/FrozenImage.h:36
(Diff revision 3)
>  public:
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    virtual void IncrementAnimationConsumers() override;
>    virtual void DecrementAnimationConsumers() override;
>  

Same comment as for the ClippedImage.h changes.

::: image/Image.h:220
(Diff revision 3)
>     * @param aRequest  The completed request.
>     * @param aContext  Context from Necko's OnStopRequest.
>     * @param aStatus   A success or failure code.
>     * @param aLastPart Whether this is the final part of the underlying request.
>     */
> -  virtual nsresult OnImageDataComplete(nsIRequest* aRequest,
> +   virtual nsresult OnImageDataComplete(nsIRequest* aRequest,

Unintended whitespace change?

::: image/OrientedImage.h:41
(Diff revision 3)
>    NS_IMETHOD_(already_AddRefed<SourceSurface>)
>      GetFrame(uint32_t aWhichFrame, uint32_t aFlags) override;
>    NS_IMETHOD_(already_AddRefed<SourceSurface>)
>      GetFrameAtSize(const gfx::IntSize& aSize,
>                     uint32_t aWhichFrame,
>                     uint32_t aFlags) override;

Same comment as ClippedImage.h

::: image/RasterImage.h:457
(Diff revision 3)
>       * be handled safely off-main-thread. Dispatches an event which reinvokes
>       * DoError on the main thread if there isn't one already pending.
>       */
>      static void DispatchIfNeeded(RasterImage* aImage);
>  
> -    NS_IMETHOD Run();
> +    MOZ_MUST_USE NS_IMETHOD Run();

We probably should avoid adding this one, the code that calls Run is far outside of imagelib and there are tons of classes overriding this specific Run function.

::: image/VectorImage.h:44
(Diff revision 3)
>    virtual size_t SizeOfSourceWithComputedFallback(SizeOfState& aState)
>      const override;
>    virtual void CollectSizeOfSurfaces(nsTArray<SurfaceMemoryCounter>& aCounters,
>                                       MallocSizeOf aMallocSizeOf) const override;
>  
> -  virtual nsresult OnImageDataAvailable(nsIRequest* aRequest,
> +  MOZ_MUST_USE virtual nsresult OnImageDataAvailable(nsIRequest* aRequest,

This function is an override from the one defined in Image.h. We should convert all of them.

::: image/VectorImage.h:79
(Diff revision 3)
>  
>  protected:
>    explicit VectorImage(ImageURL* aURI = nullptr);
>    virtual ~VectorImage();
>  
> -  virtual nsresult StartAnimation() override;
> +  MOZ_MUST_USE virtual nsresult StartAnimation() override;

These are also overrides from Image.h.

::: image/decoders/nsICODecoder.cpp:95
(Diff revision 3)
>    }
>  
> +  nsresult rv;
> +
>    // Let the contained decoder finish up if necessary.
> -  FlushContainedDecoder();
> +  if (!FlushContainedDecoder()) {

In this case I don't think we want to return right away. I think we want to run the rest of the function and if FlushContainerDecoder fails we should return a failure rv at the end.

::: image/decoders/nsJPEGDecoder.h:58
(Diff revision 3)
>    virtual ~nsJPEGDecoder();
>  
>    void NotifyDone();
>  
>  protected:
> -  nsresult InitInternal() override;
> +  MOZ_MUST_USE nsresult InitInternal() override;

These two functions are defined in Decoder.h. We should convert them all at once.

::: image/imgLoader.cpp:1423
(Diff revision 3)
>               strcmp(aTopic, "chrome-flush-caches") == 0) {
>      MinimizeCaches();
>      ClearChromeImageCache();
>    } else if (strcmp(aTopic, "last-pb-context-exited") == 0) {
>      if (mRespectPrivacy) {
> -      ClearImageCache();
> +      if (NS_FAILED(ClearImageCache())){

I think we should just ignore the return values here and try to clear both caches no matter what. So probably just remove the annotation from these two functions.

::: image/imgRequestProxy.cpp:1180
(Diff revision 3)
>                                                              currentPrincipal);
> -  req->Init(nullptr, nullptr, aLoadingDocument, mURI, nullptr);
> +  nsresult rv = req->Init(nullptr, nullptr, aLoadingDocument, mURI, nullptr);
>  
>    NS_ADDREF(*aReturn = req);
>  
> -  return NS_OK;
> +  return rv;

If we return an error rv here then it would be expected that we don't return an imgRequestProxy in the outparam. Since the init always returns NS_OK maybe we should just skip marking this as must use?
(In reply to Timothy Nikkel (:tnikkel) from comment #10)
> (In reply to Samathy Barratt from comment #8)
> > Noted that some of the tests in the image/gtest compile with warnings
> > because they don't check the return values.
> > I'm not sure what to do about those either.
> 
> Which ones are they? Because we'll need to fix them before landing.

SourceBuffer::Append and SourceBuffer::ExpectLength throw warnings in gtest/TestSourceBuffer.cpp and gtest/TestStreamingLexer.cpp
>::: image/DecodePool.h:68
>(Diff revision 3)
>>     * itself to make this decision; @see IDecodingTask::ShouldPreferSyncRun(). If
>>     * @aTask doesn't prefer it, just run @aTask asynchronously and return
>>     * immediately.
>>     * @return true if the task was run sync, false otherwise.
>>     */
>> -  bool SyncRunIfPreferred(IDecodingTask* aTask, const nsCString& aURI);
>> +  MOZ_MUST_USE bool SyncRunIfPreferred(IDecodingTask* aTask, const nsCString& aURI);

>Some callers of this just don't care about the return value, and that's fine. Either static analysis isn't catching that or it >hasn't been checked yet. For example, this LaunchDecodingTask

>https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/image/RasterImage.cpp#1301
>
>doesn't use the return value of this function after it is returned.

I can't tell if you mean remove MOZ_MUST_USE here, or are just stating a fact and I should leave it in.
(In reply to Samathy Barratt from comment #13)
> >::: image/DecodePool.h:68
> >(Diff revision 3)
> >>     * itself to make this decision; @see IDecodingTask::ShouldPreferSyncRun(). If
> >>     * @aTask doesn't prefer it, just run @aTask asynchronously and return
> >>     * immediately.
> >>     * @return true if the task was run sync, false otherwise.
> >>     */
> >> -  bool SyncRunIfPreferred(IDecodingTask* aTask, const nsCString& aURI);
> >> +  MOZ_MUST_USE bool SyncRunIfPreferred(IDecodingTask* aTask, const nsCString& aURI);
> 
> >Some callers of this just don't care about the return value, and that's fine. Either static analysis isn't catching that or it >hasn't been checked yet. For example, this LaunchDecodingTask
> 
> >https://dxr.mozilla.org/mozilla-central/rev/a461fe03fdb07218b7f50e92c59dde64b8f8a5b0/image/RasterImage.cpp#1301
> >
> >doesn't use the return value of this function after it is returned.
> 
> I can't tell if you mean remove MOZ_MUST_USE here, or are just stating a
> fact and I should leave it in.

I think remove MOZ_MUST_USE here.
Okay, I've updated with your suggested changes.
Please see attached.
Comment on attachment 8936164 [details]
Bug 1308037 - Use MOZ_MUST_USE in image/

https://reviewboard.mozilla.org/r/206924/#review212968


C/C++ static analysis found 1 defect in this patch.

You can run this analysis locally with: `./mach static-analysis check path/to/file.cpp`


::: image/decoders/nsICODecoder.cpp:96
(Diff revision 4)
>  
> +  nsresult rv;
> +
>    // Let the contained decoder finish up if necessary.
> -  FlushContainedDecoder();
> +  if (!FlushContainedDecoder()) {
> +   rv = NS_ERROR_FAILURE;

Warning: Value stored to 'rv' is never read [clang-tidy: clang-analyzer-deadcode.DeadStores]
(In reply to Static Analysis Bot [:clangbot] from comment #17)
> Comment on attachment 8936164 [details]
> Bug 1308037 - Use MOZ_MUST_USE in image/
> 
> https://reviewboard.mozilla.org/r/206924/#review212968
> 
> 
> C/C++ static analysis found 1 defect in this patch.
> 
> You can run this analysis locally with: `./mach static-analysis check
> path/to/file.cpp`
> 
> 
> ::: image/decoders/nsICODecoder.cpp:96
> (Diff revision 4)
> >  
> > +  nsresult rv;
> > +
> >    // Let the contained decoder finish up if necessary.
> > -  FlushContainedDecoder();
> > +  if (!FlushContainedDecoder()) {
> > +   rv = NS_ERROR_FAILURE;
> 
> Warning: Value stored to 'rv' is never read [clang-tidy:
> clang-analyzer-deadcode.DeadStores]

Unsure how to correct that.
I don't know what the right way to flag an error there is.
Comment on attachment 8936164 [details]
Bug 1308037 - Use MOZ_MUST_USE in image/

https://reviewboard.mozilla.org/r/206924/#review213134

::: image/ClippedImage.h:42
(Diff revision 4)
>    NS_IMETHOD GetIntrinsicSize(nsSize* aSize) override;
>    NS_IMETHOD GetIntrinsicRatio(nsSize* aRatio) override;
>    NS_IMETHOD_(already_AddRefed<SourceSurface>)
>      GetFrame(uint32_t aWhichFrame, uint32_t aFlags) override;
>    NS_IMETHOD_(already_AddRefed<SourceSurface>)
> -    GetFrameAtSize(const gfx::IntSize& aSize,
> +                           GetFrameAtSize(const gfx::IntSize& aSize,

There are useless indent changes in this file.

::: image/FrozenImage.h:37
(Diff revision 4)
>    NS_DECL_ISUPPORTS_INHERITED
>  
>    virtual void IncrementAnimationConsumers() override;
>    virtual void DecrementAnimationConsumers() override;
>  
> -  NS_IMETHOD GetAnimated(bool* aAnimated) override;
> +  MOZ_MUST_USE NS_IMETHOD GetAnimated(bool* aAnimated) override;

All the functions annotated in this file are overrides from imgIContainer.idl, so we should mark them in imgIContainer.

::: image/Image.h:220
(Diff revision 4)
>     * @param aRequest  The completed request.
>     * @param aContext  Context from Necko's OnStopRequest.
>     * @param aStatus   A success or failure code.
>     * @param aLastPart Whether this is the final part of the underlying request.
>     */
>    virtual nsresult OnImageDataComplete(nsIRequest* aRequest,

Useless indentation change.

::: image/ImageWrapper.h:45
(Diff revision 4)
> -  virtual nsresult OnImageDataAvailable(nsIRequest* aRequest,
> +  MOZ_MUST_USE virtual nsresult OnImageDataAvailable(nsIRequest* aRequest,
> -                                        nsISupports* aContext,
> +                                                     nsISupports* aContext,
> -                                        nsIInputStream* aInStr,
> +                                                     nsIInputStream* aInStr,
> -                                        uint64_t aSourceOffset,
> +                                                     uint64_t aSourceOffset,
> -                                        uint32_t aCount) override;
> +                                                     uint32_t aCount) override;
>    virtual nsresult OnImageDataComplete(nsIRequest* aRequest,

Useless indent change.

::: image/OrientedImage.h:47
(Diff revision 4)
>    NS_IMETHOD_(bool) IsImageContainerAvailable(layers::LayerManager* aManager,
>                                                uint32_t aFlags) override;
>    NS_IMETHOD_(already_AddRefed<layers::ImageContainer>)
>      GetImageContainer(layers::LayerManager* aManager,
>                        uint32_t aFlags) override;
> -  NS_IMETHOD_(bool)
> +  MOZ_MUST_USE NS_IMETHOD_(bool)

These two functions in this file are defined in imgIContainer that we should annotate there instead.

::: image/decoders/nsICODecoder.cpp:94
(Diff revision 4)
>      return NS_OK;
>    }
>  
> +  nsresult rv;
> +
>    // Let the contained decoder finish up if necessary.

I would just save the return value of FlushContainerDecoder and check it when we set the value of rv below.

::: image/imgIContainer.idl:602
(Diff revision 4)
>  
>    %{C++
>    /*
>     * Get the set of sizes the image can decode to natively.
>     */
> -  virtual nsresult GetNativeSizes(nsTArray<nsIntSize>& aNativeSizes) const = 0;
> +   virtual nsresult GetNativeSizes(nsTArray<nsIntSize>& aNativeSizes) const = 0;

Stray indent change?
Updated those indent changes. Sorry about those.

Also added a check for FlushContainerDecode. Hopefully that was what you meant?
Comment on attachment 8936164 [details]
Bug 1308037 - Use MOZ_MUST_USE in image/

https://reviewboard.mozilla.org/r/206924/#review214382

Sorry for the delay. So close! Just need one more change.

::: image/imgIContainer.idl:516
(Diff revision 5)
>    /**
>      * Indicates that this imgIContainer has been triggered to update
>      * its internal animation state. Likely this should only be called
>      * from within nsImageFrame or objects of similar type.
>      */
> -  [notxpcom] void requestRefresh([const] in TimeStamp aTime);
> +  [notxpcom, must_use] void requestRefresh([const] in TimeStamp aTime);

There's no return value for this, so must use is useless here.
I triggered a try run with statis analysis and there are failures you will need to fix too. You can find them by clicking on the mozreview request and then the treeherder link for the try results.
Ah, not sure how that crept in there either.

I'm away from my build machine until mid-january now. 
I'll get back to this when I can.

I'll stick a needinfo on this in the hope that I'll get reminded about it.
Flags: needinfo?(samathy)
Hiya,

I'm back from my break. 
I fixed that issue you mentioned in comment 22. As far as I can see, the static analysis failures were because of that.
So it should be working properly now.

Compilation was failing because of a change I made on image/decoders/nsICODecoder.cpp:94. Trying to put a boolean return value into an nsresult.
If you could check the solution I implemented, that'd be great. Compiles fine.
Comment on attachment 8936164 [details]
Bug 1308037 - Use MOZ_MUST_USE in image/

https://reviewboard.mozilla.org/r/206924/#review218680

::: image/decoders/nsICODecoder.cpp:93
(Diff revisions 5 - 6)
>    if (!mContainedDecoder) {
>      return NS_OK;
>    }
>  
>    nsresult rv;
> +  bool rvb;

Let's just call this succeeded instead of rvb.
I triggered another try static analysis build. There are some more errors. You can find it in the same place as last time.
Priority: -- → P3
Whiteboard: [gfx-noted]
Comment on attachment 8936164 [details]
Bug 1308037 - Use MOZ_MUST_USE in image/

https://reviewboard.mozilla.org/r/206924/#review241068

This was showing up on my Bugzilla dashboard as a review that I requested. (Well, I guess I did, kind of.) It seems like this needs more work, namely the static analysis issues, so I'm clearing the review request for now.
Attachment #8936164 - Flags: review?(tnikkel)
Depends on: 1625828
You need to log in before you can comment on or make changes to this bug.