Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2381633002:
Fix uninit read in PlatformNotificationServiceTest (Closed)
DescriptionFix uninit read in PlatformNotificationServiceTest.
Do some cleanup as well.
BUG=651165
Committed: https://crrev.com/b4a1e893bd1444ebb84ac4944a23a01bb379dd64
Cr-Commit-Position: refs/heads/master@{#421625}
Patch Set 1 #Patch Set 2 : Fix uninit value, more cleanup. #
Total comments: 2
Messages
Total messages: 20 (9 generated)
The CQ bit was checked by thestig@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
thestig@chromium.org changed reviewers: + awdf@chromium.org, dewittj@chromium.org
lgtm, thanks for cleaning up!
though the uninit read is actually in the test, right? So perhaps change the subject to that effect.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Fix uninit read in PlatformNotificationServiceImpl. Do some cleanup as well. BUG=651165 ========== to ========== Fix uninit read in PlatformNotificationServiceTest. Do some cleanup as well. BUG=651165 ==========
On 2016/09/28 19:34:28, dewittj wrote: > though the uninit read is actually in the test, right? So perhaps change the > subject to that effect. Done.
The CQ bit was checked by thestig@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Fix uninit read in PlatformNotificationServiceTest. Do some cleanup as well. BUG=651165 ========== to ========== Fix uninit read in PlatformNotificationServiceTest. Do some cleanup as well. BUG=651165 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Fix uninit read in PlatformNotificationServiceTest. Do some cleanup as well. BUG=651165 ========== to ========== Fix uninit read in PlatformNotificationServiceTest. Do some cleanup as well. BUG=651165 Committed: https://crrev.com/b4a1e893bd1444ebb84ac4944a23a01bb379dd64 Cr-Commit-Position: refs/heads/master@{#421625} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/b4a1e893bd1444ebb84ac4944a23a01bb379dd64 Cr-Commit-Position: refs/heads/master@{#421625}
Message was sent while issue was closed.
https://codereview.chromium.org/2381633002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/2381633002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_unittest.cc:259: notification_data.actions[0].type = Am I correct in surmising that it was just the lack of these 4 lines caused the warning you reported, the rest is just cleanup? Thank you for fixing this. Sorry I didn't notice it myself - obviously I should have looked for, run and fixed up the unit tests (if necessary) for every .cc file changed - I will remember to do this in future! Out of interest, would the only way to have seen that warning been to run that particular test locally? It's not visible on trybot results is it? (They were all green ... except for android_n5x_swarming_rel - what's that about?)
Message was sent while issue was closed.
https://codereview.chromium.org/2381633002/diff/20001/chrome/browser/notifica... File chrome/browser/notifications/platform_notification_service_unittest.cc (right): https://codereview.chromium.org/2381633002/diff/20001/chrome/browser/notifica... chrome/browser/notifications/platform_notification_service_unittest.cc:259: notification_data.actions[0].type = On 2016/09/29 10:47:57, awdf wrote: > Am I correct in surmising that it was just the lack of these 4 lines caused the > warning you reported, the rest is just cleanup? > > Thank you for fixing this. Sorry I didn't notice it myself - obviously I should > have looked for, run and fixed up the unit tests (if necessary) for every .cc > file changed - I will remember to do this in future! > > Out of interest, would the only way to have seen that warning been to run that > particular test locally? It's not visible on trybot results is it? (They were > all green ... except for android_n5x_swarming_rel - what's that about?) > Peter's since explained that the test which gave the warning only runs post-submit, and I couldn't have seen it by running the test locally. How did you notice it then? - I would have expected an email from a post-submit step saying my change could have been responsible, like for performance tests, but didn't get one.. (Peter also suggested a more fundamental fix of making sure PlatformNotificationAction's PlatformNotificationActionType has a default value so is always initialized - I've now done that at https://codereview.chromium.org/2381023002, you should be cc'd)
Message was sent while issue was closed.
On 2016/09/29 10:47:57, awdf wrote: > Out of interest, would the only way to have seen that warning been to run that > particular test locally? It's not visible on trybot results is it? (They were > all green ... except for android_n5x_swarming_rel - what's that about?) There's no trybots that run MSAN by default. (yet) On the memory.full waterfall, there are MSAN bots and they found this. I was sheriff yesterday. See the bug for details.
Message was sent while issue was closed.
On 2016/09/29 15:31:21, Lei Zhang wrote: > On 2016/09/29 10:47:57, awdf wrote: > > Out of interest, would the only way to have seen that warning been to run that > > particular test locally? It's not visible on trybot results is it? (They were > > all green ... except for android_n5x_swarming_rel - what's that about?) > > There's no trybots that run MSAN by default. (yet) On the memory.full waterfall, > there are MSAN bots and they found this. I was sheriff yesterday. See the bug > for details. Thanks, it all makes sense now :) |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
