Bug 161739 – [css-grid] Fix a dangling reference

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sergio Villar Senin]

[css-grid] Fix a dangling reference

[Sergio Villar Senin]

Created attachment 288268 [details]
Patch

[Manuel Rego Casasnovas]

Comment on attachment 288268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288268&action=review

r=me

> Source/WebCore/rendering/RenderGrid.cpp:2009
> +        GridLength maxTrackSize = gridTrackSize(ForRows, trackPosition, sizingOperation).maxTrackBreadth();

Nit: You could even use "auto" instead of "GridLength".

[Alexey Proskuryakov]

Comment on attachment 288268 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=288268&action=review

> Source/WebCore/ChangeLog:9
> +        The code was trying to get a reference to a private attribute of a temporary object returned
> +        by gridTrackSize().

What symptom did you observe?

This code is correct in C++ - a const reference extends lifetime of a temporary. Also, this pattern is repeated multiple times in this file, so if this were a problem, it would need to be addressed in more than this single spot.

[Alexey Proskuryakov]

Comment on attachment 288268 [details]
Patch

Looking again, I misread the code. This does have a problem.

Looks like we need a test that would catch it under ASan.

[Sergio Villar Senin]

(In reply to comment #4)
> Comment on attachment 288268 [details]
> Patch
> 
> Looking again, I misread the code. This does have a problem.
> 
> Looks like we need a test that would catch it under ASan.

Correct me if I'm wrong but the ASan build is only currently available for the Mac port?

I don't currently have access to a Mac setup. Perhaps we could land this now and a test later.

[Alexey Proskuryakov]

I would expect any test that executes this code path to crash under ASan, so my guess is that this code is not covered by any tests. If you have a test that executes this code, I can run it under ASan to confirm that.

[Sergio Villar Senin]

(In reply to comment #6)
> I would expect any test that executes this code path to crash under ASan, so
> my guess is that this code is not covered by any tests. If you have a test
> that executes this code, I can run it under ASan to confirm that.

So basically most of our tests with orthogonal flows go through that code path you could test it with:

fast/css-grid-layout/grid-item-positioning-with-orthogonal-flows.html
fast/css-grid-layout/grid-item-sizing-with-orthogonal-flows.html
fast/css-grid-layout/grid-item-spanning-and-orthogonal-flows.html
fast/css-grid-layout/grid-track-sizing-with-orthogonal-flows.html

[Alexey Proskuryakov]

fast/css-grid-layout/grid-item-positioning-with-orthogonal-flows.html doesn't crash with ASan enabled on Mac. I haven't yet had the chance to check whether the code path is being hit.

[Alexey Proskuryakov]

This code is indeed executed, and looks like ASan doesn't currently detect this error. In trunk clang, that can be enabled separately with -fsanitize-address-use-after-scope.

[Sergio Villar Senin]

(In reply to comment #9)
> This code is indeed executed, and looks like ASan doesn't currently detect
> this error. In trunk clang, that can be enabled separately with
> -fsanitize-address-use-after-scope.

Weird.

In any case, provided the fix is correct I guess we can safely land this.

[Alexey Proskuryakov]

Yes.

[WebKit Commit Bot]

Comment on attachment 288268 [details]
Patch

Clearing flags on attachment: 288268

Committed r205973: <http://trac.webkit.org/changeset/205973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.