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

RESOLVED FIXED in Firefox 50

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: andi, Assigned: andi)

Tracking

(Blocks: 1 bug)

Trunk
mozilla50
Points:
---

Firefox Tracking Flags

(firefox50 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

2 years ago
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);
(Assignee)

Comment 1

2 years ago
Created attachment 8772393 [details]
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/
Attachment #8772393 - Flags: review?(bobbyholley)
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)
(Assignee)

Comment 5

2 years ago
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+
(Assignee)

Comment 7

2 years ago
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

2 years ago
Pushed by bpostelnicu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c04758cf354
eliminate assignment from MOZ_ASSERT in EnsureContentXBLScope. r=bholley

Comment 9

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c04758cf354
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox50: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.