Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1059383003:
Make sure observers are not nullptr (Closed)
|
Created:
5 years, 8 months ago by oshima Modified:
5 years, 8 months ago CC:
chromium-reviews, tfarina, tdanderson+views_chromium.org, erikwright+watch_chromium.org, willchan no longer on Chromium Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionMake sure observers are not nullptr
I believe there is no good reason to allow nullptr observer,
and the bug seems to be hitting this. This is to catch such bug early.
BUG=471649
Committed: https://crrev.com/8750dd91b2d1b9e48db1c97ca6cbb059d005991d
Cr-Commit-Position: refs/heads/master@{#324299}
Patch Set 1 : #Patch Set 2 : #
Total comments: 4
Patch Set 3 : #
Total comments: 3
Patch Set 4 : #
Messages
Total messages: 29 (7 generated)
Patchset #1 (id:1) has been deleted
oshima@chromium.org changed reviewers: + danakj@chromium.org
Dana, can you review this? willchan@ may be the best person to review, but he's on leave until last April.
https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h#ne... base/observer_list.h:157: DCHECK(obs); This will most likely lead to crash, so maybe we want CHECK instead? (andi'll revert the change in widget)
Is the telemetry unittest failure this crashing? https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h File base/observer_list.h (right): https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h#ne... base/observer_list.h:157: DCHECK(obs); On 2015/04/03 23:09:28, oshima wrote: > This will most likely lead to crash, so maybe we want CHECK instead? > > (andi'll revert the change in widget) DCHECK is right. You can CHECK with a TODO and bug to put it back to a DCHECK if you want to, to track down a bug. But it should be a DCHECK in the long term. https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h#ne... base/observer_list.h:168: DCHECK(obs); do we care about removing null? we don't seem to care about removing things not in the list. https://codereview.chromium.org/1059383003/diff/40001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1059383003/diff/40001/ui/views/widget/widget.... ui/views/widget/widget.cc:392: CHECK(observer); please add a TODO and point at a bug you're trying to find. then we can remove this once it is resolved.
On 2015/04/06 17:18:47, danakj wrote: > Is the telemetry unittest failure this crashing? 2nd try passed the test. > > https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h > File base/observer_list.h (right): > > https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h#ne... > base/observer_list.h:157: DCHECK(obs); > On 2015/04/03 23:09:28, oshima wrote: > > This will most likely lead to crash, so maybe we want CHECK instead? > > > > (andi'll revert the change in widget) > > DCHECK is right. You can CHECK with a TODO and bug to put it back to a DCHECK if > you want to, to track down a bug. But it should be a DCHECK in the long term. > > https://codereview.chromium.org/1059383003/diff/40001/base/observer_list.h#ne... > base/observer_list.h:168: DCHECK(obs); > do we care about removing null? we don't seem to care about removing things not > in the list. I think this is still useful if someone is removing an already-reset-observer in an ObserverList with check_empty=true. > https://codereview.chromium.org/1059383003/diff/40001/ui/views/widget/widget.cc > File ui/views/widget/widget.cc (right): > > https://codereview.chromium.org/1059383003/diff/40001/ui/views/widget/widget.... > ui/views/widget/widget.cc:392: CHECK(observer); > please add a TODO and point at a bug you're trying to find. then we can remove > this once it is resolved. done.
LGTM
oshima@chromium.org changed reviewers: + sadrul@chormium.org
sadrul -> ui/views OWNERS
On 2015/04/06 21:35:03, oshima wrote: > sadrul -> ui/views OWNERS sadrul: ping?
sadrul@chromium.org changed reviewers: + sadrul@chromium.org
lgtm https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once crbug.com/471649 is resolved. I actually like this check. Maybe just always keep it and remove the TODO?
oshima@chromium.org changed reviewers: - sadrul@chormium.org
https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once crbug.com/471649 is resolved. On 2015/04/08 19:12:33, sadrul wrote: > I actually like this check. Maybe just always keep it and remove the TODO? Sure. I just updated the comment to give more context.
The CQ bit was checked by oshima@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org, sadrul@chromium.org Link to the patchset: https://codereview.chromium.org/1059383003/#ps80001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1059383003/80001
https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.cc File ui/views/widget/widget.cc (right): https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.... ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once crbug.com/471649 is resolved. On 2015/04/08 19:17:10, oshima wrote: > On 2015/04/08 19:12:33, sadrul wrote: > > I actually like this check. Maybe just always keep it and remove the TODO? > > Sure. I just updated the comment to give more context. Why CHECK instead of DCHECK? It adds code/strings to the binary we ship.
On 2015/04/08 20:03:45, danakj wrote: > https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.cc > File ui/views/widget/widget.cc (right): > > https://codereview.chromium.org/1059383003/diff/60001/ui/views/widget/widget.... > ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once > crbug.com/471649 is resolved. > On 2015/04/08 19:17:10, oshima wrote: > > On 2015/04/08 19:12:33, sadrul wrote: > > > I actually like this check. Maybe just always keep it and remove the TODO? > > > > Sure. I just updated the comment to give more context. > > Why CHECK instead of DCHECK? It adds code/strings to the binary we ship. This has to be CHECK now to diagnose the issue. I don't have strong opinion if we should remove this after that. I'll leave this to you guys, so please let me know if you want me to change it.
On Wed, Apr 8, 2015 at 1:14 PM, <oshima@chromium.org> wrote: > On 2015/04/08 20:03:45, danakj wrote: > > https://codereview.chromium.org/1059383003/diff/60001/ui/ > views/widget/widget.cc > >> File ui/views/widget/widget.cc (right): >> > > > https://codereview.chromium.org/1059383003/diff/60001/ui/ > views/widget/widget.cc#newcode392 > >> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once >> crbug.com/471649 is resolved. >> On 2015/04/08 19:17:10, oshima wrote: >> > On 2015/04/08 19:12:33, sadrul wrote: >> > > I actually like this check. Maybe just always keep it and remove the >> TODO? >> > >> > Sure. I just updated the comment to give more context. >> > > Why CHECK instead of DCHECK? It adds code/strings to the binary we ship. >> > > This has to be CHECK now to diagnose the issue. I don't have strong > opinion if > we should remove this after that. > Ya, I'm wondering about after we resolve the bug. I don't think there's a reason to ship CHECKs most of the time. > I'll leave this to you guys, so please let me know if you want me to > change it. > > https://codereview.chromium.org/1059383003/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <danakj@chromium.org> wrote: > On Wed, Apr 8, 2015 at 1:14 PM, <oshima@chromium.org> wrote: > >> On 2015/04/08 20:03:45, danakj wrote: >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >> views/widget/widget.cc >> >>> File ui/views/widget/widget.cc (right): >>> >> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >> views/widget/widget.cc#newcode392 >> >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once >>> crbug.com/471649 is resolved. >>> On 2015/04/08 19:17:10, oshima wrote: >>> > On 2015/04/08 19:12:33, sadrul wrote: >>> > > I actually like this check. Maybe just always keep it and remove the >>> TODO? >>> > >>> > Sure. I just updated the comment to give more context. >>> >> >> Why CHECK instead of DCHECK? It adds code/strings to the binary we ship. >>> >> >> This has to be CHECK now to diagnose the issue. I don't have strong >> opinion if >> we should remove this after that. >> > > Ya, I'm wondering about after we resolve the bug. I don't think there's a > reason to ship CHECKs most of the time. > sadrul, what's your opinion? > > >> I'll leave this to you guys, so please let me know if you want me to >> change it. >> >> https://codereview.chromium.org/1059383003/ >> > > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/08 20:50:57, oshima wrote: > On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <mailto:danakj@chromium.org> wrote: > > > On Wed, Apr 8, 2015 at 1:14 PM, <mailto:oshima@chromium.org> wrote: > > > >> On 2015/04/08 20:03:45, danakj wrote: > >> > >> https://codereview.chromium.org/1059383003/diff/60001/ui/ > >> views/widget/widget.cc > >> > >>> File ui/views/widget/widget.cc (right): > >>> > >> > >> > >> https://codereview.chromium.org/1059383003/diff/60001/ui/ > >> views/widget/widget.cc#newcode392 > >> > >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once > >>> crbug.com/471649 is resolved. > >>> On 2015/04/08 19:17:10, oshima wrote: > >>> > On 2015/04/08 19:12:33, sadrul wrote: > >>> > > I actually like this check. Maybe just always keep it and remove the > >>> TODO? > >>> > > >>> > Sure. I just updated the comment to give more context. > >>> > >> > >> Why CHECK instead of DCHECK? It adds code/strings to the binary we ship. > >>> > >> > >> This has to be CHECK now to diagnose the issue. I don't have strong > >> opinion if > >> we should remove this after that. > >> > > > > Ya, I'm wondering about after we resolve the bug. I don't think there's a > > reason to ship CHECKs most of the time. > > > > sadrul, what's your opinion? I suppose the dchecks in ObserverListBase would be equivalent if we do dcheck here too (and so we don't need this dcheck in widget), but presumably, that wouldn't have caught this particular issue?
On Wed, Apr 8, 2015 at 2:01 PM, <sadrul@chromium.org> wrote: > On 2015/04/08 20:50:57, oshima wrote: > >> On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <mailto:danakj@chromium.org> >> > wrote: > > > On Wed, Apr 8, 2015 at 1:14 PM, <mailto:oshima@chromium.org> wrote: >> > >> >> On 2015/04/08 20:03:45, danakj wrote: >> >> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >> >> views/widget/widget.cc >> >> >> >>> File ui/views/widget/widget.cc (right): >> >>> >> >> >> >> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >> >> views/widget/widget.cc#newcode392 >> >> >> >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once >> >>> crbug.com/471649 is resolved. >> >>> On 2015/04/08 19:17:10, oshima wrote: >> >>> > On 2015/04/08 19:12:33, sadrul wrote: >> >>> > > I actually like this check. Maybe just always keep it and remove >> the >> >>> TODO? >> >>> > >> >>> > Sure. I just updated the comment to give more context. >> >>> >> >> >> >> Why CHECK instead of DCHECK? It adds code/strings to the binary we >> ship. >> >>> >> >> >> >> This has to be CHECK now to diagnose the issue. I don't have strong >> >> opinion if >> >> we should remove this after that. >> >> >> > >> > Ya, I'm wondering about after we resolve the bug. I don't think there's >> a >> > reason to ship CHECKs most of the time. >> > >> > > sadrul, what's your opinion? >> > > I suppose the dchecks in ObserverListBase would be equivalent if we do > dcheck > here too (and so we don't need this dcheck in widget), but presumably, that > wouldn't have caught this particular issue? > My thought is: It would in a debug build/on the bots, which may very well have been enough. If crashes make it past DCHECKs, you figure out the crash, write a unit test, and put it back to a DCHECK state. CHECKs "just in case" lead to binary bloat without a strong value proposition most of the time. If you really like you can keep the CHECK, but I want to push back against it, I think we have a bad practice of using CHECKs when we don't need to in general. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2015/04/08 21:01:28, sadrul wrote: > On 2015/04/08 20:50:57, oshima wrote: > > On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <mailto:danakj@chromium.org> > wrote: > > > > > On Wed, Apr 8, 2015 at 1:14 PM, <mailto:oshima@chromium.org> wrote: > > > > > >> On 2015/04/08 20:03:45, danakj wrote: > > >> > > >> https://codereview.chromium.org/1059383003/diff/60001/ui/ > > >> views/widget/widget.cc > > >> > > >>> File ui/views/widget/widget.cc (right): > > >>> > > >> > > >> > > >> https://codereview.chromium.org/1059383003/diff/60001/ui/ > > >> views/widget/widget.cc#newcode392 > > >> > > >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once > > >>> crbug.com/471649 is resolved. > > >>> On 2015/04/08 19:17:10, oshima wrote: > > >>> > On 2015/04/08 19:12:33, sadrul wrote: > > >>> > > I actually like this check. Maybe just always keep it and remove the > > >>> TODO? > > >>> > > > >>> > Sure. I just updated the comment to give more context. > > >>> > > >> > > >> Why CHECK instead of DCHECK? It adds code/strings to the binary we ship. > > >>> > > >> > > >> This has to be CHECK now to diagnose the issue. I don't have strong > > >> opinion if > > >> we should remove this after that. > > >> > > > > > > Ya, I'm wondering about after we resolve the bug. I don't think there's a > > > reason to ship CHECKs most of the time. > > > > > > > sadrul, what's your opinion? > > I suppose the dchecks in ObserverListBase would be equivalent if we do dcheck > here too (and so we don't need this dcheck in widget), Yes > but presumably, that > wouldn't have caught this particular issue? That is why I added CHECK here. The question is if we want to remove after the issue is resolved. Dana suggested to remove it but you wanted to keep it.
On 2015/04/08 21:05:18, danakj wrote: > On Wed, Apr 8, 2015 at 2:01 PM, <mailto:sadrul@chromium.org> wrote: > > > On 2015/04/08 20:50:57, oshima wrote: > > > >> On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <mailto:danakj@chromium.org> > >> > > wrote: > > > > > On Wed, Apr 8, 2015 at 1:14 PM, <mailto:oshima@chromium.org> wrote: > >> > > >> >> On 2015/04/08 20:03:45, danakj wrote: > >> >> > >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ > >> >> views/widget/widget.cc > >> >> > >> >>> File ui/views/widget/widget.cc (right): > >> >>> > >> >> > >> >> > >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ > >> >> views/widget/widget.cc#newcode392 > >> >> > >> >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK once > >> >>> crbug.com/471649 is resolved. > >> >>> On 2015/04/08 19:17:10, oshima wrote: > >> >>> > On 2015/04/08 19:12:33, sadrul wrote: > >> >>> > > I actually like this check. Maybe just always keep it and remove > >> the > >> >>> TODO? > >> >>> > > >> >>> > Sure. I just updated the comment to give more context. > >> >>> > >> >> > >> >> Why CHECK instead of DCHECK? It adds code/strings to the binary we > >> ship. > >> >>> > >> >> > >> >> This has to be CHECK now to diagnose the issue. I don't have strong > >> >> opinion if > >> >> we should remove this after that. > >> >> > >> > > >> > Ya, I'm wondering about after we resolve the bug. I don't think there's > >> a > >> > reason to ship CHECKs most of the time. > >> > > >> > > > > sadrul, what's your opinion? > >> > > > > I suppose the dchecks in ObserverListBase would be equivalent if we do > > dcheck > > here too (and so we don't need this dcheck in widget), but presumably, that > > wouldn't have caught this particular issue? > > > > My thought is: It would in a debug build/on the bots, which may very well > have been enough. If crashes make it past DCHECKs, you figure out the > crash, write a unit test, and put it back to a DCHECK state. CHECKs "just > in case" lead to binary bloat without a strong value proposition most of > the time. > > If you really like you can keep the CHECK, but I want to push back against > it, I think we have a bad practice of using CHECKs when we don't need to in > general. I agree with all of this. > ... If crashes make it past DCHECKs, you figure out the > crash, ... This is the step that requires the CHECKs, and I want to avoid having to do this add/remove too often. But I suppose we can keep the CHECK iff it does happen often enough. So I am OK with removing the CHECK for now once the mentioned bug is fixed.
On Wed, Apr 8, 2015 at 2:20 PM, <sadrul@chromium.org> wrote: > On 2015/04/08 21:05:18, danakj wrote: > > On Wed, Apr 8, 2015 at 2:01 PM, <mailto:sadrul@chromium.org> wrote: >> > > > On 2015/04/08 20:50:57, oshima wrote: >> > >> >> On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <mailto: >> danakj@chromium.org> >> >> >> > wrote: >> > >> > > On Wed, Apr 8, 2015 at 1:14 PM, <mailto:oshima@chromium.org> wrote: >> >> > >> >> >> On 2015/04/08 20:03:45, danakj wrote: >> >> >> >> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >> >> >> views/widget/widget.cc >> >> >> >> >> >>> File ui/views/widget/widget.cc (right): >> >> >>> >> >> >> >> >> >> >> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >> >> >> views/widget/widget.cc#newcode392 >> >> >> >> >> >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK >> once >> >> >>> crbug.com/471649 is resolved. >> >> >>> On 2015/04/08 19:17:10, oshima wrote: >> >> >>> > On 2015/04/08 19:12:33, sadrul wrote: >> >> >>> > > I actually like this check. Maybe just always keep it and >> remove >> >> the >> >> >>> TODO? >> >> >>> > >> >> >>> > Sure. I just updated the comment to give more context. >> >> >>> >> >> >> >> >> >> Why CHECK instead of DCHECK? It adds code/strings to the binary we >> >> ship. >> >> >>> >> >> >> >> >> >> This has to be CHECK now to diagnose the issue. I don't have strong >> >> >> opinion if >> >> >> we should remove this after that. >> >> >> >> >> > >> >> > Ya, I'm wondering about after we resolve the bug. I don't think >> there's >> >> a >> >> > reason to ship CHECKs most of the time. >> >> > >> >> >> > >> > sadrul, what's your opinion? >> >> >> > >> > I suppose the dchecks in ObserverListBase would be equivalent if we do >> > dcheck >> > here too (and so we don't need this dcheck in widget), but presumably, >> that >> > wouldn't have caught this particular issue? >> > >> > > My thought is: It would in a debug build/on the bots, which may very well >> have been enough. If crashes make it past DCHECKs, you figure out the >> crash, write a unit test, and put it back to a DCHECK state. CHECKs "just >> in case" lead to binary bloat without a strong value proposition most of >> the time. >> > > If you really like you can keep the CHECK, but I want to push back against >> it, I think we have a bad practice of using CHECKs when we don't need to >> in >> general. >> > > I agree with all of this. > > ... If crashes make it past DCHECKs, you figure out the >> crash, ... >> > > This is the step that requires the CHECKs, and I want to avoid having to > do this > add/remove too often. But I suppose we can keep the CHECK iff it does > happen > often enough. So I am OK with removing the CHECK for now once the > mentioned bug > is fixed. > I agree, if it's a super frequent problem we could consider changing it to CHECK in ObserverListBase. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Message was sent while issue was closed.
Committed patchset #4 (id:80001)
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/8750dd91b2d1b9e48db1c97ca6cbb059d005991d Cr-Commit-Position: refs/heads/master@{#324299}
Message was sent while issue was closed.
Okay, I'll remove it once this is fixed. We can revisit when we've got more similar reports after fix. On Wed, Apr 8, 2015 at 2:22 PM, Dana Jansens <danakj@chromium.org> wrote: > On Wed, Apr 8, 2015 at 2:20 PM, <sadrul@chromium.org> wrote: > >> On 2015/04/08 21:05:18, danakj wrote: >> >> On Wed, Apr 8, 2015 at 2:01 PM, <mailto:sadrul@chromium.org> wrote: >>> >> >> > On 2015/04/08 20:50:57, oshima wrote: >>> > >>> >> On Wed, Apr 8, 2015 at 1:26 PM, Dana Jansens <mailto: >>> danakj@chromium.org> >>> >> >>> > wrote: >>> > >>> > > On Wed, Apr 8, 2015 at 1:14 PM, <mailto:oshima@chromium.org> wrote: >>> >> > >>> >> >> On 2015/04/08 20:03:45, danakj wrote: >>> >> >> >>> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >>> >> >> views/widget/widget.cc >>> >> >> >>> >> >>> File ui/views/widget/widget.cc (right): >>> >> >>> >>> >> >> >>> >> >> >>> >> >> https://codereview.chromium.org/1059383003/diff/60001/ui/ >>> >> >> views/widget/widget.cc#newcode392 >>> >> >> >>> >> >>> ui/views/widget/widget.cc:392: // TODO(oshima): Remove this CHECK >>> once >>> >> >>> crbug.com/471649 is resolved. >>> >> >>> On 2015/04/08 19:17:10, oshima wrote: >>> >> >>> > On 2015/04/08 19:12:33, sadrul wrote: >>> >> >>> > > I actually like this check. Maybe just always keep it and >>> remove >>> >> the >>> >> >>> TODO? >>> >> >>> > >>> >> >>> > Sure. I just updated the comment to give more context. >>> >> >>> >>> >> >> >>> >> >> Why CHECK instead of DCHECK? It adds code/strings to the binary we >>> >> ship. >>> >> >>> >>> >> >> >>> >> >> This has to be CHECK now to diagnose the issue. I don't have strong >>> >> >> opinion if >>> >> >> we should remove this after that. >>> >> >> >>> >> > >>> >> > Ya, I'm wondering about after we resolve the bug. I don't think >>> there's >>> >> a >>> >> > reason to ship CHECKs most of the time. >>> >> > >>> >> >>> > >>> > sadrul, what's your opinion? >>> >> >>> > >>> > I suppose the dchecks in ObserverListBase would be equivalent if we do >>> > dcheck >>> > here too (and so we don't need this dcheck in widget), but presumably, >>> that >>> > wouldn't have caught this particular issue? >>> > >>> >> >> My thought is: It would in a debug build/on the bots, which may very well >>> have been enough. If crashes make it past DCHECKs, you figure out the >>> crash, write a unit test, and put it back to a DCHECK state. CHECKs "just >>> in case" lead to binary bloat without a strong value proposition most of >>> the time. >>> >> >> If you really like you can keep the CHECK, but I want to push back >>> against >>> it, I think we have a bad practice of using CHECKs when we don't need to >>> in >>> general. >>> >> >> I agree with all of this. >> >> ... If crashes make it past DCHECKs, you figure out the >>> crash, ... >>> >> >> This is the step that requires the CHECKs, and I want to avoid having to >> do this >> add/remove too often. But I suppose we can keep the CHECK iff it does >> happen >> often enough. So I am OK with removing the CHECK for now once the >> mentioned bug >> is fixed. >> > > I agree, if it's a super frequent problem we could consider changing it to > CHECK in ObserverListBase. > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
