1287743 - [Static Analysis][Assignment in Assert] In function nsHTMLFramesetFrame::Reflow

Status: RESOLVED FIXED

(Core :: Layout, 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.

Comment 1

[Andi [:andi]]

The part of the code that should be changed is:

>>         DebugOnly<nsHTMLFramesetBlankFrame*> blank;
>>         MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame type");
>>         childVis = NONE_VIS;

with 

>>         DebugOnly<nsHTMLFramesetBlankFrame*> blank;
>>         blank = do_QueryFrame(child);
>>         MOZ_ASSERT(blank, "unexpected child frame type");
>>         childVis = NONE_VIS;

Comment 2

[Andi [:andi]]

Attachment: Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

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

Comment 3

[Daniel Holbert [:dholbert]]

Comment on attachment 8772381 [details]
Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

https://reviewboard.mozilla.org/r/65208/#review62272

This needs to change a bit.  There's a reason "do_QueryFrame(child)" happens inside of the MOZ_ASSERT right now -- do_QueryFrame is expensive, and we don't want to unnecessarily take that cost in optimized builds.

So, this needs to stay debug-only. If we can't do the assignment inside of MOZ_ASSERT anymore, then the right way to fix this is:
 (1) Add #ifdef DEBUG just before the "blank" decl.
 (2) Just declare/assign blank on a single line and without DebugOnly wrapper:
    nsHTMLFramesetBlankFrame* blank = do_QueryFrame(child);
 (3) Add an #endif after the assertion.

Comment 4

[Andi [:andi]]

(In reply to Daniel Holbert [:dholbert] from comment #3)
> Comment on attachment 8772381 [details]
> Bug 1287743 - assign |blank| outside of the assert.
> 
> https://reviewboard.mozilla.org/r/65208/#review62272
> 
> This needs to change a bit.  There's a reason "do_QueryFrame(child)" happens
> inside of the MOZ_ASSERT right now -- do_QueryFrame is expensive, and we
> don't want to unnecessarily take that cost in optimized builds.
> 
> So, this needs to stay debug-only. If we can't do the assignment inside of
> MOZ_ASSERT anymore, then the right way to fix this is:
>  (1) Add #ifdef DEBUG just before the "blank" decl.
>  (2) Just declare/assign blank on a single line and without DebugOnly
> wrapper:
>     nsHTMLFramesetBlankFrame* blank = do_QueryFrame(child);
>  (3) Add an #endif after the assertion.

Thanks for letting me know, i didn't know that do_QueryFrame is that expensive. I will update the patch.

Comment 5

[Andi [:andi]]

Comment on attachment 8772381 [details]
Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65208/diff/1-2/

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8772381 [details]
Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

https://reviewboard.mozilla.org/r/65208/#review62294

Commit message needs a tweak.  It's too specific about something that doesn't matter (nobody reading the commit message is going to have any idea what |blank| is), and not specific enough about what does matter (the area of code that's changing and why it's changing).

I'd suggest you replace the commit message with something like:
 Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

(That drops the unnecessary-specificity about |blank|, and gives a better hint about what's changing & why & that it likely shouldn't affect behavior.)

::: layout/generic/nsFrameSetFrame.cpp:983
(Diff revision 2)
>            childVis = (eFrameborder_No == frameborder) ? NONE_VIS : ALL_VIS;
>          }
>        } else {  // blank
> -        DebugOnly<nsHTMLFramesetBlankFrame*> blank;
> -        MOZ_ASSERT(blank = do_QueryFrame(child), "unexpected child frame type");
> +#ifdef DEBUG
> +        nsHTMLFramesetBlankFrame* blank;
> +        blank = do_QueryFrame(child);

Can't we do the declaration/assignment in a single line, per my review comment labeled (2) above?

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8772381 [details]
Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

https://reviewboard.mozilla.org/r/65208/#review62296

Sorry, I meant to tag this as "r+ with those review nits addressed". (No need for another round of review.)

Thanks for fixing this!

Comment 8

[Andi [:andi]]

Comment on attachment 8772381 [details]
Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/65208/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/528caba5bd20
Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue. r=dholbert

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/528caba5bd20