Closed Bug 1250134 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dereference after null check] In function nsCycleCollector::CollectWhite

Categories

(Core :: XPCOM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1353455)

Attachments

(1 file)

The Static Analysis tool Coverity added that pointer |mJSRuntime| is dereferenced after it's null checked:

null checked:
>>  bool hasJSRuntime = !!mJSRuntime;
>>  nsCycleCollectionParticipant* zoneParticipant =
>>    hasJSRuntime ? mJSRuntime->ZoneParticipant() : nullptr; 

dereferencing:
>>        mJSRuntime->AddZoneWaitingForGC(zone);
>>      } else {
I can review cycle collector changes.

This null check is actually redundant because IsGrayJS() can only be true if mJSRuntime is true. Maybe there could be an assert for that or something because it isn't directly obvious from the code.
Or something like MOZ_RELEASE_ASSERT(mJSRuntime) if that would make Coverity happier.
Attachment #8721977 - Flags: review-
(In reply to Andrew McCreight [:mccr8] from comment #3)
> Or something like MOZ_RELEASE_ASSERT(mJSRuntime) if that would make Coverity
> happier.
I've added MOZ_ASSERT since coverity runs on a debug build and 't think we need this kind of assert on our release as well.
Attachment #8721977 - Attachment description: MozReview Request: Bug 1250134 - call AddZoneWaitingForGC only if mJSRuntime is not nullptr. r?nfroyd → MozReview Request: Bug 1250134 - assert mJSRuntime when IsGrayJS() is true. r?mccr8
Attachment #8721977 - Flags: review- → review?(continuation)
Comment on attachment 8721977 [details]
MozReview Request: Bug 1250134 - assert mJSRuntime when IsGrayJS() is true. r?mccr8

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/35815/diff/1-2/
Comment on attachment 8721977 [details]
MozReview Request: Bug 1250134 - assert mJSRuntime when IsGrayJS() is true. r?mccr8

https://reviewboard.mozilla.org/r/35815/#review32685

Thanks.
Attachment #8721977 - Flags: review?(continuation) → review+
https://hg.mozilla.org/mozilla-central/rev/bf62a6c05fa7
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.