Pull back some CHECK calls which were removed.

Pull back some CHECK calls which were removed.

Original CL http://codereview.chromium.org/8368018/ was sort of
aggressive in some areas.  I reveiewed it and this pulls back the
cases I'm pretty sure should be kept.  My basic approach was to keep
things which would probably never happen in development, and which
signal a problem which is unlikely to be transient.

 - important_file_writer.cc getting impossible amounts of data.
 - cfbundle_blocker.mm logs blocked bundles so user can see.
 - objc_zombie.mm crashes if the objc runtime is unexpectedly bizarre.
 - multi-process lock logs failure.

I've actually seen that last before when a test nested a profile too
deeply, in which case knowing the filename was the key hint.


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=109080

[Scott Hess - ex-Googler, 2011-11-04 23:23:48]

Sorry so late, sheriff'ing and sick pushed it up my stack.

[Mark Mentovai, 2011-11-05 00:49:02]

I agree with all of your changes except for the ones I’ve called out as maybe OK
to be D still.

http://codereview.chromium.org/8477018/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/8477018/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:53: // If happens in the wild something
really bad is likely to be going on.
If happens -> if this happens?

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.mm
File chrome/common/mac/objc_zombie.mm (right):

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.m...
chrome/common/mac/objc_zombie.mm:453: CHECK([NSThread isMainThread]);
Can this one stay D?

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.m...
chrome/common/mac/objc_zombie.mm:530: CHECK([NSThread isMainThread]);
This too?

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.m...
chrome/common/mac/objc_zombie.mm:539: CHECK(m);
This too? If m is 0, the next line will either crash or harmlessly do nothing.
The D might be OK here.

[Mark Mentovai, 2011-11-05 00:49:28]

(It’s still your call, but you might have to defend your decisions I’ve called
out a bit more if you feel strongly.)

[Scott Hess - ex-Googler, 2011-11-08 18:47:51]

The two on-main-thread checks I don't feel super strongly about.  Mostly my
concern was if someone accidentally wires things up so they could be called on a
thread (like if some global on-first-use initializer somewhere which is
thread-safe calls into this).  That's probably pretty unlikely, though, and the
resulting race condition is pretty unlikely to be hit, and if it is hit, there's
a good chance that it would work anyhow.

The check that -dealloc exists I'm a little more nervous about.  I _think_
nothing will break because of this, the replacement -dealloc would continue to
work, it just wouldn't leave anything on the treadmill, I think.  But the code
itself indicates that the replacement -dealloc isn't there, so WTF?  The world's
gone topsy-turvy!  All of which means that the barn door has been open and the
horse is long gone anyhow.

But, really, production code isn't calling ZombieDisable(), so I can't see it
ever firing outside of a testing framework, so it won't be able to act as a
canary.  Then again, it's probably stripped away so removing that CHECK won't
help anything.

http://codereview.chromium.org/8477018/diff/1/chrome/common/important_file_wr...
File chrome/common/important_file_writer.cc (right):

http://codereview.chromium.org/8477018/diff/1/chrome/common/important_file_wr...
chrome/common/important_file_writer.cc:53: // If happens in the wild something
really bad is likely to be going on.
On 2011/11/05 00:49:02, Mark Mentovai wrote:
> If happens -> if this happens?

Done.

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.mm
File chrome/common/mac/objc_zombie.mm (right):

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.m...
chrome/common/mac/objc_zombie.mm:453: CHECK([NSThread isMainThread]);
On 2011/11/05 00:49:02, Mark Mentovai wrote:
> Can this one stay D?

Done.

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.m...
chrome/common/mac/objc_zombie.mm:530: CHECK([NSThread isMainThread]);
On 2011/11/05 00:49:02, Mark Mentovai wrote:
> This too?

Done.

http://codereview.chromium.org/8477018/diff/1/chrome/common/mac/objc_zombie.m...
chrome/common/mac/objc_zombie.mm:539: CHECK(m);
On 2011/11/05 00:49:02, Mark Mentovai wrote:
> This too? If m is 0, the next line will either crash or harmlessly do nothing.
> The D might be OK here.

Done.

[Mark Mentovai, 2011-11-08 18:49:24]

You haven’t uploaded a new change set but I suspect it’s all LGTM.

[Scott Hess - ex-Googler, 2011-11-08 18:54:35]

On 2011/11/08 18:49:24, Mark Mentovai wrote:
> You haven’t uploaded a new change set but I suspect it’s all LGTM.

Oops, right you are.

Ironically, objc_zombie.mm was what made me look at the original CL :-).

[Mark Mentovai, 2011-11-08 18:58:52]

L
 G
  T
   M

[commit-bot: I haz the power, 2011-11-08 18:59:53]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/shess@chromium.org/8477018/7002

[commit-bot: I haz the power, 2011-11-08 20:04:49]

Change committed as 109080