Closed Bug 1240708 Opened 4 years ago Closed 4 years ago

Various trivial coverity warnings

Categories

(Core :: Graphics: Layers, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: [CID 1314987][CID 1324680][CID 749990][CID 1123688][CID 1311569][CID 1314986][CID 1325960][CID 1123713][CID 1244663][CID 1314936])

Attachments

(2 files)

Fixing a bunch of trivial warnings in one bug to reduce the noise and avoid some potential crashes.
Assignee: nobody → nical.bugzilla
Keywords: coverity
Whiteboard: [CID 1314987][CID 1324680][CID 749990][CID 1123688]
Whiteboard: [CID 1314987][CID 1324680][CID 749990][CID 1123688] → [CID 1314987][CID 1324680][CID 749990][CID 1123688][CID 1311569][CID 1314986][CID 1325960]
Mostly uninitialized members and a null check. Most or all of the uninitialized members are legit but since they confuse the static analysis and certainly aren't in hot code paths it's better to just initialize them.
Attachment #8709404 - Flags: review?(bugmail.mozilla)
Comment on attachment 8709404 [details] [diff] [review]
various fixes in gfx/layers

Review of attachment 8709404 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/ImageContainer.cpp
@@ +60,5 @@
>    : mLock("mozilla.layers.BufferRecycleBin.mLock")
> +  // This member is only valid when the bin is not empty and will be properly
> +  // initialized in RecycleBuffer, but initializing it here avoids static analysis
> +  // noise.
> +  , mRecycledBufferSize(0) 

nit: trailing whitespace

::: gfx/layers/apz/src/HitTestingTreeNode.cpp
@@ +24,5 @@
>    : mApzc(aApzc)
>    , mIsPrimaryApzcHolder(aIsPrimaryHolder)
>    , mLayersId(aLayersId)
> +  // Some arbitrary bit pattern that we wouldn't easily confuse with a valid id.
> +  , mScrollViewId(0xffff)

You can initialize this to FrameMetrics::NULL_SCROLL_ID and drop the comment

@@ +31,3 @@
>    , mOverride(EventRegionsOverride::NoOverride)
>  {
> +if (mIsPrimaryApzcHolder) {

nit: restore indent
Attachment #8709404 - Flags: review?(bugmail.mozilla) → review+
Whiteboard: [CID 1314987][CID 1324680][CID 749990][CID 1123688][CID 1311569][CID 1314986][CID 1325960] → [CID 1314987][CID 1324680][CID 749990][CID 1123688][CID 1311569][CID 1314986][CID 1325960][CID 1123713][CID 1244663]
Whiteboard: [CID 1314987][CID 1324680][CID 749990][CID 1123688][CID 1311569][CID 1314986][CID 1325960][CID 1123713][CID 1244663] → [CID 1314987][CID 1324680][CID 749990][CID 1123688][CID 1311569][CID 1314986][CID 1325960][CID 1123713][CID 1244663][CID 1314936]
Another wave of trivial coverity fixes. It'll be all for this bug because I filled the whiteboard and can't add any more coverity ids.
Attachment #8709516 - Flags: review?(bugmail.mozilla)
Comment on attachment 8709516 [details] [diff] [review]
Fix some more coverity warnings

Review of attachment 8709516 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with comments addressed

::: gfx/gl/GLLibraryLoader.cpp
@@ +82,5 @@
>  
>              const char *s = ss->symNames[i];
> +            if (strnlen(s, MAX_SYMBOL_LENGTH) < MAX_SYMBOL_LENGTH) {
> +
> +            }

Not sure if this is useful. I would instead put this inside the if (prefix..) block below:

if (strlen(s) + strlen(prefix) >= MAX_SYMBOL_LENGTH * 2)) {
    break;
}

although I don't know if it would fix your coverity warning.
Attachment #8709516 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #4)
> Comment on attachment 8709516 [details] [diff] [review]
> Fix some more coverity warnings
> 
> Review of attachment 8709516 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r+ with comments addressed
> 
> ::: gfx/gl/GLLibraryLoader.cpp
> @@ +82,5 @@
> >  
> >              const char *s = ss->symNames[i];
> > +            if (strnlen(s, MAX_SYMBOL_LENGTH) < MAX_SYMBOL_LENGTH) {
> > +
> > +            }
> 
> Not sure if this is useful. I would instead put this inside the if
> (prefix..) block below:
> 
> if (strlen(s) + strlen(prefix) >= MAX_SYMBOL_LENGTH * 2)) {
>     break;
> }
> 
> although I don't know if it would fix your coverity warning.

Ah, sorry I didn't mean for this to stay in the patch. the strings here are hard-coded gl symbols that don't depend on external inputs and are always far from the size limit so I ended up silencing the warning in coverity but I failed to remove entirely the piece of code I had started writing.
https://hg.mozilla.org/mozilla-central/rev/7f0b471f88e9
https://hg.mozilla.org/mozilla-central/rev/e5913edef613
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f1ab7a126d3
Follow-up to repair indentation damage. r=me and DONTBUILD
You need to log in before you can comment on or make changes to this bug.