Closed Bug 1287743 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, defect)

defect
Not set

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.
No longer blocks: coverity-analysis
Blocks: 1287752
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 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.
Attachment #8772381 - Flags: review?(dholbert) → review-
(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 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/
Attachment #8772381 - Flags: review- → review?(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?
Attachment #8772381 - Flags: review?(dholbert) → review-
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!
Attachment #8772381 - Flags: review- → review+
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/
Attachment #8772381 - Attachment description: Bug 1287743 - assign |blank| outside of the assert. → Bug 1287743 - Shift an assignment to be outside of an assert, in nsFrameSetFrame, to address a static-analysis issue.
Pushed by bpostelnicu@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/528caba5bd20
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.