[Static Analysis][Logically dead code] In functions PushNotifier::NotifySubscriptionChangeWorkers and PushNotifier::NotifyPushWorkers

RESOLVED FIXED in Firefox 47

Status

()

Core
DOM: Push Notifications
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug, {coverity})

Trunk
mozilla47
coverity
Points:
---

Firefox Tracking Flags

(firefox46 affected, firefox47 fixed)

Details

(Whiteboard: CID 1349970, CID 1349969)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

3 years ago
The Static Analysis tool Coverity added that variable ok is assign false an thus determining a logically dead code. 
For example, the below code from NotifySubscriptionChangeWorkers: 

>>  // Parent process, e10s enabled.
>>  bool ok = false;
>>  nsTArray<ContentParent*> contentActors;
>>  ContentParent::GetAll(contentActors);
>>  for (uint32_t i = 0; i < contentActors.Length(); ++i) {
>>    ok &= contentActors[i]->SendPushSubscriptionChange(
>>      PromiseFlatCString(aScope), IPC::Principal(aPrincipal));
>> }
>>  return ok ? NS_OK : NS_ERROR_FAILURE;

regardless of the value from contentActors[i]->SendPushSubscriptionChange, ok will always be false and in the end function will always return NS_ERROR_FAILURE.

I guess that default value for ok should have been true, and in this way if SendPushSubscriptionChange had failed value of ok would change to false, resulting that function would return  NS_ERROR_FAILURE otherwise returns NS_OK.
(Assignee)

Comment 1

3 years ago
Created attachment 8711640 [details]
MozReview Request: Bug 1242436 - default value of ok = true in order to check the return of SendPush and SendPushSubscriptionChange. r?kitcambridge

Review commit: https://reviewboard.mozilla.org/r/32277/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/32277/
(Assignee)

Comment 2

3 years ago
Comment on attachment 8711640 [details]
MozReview Request: Bug 1242436 - default value of ok = true in order to check the return of SendPush and SendPushSubscriptionChange. r?kitcambridge

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/32277/diff/1-2/
Attachment #8711640 - Attachment description: MozReview Request: Bug 1242436 - default value of ok = true in order to check the return of SendPush and SendPushSubscriptionChange. r?kcambridge@mozilla.com → MozReview Request: Bug 1242436 - default value of ok = true in order to check the return of SendPush and SendPushSubscriptionChange. r?kitcambridge
Attachment #8711640 - Flags: review?(kcambridge)
Comment on attachment 8711640 [details]
MozReview Request: Bug 1242436 - default value of ok = true in order to check the return of SendPush and SendPushSubscriptionChange. r?kitcambridge

https://reviewboard.mozilla.org/r/32277/#review28943

Thanks for catching that!
Attachment #8711640 - Flags: review?(kcambridge) → review+
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(Assignee)

Updated

3 years ago
Keywords: checkin-needed

Comment 5

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/fd1b82f7fbee
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46

Updated

3 years ago
Target Milestone: mozilla46 → mozilla47
You need to log in before you can comment on or make changes to this bug.