Closed Bug 1287776 Opened 5 years ago Closed 5 years ago

[Static Analysis][Assignment in Assert] In function XPCWrappedNativeScope::EnsureContentXBLScope

Categories

(Core :: XPConnect, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: andi, Assigned: andi)

References

Details

Attachments

(1 file)

Sometimes assignments from assert calls can have side effects and for this we implemented a static analysis checker: https://bugzilla.mozilla.org/show_bug.cgi?id=1283395
In this particular case there is no side effect but in order to be able to integrate the checker the assignment must be done outside of the assert.

>>     nsIPrincipal* principal = GetPrincipal();
>>     nsCOMPtr<nsIExpandedPrincipal> ep;
>>     MOZ_ASSERT(!(ep = do_QueryInterface(principal)));
>>     nsTArray< nsCOMPtr<nsIPrincipal> > principalAsArray(1);
>>     principalAsArray.AppendElement(principal);

We should do the assignment outside of the assert expression, like:

>>    nsIPrincipal* principal = GetPrincipal();
>>    nsCOMPtr<nsIExpandedPrincipal> ep;
>>    ep = do_QueryInterface(principal);
>>    MOZ_ASSERT(!ep);
>>    nsTArray< nsCOMPtr<nsIPrincipal> > principalAsArray(1);
Comment on attachment 8772393 [details]
Bug 1287776 - eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope.

https://reviewboard.mozilla.org/r/65220/#review62274

This work is debug-only, so we don't want to do an unnecessary QI in release builds. Please just use nsContentUtils::IsExpandedPrincipal and put that directly inside the MOZ_ASSERT.
Attachment #8772393 - Flags: review?(bobbyholley) → review-
https://reviewboard.mozilla.org/r/65220/#review62276

This causes us to do an unnecessary QI in release builds. Please use nsContentUtils::IsExpandedPrincipal directly inside the MOZ_ASSERT instead.
(whoops, sorry for the double post, I thought my review comment got dropped)
Comment on attachment 8772393 [details]
Bug 1287776 - eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65220/diff/1-2/
Attachment #8772393 - Attachment description: Bug 1287776 - do assignment for |ep| outside of MOZ_ASSERT. → Bug 1287776 - eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope.
Attachment #8772393 - Flags: review- → review?(bobbyholley)
Comment on attachment 8772393 [details]
Bug 1287776 - eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope.

https://reviewboard.mozilla.org/r/65220/#review62308

r=me with that fixed.

::: js/xpconnect/src/XPCWrappedNativeScope.cpp:298
(Diff revision 2)
> +    nsCOMPtr<nsIExpandedPrincipal> ep;
>      ep = new nsExpandedPrincipal(principalAsArray);

You can combine this in one line:

nsCOMPtr<nsIExpandedPrincipal> ep = new nsExpandedPrincipal(principalAsArray);
Attachment #8772393 - Flags: review?(bobbyholley) → review+
Comment on attachment 8772393 [details]
Bug 1287776 - eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65220/diff/2-3/
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c04758cf354
eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope. r=bholley
https://hg.mozilla.org/mozilla-central/rev/2c04758cf354
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.