161739 2016-09-08 08:13:05 -0700 [css-grid] Fix a dangling reference 2016-09-15 09:34:27 -0700 1 1 1 Unclassified WebKit New Bugs WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 svillar svillar ap commit-queue darin esprehn+autocc glenn jfernandez kondapallykalyan rego svillar oldest_to_newest 1227598 0 svillar 2016-09-08 08:13:05 -0700 [css-grid] Fix a dangling reference 1227599 1 288268 svillar 2016-09-08 08:16:12 -0700 Created attachment 288268 Patch 1227600 2 288268 rego 2016-09-08 08:26:17 -0700 Comment on attachment 288268 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". 1228006 3 288268 ap 2016-09-08 19:37:42 -0700 Comment on attachment 288268 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. 1228007 4 288268 ap 2016-09-08 19:39:35 -0700 Comment on attachment 288268 Patch Looking again, I misread the code. This does have a problem. Looks like we need a test that would catch it under ASan. 1229154 5 svillar 2016-09-13 06:34:16 -0700 (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. 1229183 6 ap 2016-09-13 09:46:46 -0700 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. 1229487 7 svillar 2016-09-14 01:32:20 -0700 (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 1229836 8 ap 2016-09-14 17:10:36 -0700 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. 1229907 9 ap 2016-09-14 22:40:04 -0700 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. 1229925 10 svillar 2016-09-15 00:38:44 -0700 (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. 1230021 11 ap 2016-09-15 09:06:53 -0700 Yes. 1230027 12 288268 commit-queue 2016-09-15 09:34:23 -0700 Comment on attachment 288268 Patch Clearing flags on attachment: 288268 Committed r205973: <http://trac.webkit.org/changeset/205973> 1230028 13 commit-queue 2016-09-15 09:34:27 -0700 All reviewed patches have been landed. Closing bug. 288268 2016-09-08 08:16:12 -0700 2016-09-15 09:34:23 -0700 Patch bug-161739-20160908171247.patch text/plain 1758 svillar U3VidmVyc2lvbiBSZXZpc2lvbjogMjA1MTE2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMDZkMjQ5NTM2MzA2NmQy NGRiZmJmNWZmZjBiOWQzOWM4ZmQ1OWJhNi4uNzkxZWY1YTUyOWM1MjdlZTA2ZDEwMTI5YzUwNTRh ZGFjZjdmMDA5MiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDE2IEBACisyMDE2LTA5LTA4ICBTZXJn aW8gVmlsbGFyIFNlbmluICA8c3ZpbGxhckBpZ2FsaWEuY29tPgorCisgICAgICAgIFtjc3MtZ3Jp ZF0gRml4IGEgZGFuZ2xpbmcgcmVmZXJlbmNlCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQu b3JnL3Nob3dfYnVnLmNnaT9pZD0xNjE3MzkKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkg KE9PUFMhKS4KKworICAgICAgICBUaGUgY29kZSB3YXMgdHJ5aW5nIHRvIGdldCBhIHJlZmVyZW5j ZSB0byBhIHByaXZhdGUgYXR0cmlidXRlIG9mIGEgdGVtcG9yYXJ5IG9iamVjdCByZXR1cm5lZAor ICAgICAgICBieSBncmlkVHJhY2tTaXplKCkuCisKKyAgICAgICAgKiByZW5kZXJpbmcvUmVuZGVy R3JpZC5jcHA6CisgICAgICAgIChXZWJDb3JlOjpSZW5kZXJHcmlkOjphc3N1bWVkUm93c1NpemVG b3JPcnRob2dvbmFsQ2hpbGQpOgorCiAyMDE2LTA4LTE2ICBDYXJsb3MgR2FyY2lhIENhbXBvcyAg PGNnYXJjaWFAaWdhbGlhLmNvbT4KIAogICAgICAgICBbR1RLXSBBY2NlbGVyYXRlZCBjb21wb3Np dGluZyBkb2VzIG5vdCB3b3JrIGluIFdheWxhbmQKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3Jl L3JlbmRlcmluZy9SZW5kZXJHcmlkLmNwcCBiL1NvdXJjZS9XZWJDb3JlL3JlbmRlcmluZy9SZW5k ZXJHcmlkLmNwcAppbmRleCA1ZjA3OGJjODJlMTE5Zjc0NDlkN2Y4ZTcxZTJlZDYyOGU0ZGJmZGI5 Li4zMWQ0MzQ2YTA0NmIxMDBmMmI2NmEwNjRkZDJjNzA5ZTdhZDMyYzZjIDEwMDY0NAotLS0gYS9T b3VyY2UvV2ViQ29yZS9yZW5kZXJpbmcvUmVuZGVyR3JpZC5jcHAKKysrIGIvU291cmNlL1dlYkNv cmUvcmVuZGVyaW5nL1JlbmRlckdyaWQuY3BwCkBAIC0yMDA2LDcgKzIwMDYsNyBAQCBMYXlvdXRV bml0IFJlbmRlckdyaWQ6OmFzc3VtZWRSb3dzU2l6ZUZvck9ydGhvZ29uYWxDaGlsZChjb25zdCBS ZW5kZXJCb3gmIGNoaWxkLAogICAgIGJvb2wgZ3JpZEFyZWFJc0luZGVmaW5pdGUgPSBmYWxzZTsK ICAgICBMYXlvdXRVbml0IGNvbnRhaW5pbmdCbG9ja0F2YWlsYWJsZVNpemUgPSBjb250YWluaW5n QmxvY2tMb2dpY2FsSGVpZ2h0Rm9yQ29udGVudChFeGNsdWRlTWFyZ2luQm9yZGVyUGFkZGluZyk7 CiAgICAgZm9yIChhdXRvIHRyYWNrUG9zaXRpb24gOiBzcGFuKSB7Ci0gICAgICAgIGNvbnN0IEdy aWRMZW5ndGgmIG1heFRyYWNrU2l6ZSA9IGdyaWRUcmFja1NpemUoRm9yUm93cywgdHJhY2tQb3Np dGlvbiwgc2l6aW5nT3BlcmF0aW9uKS5tYXhUcmFja0JyZWFkdGgoKTsKKyAgICAgICAgR3JpZExl bmd0aCBtYXhUcmFja1NpemUgPSBncmlkVHJhY2tTaXplKEZvclJvd3MsIHRyYWNrUG9zaXRpb24s IHNpemluZ09wZXJhdGlvbikubWF4VHJhY2tCcmVhZHRoKCk7CiAgICAgICAgIGlmIChtYXhUcmFj a1NpemUuaXNDb250ZW50U2l6ZWQoKSB8fCBtYXhUcmFja1NpemUuaXNGbGV4KCkpCiAgICAgICAg ICAgICBncmlkQXJlYUlzSW5kZWZpbml0ZSA9IHRydWU7CiAgICAgICAgIGVsc2UK