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

Status: RESOLVED FIXED

(Core :: DOM: IndexedDB, defect)

Description

[Andi-Bogdan Postelnicu [:andi]]

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);

Comment 1

[Andi-Bogdan Postelnicu [:andi]]

Attachment: Bug 1287756 - prevent assignment in assert expression for |mCurrentMaintenance|.

Review commit: https://reviewboard.mozilla.org/r/65216/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/65216/

Comment 2

[Daniel Holbert [:dholbert] (AFK from Oct 16-23)]

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.

Comment 3

[Andi-Bogdan Postelnicu [:andi]]

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/

Comment 4

[Andi-Bogdan Postelnicu [:andi]]

(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

Comment 5

[Jan Varga [:janv]]

Comment on attachment 8772389 [details]
Bug 1287756 - prevent assignment in assert expression for |mCurrentMaintenance|.

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

r=me, Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/99837fd92258
prevent assignment in assert expression for |mCurrentMaintenance|.r=janv

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/99837fd92258