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

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andi [:andi]]

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 1

[Andi [:andi]]

Attachment: Bug 1287776 - eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope.

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

Comment 2

[Bobby Holley (:bholley)]

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.

Comment 3

[Bobby Holley (:bholley)]

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.

Comment 4

[Bobby Holley (:bholley)]

(whoops, sorry for the double post, I thought my review comment got dropped)

Comment 5

[Andi [:andi]]

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/

Comment 6

[Bobby Holley (:bholley)]

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

Comment 7

[Andi [:andi]]

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/

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c04758cf354
eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope. r=bholley

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2c04758cf354