Closed Bug 1287756 Opened 3 years ago Closed 3 years ago

[Static Analysis][Assignment in Assert] In function NoteFinishedMaintenance

Categories

(Core :: DOM: IndexedDB, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(1 file)

Our own static analysis tool based on clang triggered that an assignment is present inside an assert expression:

>>    AssertIsOnBackgroundThread();
>>    MOZ_ASSERT(aMaintenance);
>>    MOZ_ASSERT(mCurrentMaintenance = aMaintenance);
>>
>>    mCurrentMaintenance = nullptr;
>>    ProcessMaintenanceQueue();

I think the correct behaviour here would have been to have a logical expression instead of an assignment, like:

>>    MOZ_ASSERT(mCurrentMaintenance == aMaintenance);
Attachment #8772389 - Flags: review?(dholbert) → review?(bent.mozilla)
Component: Layout → DOM: IndexedDB
hg blame says that Jan Varga added this line, reviewed by khuey:
 https://hg.mozilla.org/mozilla-central/rev/ca8e9cc4f661#l1.108

One of them would be the most appropriate reviewer, probably.  I believe bent is largely inactive, as noted by his bugzilla handle.
Flags: needinfo?(bpostelnicu)
Comment on attachment 8772389 [details]
Bug 1287756 - prevent assignment in assert expression for |mCurrentMaintenance|.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65216/diff/1-2/
Attachment #8772389 - Flags: review?(bent.mozilla) → review?(jvarga)
(In reply to Daniel Holbert [:dholbert] from comment #2)
> hg blame says that Jan Varga added this line, reviewed by khuey:
>  https://hg.mozilla.org/mozilla-central/rev/ca8e9cc4f661#l1.108
> 
> One of them would be the most appropriate reviewer, probably.  I believe
> bent is largely inactive, as noted by his bugzilla handle.

I've forwarded this to Jan
Flags: needinfo?(bpostelnicu)
Comment on attachment 8772389 [details]
Bug 1287756 - prevent assignment in assert expression for |mCurrentMaintenance|.

https://reviewboard.mozilla.org/r/65216/#review62352

r=me, Thanks!
Attachment #8772389 - Flags: review?(jvarga) → review+
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/99837fd92258
prevent assignment in assert expression for |mCurrentMaintenance|.r=janv
https://hg.mozilla.org/mozilla-central/rev/99837fd92258
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.