Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748363002:
Fix UBSan testing builds (Closed)
DescriptionFix UBSan testing builds
The linked issue showed a problem with the latest ChromeViewsDelegate and LayoutDelegate changes in
light of how the lower-level vs. higher-level unit tests operate by creating special ViewsDelegate
descendants.
BUG=701415
Review-Url: https://codereview.chromium.org/2748363002
Cr-Commit-Position: refs/heads/master@{#457546}
Committed: https://chromium.googlesource.com/chromium/src/+/d6fe18d8eb8c01d1f420727005653e24dee8a431
Patch Set 1 #
Messages
Total messages: 22 (10 generated)
The CQ bit was checked by kylixrd@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...
Description was changed from ========== Fix UBSan testing builds The linked issue showed a problem with the latest ChromeViewsDelegate and LayoutDelegate changes in light of how the lower-level vs. higher-level unit tests operate by creating special ViewsDelegate descendants. BUG=701415 ========== to ========== Fix UBSan testing builds The linked issue showed a problem with the latest ChromeViewsDelegate and LayoutDelegate changes in light of how the lower-level vs. higher-level unit tests operate by creating special ViewsDelegate descendants. BUG=701415 ==========
kylixrd@chromium.org changed reviewers: + pkasting@chromium.org
This CL is an attempt to fix the problem uncovered by UBSan in the referenced issue. The patch does eliminate the specific issue, however another following failure is uncovered which I doubt is related.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
krasin@chromium.org changed reviewers: + krasin@chromium.org
lgtm
This doesn't seem right. ViewsDelegate must never be null. If it's null somewhere, the test should be creating it. We shouldn't work around that be adding a null-check here. Where is the above line of reasoning going wrong?
On 2017/03/15 21:56:43, Peter Kasting wrote: > This doesn't seem right. ViewsDelegate must never be null. If it's null > somewhere, the test should be creating it. We shouldn't work around that be > adding a null-check here. > > Where is the above line of reasoning going wrong? The problem is that ToolbarActionsBarBubbleViewsTest descends from views::TestViewsBase which uses a special test-only direct descendant of ViewsDelegate, TestViewsDelegate and explicitly instantiates it. The way it is written would not allow another descendant of ViewsDelegate to supplant it. views::TestViewsBase is down in ui/views/test, so it cannot references ChromeViewsDelegate up in chrome/browser/ui/views. If an instance of ChromeViewsDelegate is also created, the constructor of ViewsDelegate will fail because it's global singleton reference is not nullptr. So the idea here is that instead of calling GetDefaultDistanceMetric on the instance, convert it to a static function and then check if the private views_delegate_ global reference is nullptr and only then call via that instance, otherwise explicitly call views::ViewsDelegate::GetInstance()->GetDistanceMetric(); Other than reworking the tests or rethink the whole ViewsDelegate/ChromeViewsDelegate thing, I figured this was a middle compromise. I suspect there will be more fallout like this as we split out LayoutDelegate and push it down into ui/views.
On 2017/03/15 22:26:20, kylix_rd wrote: > On 2017/03/15 21:56:43, Peter Kasting wrote: > > This doesn't seem right. ViewsDelegate must never be null. If it's null > > somewhere, the test should be creating it. We shouldn't work around that be > > adding a null-check here. > > > > Where is the above line of reasoning going wrong? > > The problem is that ToolbarActionsBarBubbleViewsTest descends from > views::TestViewsBase which uses a special test-only direct descendant of > ViewsDelegate, TestViewsDelegate and explicitly instantiates it. The way it is > written would not allow another descendant of ViewsDelegate to supplant it. > views::TestViewsBase is down in ui/views/test, so it cannot references > ChromeViewsDelegate up in chrome/browser/ui/views. If an instance of > ChromeViewsDelegate is also created, the constructor of ViewsDelegate will fail > because it's global singleton reference is not nullptr. So, in short, TestViewsBase explicitly depends on a ViewsDelegate implementation that won't work for chrome-side code, and this chrome-side test is descended from TestViewsBase. Is that a correct summary? This suggests that the fix is something like these: * Pull most of TestViewsBase into a base class that would also work for chrome/ code. Have some pure-abstract method to create the appropriate ViewsDelegate instance. Provide concrete subclasses for views and chrome tests. Then have tests extend the appropriate one. * Or, inject the appropriate delegate, by having TestViewsBase take a pointer to the delegate or a pointer to the delegate getter function. Then tests pass the appropriate thing for their type. Would these mechanisms work? I just got done telling other people to rip out all the null checks and ensure the code assumes this is never null, so I really don't want to add one back.
On 2017/03/16 00:27:02, Peter Kasting wrote: > On 2017/03/15 22:26:20, kylix_rd wrote: > > On 2017/03/15 21:56:43, Peter Kasting wrote: > > > This doesn't seem right. ViewsDelegate must never be null. If it's null > > > somewhere, the test should be creating it. We shouldn't work around that be > > > adding a null-check here. > > > > > > Where is the above line of reasoning going wrong? > > > > The problem is that ToolbarActionsBarBubbleViewsTest descends from > > views::TestViewsBase which uses a special test-only direct descendant of > > ViewsDelegate, TestViewsDelegate and explicitly instantiates it. The way it is > > written would not allow another descendant of ViewsDelegate to supplant it. > > views::TestViewsBase is down in ui/views/test, so it cannot references > > ChromeViewsDelegate up in chrome/browser/ui/views. If an instance of > > ChromeViewsDelegate is also created, the constructor of ViewsDelegate will > fail > > because it's global singleton reference is not nullptr. > > So, in short, TestViewsBase explicitly depends on a ViewsDelegate implementation > that won't work for chrome-side code, and this chrome-side test is descended > from TestViewsBase. Is that a correct summary? > > This suggests that the fix is something like these: > * Pull most of TestViewsBase into a base class that would also work for chrome/ > code. Have some pure-abstract method to create the appropriate ViewsDelegate > instance. Provide concrete subclasses for views and chrome tests. Then have > tests extend the appropriate one. > * Or, inject the appropriate delegate, by having TestViewsBase take a pointer to > the delegate or a pointer to the delegate getter function. Then tests pass the > appropriate thing for their type. > > Would these mechanisms work? That would be difficult to pull off. ChromeViewsDelegate introduces the GetDefaultDistanceMetric() function and is explicitly invoked from LayoutDelegate::GetMetric() via that specific type. I could push GetDefaultDistanceMetric() back up to ViewsDelegate, but sky@ had issues with that approach. > I just got done telling other people to rip out all the null checks and ensure > the code assumes this is never null, so I really don't want to add one back. In this case, the null check is there to decide whether to call the InternalGetDefaultInstanceMetric() function to directly call GetDistanceMetric via ViewsDelegate::GetInstance(). In either case, the intent is to invoke the code in the ViewsDelegate::GetDistanceMetric() and not the override in ChromeViewsDelegate. Yes, it's a null check, but not one that bypasses the call altogether. The eventual "fix" would likely come from pushing LayoutDelegate up into ui/views and breaking it out of ViewsDelegate. At that point, a future ChromeLayoutDelegate could then be instantiated independently from the TestViewsDelegate. This CL is really just a stop-gap measure until we can achieve that goal. The bottom line is that ViewsDelegate is now covering too many independent concepts which makes it cumbersome to mock and test.
On 2017/03/16 13:39:35, kylix_rd wrote: > The eventual "fix" would likely come from pushing LayoutDelegate up into > ui/views and breaking it out of ViewsDelegate. Can we just do that right now? I think I'm unclear on what the blockers are to doing this immediately, all at once, in a single CL. It seems like you view this as a major refactoring that will take notable time and effort and several iterative CLs along the way. This is also why I had wanted your initial patch to just do this directly, but accepted the alternate route because I read you as saying "I need to do this smaller piece as a step towards that thing". So, how long is this stopgap intended to be in place? A day? A week? Six months? If it's longer than "a week", then I think I need to find someone to jump in and help out here so we can get the time down, or else I've really misunderstood what it takes to accomplish this.
On 2017/03/16 19:27:24, Peter Kasting wrote: > On 2017/03/16 13:39:35, kylix_rd wrote: > > The eventual "fix" would likely come from pushing LayoutDelegate up into > > ui/views and breaking it out of ViewsDelegate. > > Can we just do that right now? > > I think I'm unclear on what the blockers are to doing this immediately, all at > once, in a single CL. It seems like you view this as a major refactoring that > will take notable time and effort and several iterative CLs along the way. This > is also why I had wanted your initial patch to just do this directly, but > accepted the alternate route because I read you as saying "I need to do this > smaller piece as a step towards that thing". > > So, how long is this stopgap intended to be in place? A day? A week? Six > months? > > If it's longer than "a week", then I think I need to find someone to jump in and > help out here so we can get the time down, or else I've really misunderstood > what it takes to accomplish this. I've started that work. It should be ready for review early next week. In the meantime, this CL will help get the UBSan bots back to health.
> I've started that work. It should be ready for review early next week. In the > meantime, this CL will help get the UBSan bots back to health. Yes, that will be very handy, as there's another, currently shadowed bug sitting in Chromium code, and might be even more of them since the bots have been red for a while now.
Cool, I am 100% fine with this as a short-term bandaid then. LGTM
The CQ bit was checked by kylixrd@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1489694481935910, "parent_rev":
"6472567a4740b44a54e1a27692d7c31c94a47bb3", "commit_rev":
"d6fe18d8eb8c01d1f420727005653e24dee8a431"}
Message was sent while issue was closed.
Description was changed from ========== Fix UBSan testing builds The linked issue showed a problem with the latest ChromeViewsDelegate and LayoutDelegate changes in light of how the lower-level vs. higher-level unit tests operate by creating special ViewsDelegate descendants. BUG=701415 ========== to ========== Fix UBSan testing builds The linked issue showed a problem with the latest ChromeViewsDelegate and LayoutDelegate changes in light of how the lower-level vs. higher-level unit tests operate by creating special ViewsDelegate descendants. BUG=701415 Review-Url: https://codereview.chromium.org/2748363002 Cr-Commit-Position: refs/heads/master@{#457546} Committed: https://chromium.googlesource.com/chromium/src/+/d6fe18d8eb8c01d1f42072700565... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/d6fe18d8eb8c01d1f42072700565... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
