1444852 - nsIFrame::BuildDisplayListForChild: Value stored to 'wrapListASR' during its initialization is never read

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Sylvestre Ledru [:Sylvestre]]

I am doing a presentation at Paris Diderot.
I am now explaining how to fix a bug in Firefox!

Comment 1

[Sylvestre Ledru [:Sylvestre]]

Attachment: Bug 1444852 - nsIFrame::BuildDisplayListForChild Remove the init step as the result is unused

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

Comment 2

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8958065 [details]
Bug 1444852 - nsIFrame::BuildDisplayListForChild Remove the init step as the result is unused

https://reviewboard.mozilla.org/r/227010/#review232750

::: layout/generic/nsFrame.cpp:3744
(Diff revision 1)
>      awayFromCommonPath = true;
>    }
>  
>    nsDisplayList list;
>    nsDisplayList extraPositionedDescendants;
> -  const ActiveScrolledRoot* wrapListASR = aBuilder->CurrentActiveScrolledRoot();
> +  const ActiveScrolledRoot* wrapListASR;

In general having uninitialized pointers around is pretty scary, and `CurrentActiveScrolledRoot` should be just a member offset so should be cheap...

Perhaps the giant `if` / `else` block below should be moved to a helper function that returns the ASR along with all the other stuff like `awayFromCommonPath` and such.

This is fine for me, but I don't want to introduce a footgun, and I'm not that familiar with display-list building to know how likely this is to become a footgun, so 301 Miko who can probably decide on how this should be fixed / whether this is ok. Maybe landing this is ok for now :)

Comment 3

[Miko Mynttinen]

(In reply to Emilio Cobos Álvarez [:emilio] from comment #2)
> Perhaps the giant `if` / `else` block below should be moved to a helper
> function that returns the ASR along with all the other stuff like
> `awayFromCommonPath` and such.

Yeah, this is pretty ugly. Looks like the branch for stacking context/pseudo-stacking context could be combined to reuse some code.

Comment 4

[Miko Mynttinen]

Comment on attachment 8958065 [details]
Bug 1444852 - nsIFrame::BuildDisplayListForChild Remove the init step as the result is unused

https://reviewboard.mozilla.org/r/227010/#review232754

r+ since this was found by static analysis.

Comment 5

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8958065 [details]
Bug 1444852 - nsIFrame::BuildDisplayListForChild Remove the init step as the result is unused

https://reviewboard.mozilla.org/r/227010/#review232754

Thanks!

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/09af3a6b460a
nsIFrame::BuildDisplayListForChild Remove the init step as the result is unused r=miko

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/09af3a6b460a