base: add a thread-safety assertion checker

base: add a thread-safety assertion checker

ThreadAssertions lets us assert that the current thread is allowed to
make blocking calls.  See the header for more background.

This change implements ThreadAssertions but doesn't turn it on for
any of Chrome.

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

[Aaron Boodman, 2010-10-18 20:22:11]

cool.

http://codereview.chromium.org/3824006/diff/1/4
File base/strict_mode.h (right):

http://codereview.chromium.org/3824006/diff/1/4#newcode5
base/strict_mode.h:5: #ifndef BASE_STRICT_MODE_H_
Is "strict mode" a term of art that I'm unaware of? Otherwise, it seems like a
name that is too generic for the functionality it contains. Maybe
"file_thread_restrictions.h" or something?

http://codereview.chromium.org/3824006/diff/1/4#newcode14
base/strict_mode.h:14: //      base::StrictMode::SetIOAllowed(true);
Maybe SetIOAllowedOnCurrentThread() for clarity?

[Evan Martin, 2010-10-18 20:40:50]

Having tried this out locally, I found we have extra threads in e.g.
base_unittests that do want to access files.  I'm considering switching it to a
blacklist -- like the UI and IO threads could mark themselves as "disallow IO",
and threads by default could make IO calls.

[jam, 2010-10-18 23:33:15]

I agree a blacklist would be better.  That way when someone adds a new
background thread, they don't have a "process" to follow.

http://codereview.chromium.org/3824006/diff/10001/11003
File base/thread_restrictions.cc (right):

http://codereview.chromium.org/3824006/diff/10001/11003#newcode35
base/thread_restrictions.cc:35: << "by calling
base::ThreadRestrictions::SetIOAllowed(true) at startup.";
nit: 80

[jam, 2010-10-18 23:34:01]

also, can we check this in with asserts in file_util and
PluginList::LoadPlugins?  otherwise it'll be a cool class but no one will be
motivated to track down the asserts ;)

[Evan Martin, 2010-10-18 23:35:12]

On 2010/10/18 23:34:01, John Abd-El-Malek wrote:
> also, can we check this in with asserts in file_util and
> PluginList::LoadPlugins?  otherwise it'll be a cool class but no one will be
> motivated to track down the asserts ;)

Yeah, once we're happy with the API I intend to do followup changes.  I have the
file_util changes locally (which is why I learned that whitelisting is probably
bad)

[tfarina, 2010-10-19 02:42:38]

http://codereview.chromium.org/3824006/diff/10001/11004
File base/thread_restrictions.h (right):

http://codereview.chromium.org/3824006/diff/10001/11004#newcode20
base/thread_restrictions.h:20: //     
base::ThreadRestrictions::AssertIsIOAllowed();
typo: AssertIOAllowed

To match with the method below.

[Evan Martin, 2010-10-19 18:03:31]

Thanks Thiago!

I think this is ready to go in, unless Darin has any comments...

[Evan Martin, 2010-10-19 18:03:44]

Thanks Thiago!

I think this is ready to go in, unless Darin has any comments...

[jam, 2010-10-19 20:27:17]

lgtm

[darin (slow to review), 2010-10-22 06:30:44]

http://codereview.chromium.org/3824006/diff/17001/18004
File base/thread_restrictions.h (right):

http://codereview.chromium.org/3824006/diff/17001/18004#newcode28
base/thread_restrictions.h:28: static void SetIOAllowed(bool allowed);
I think it is confusing to use the term "IO" here like this.  Asynchronous IO is
fine.  The problem is blocking IO.

SetBlockingIOAllowed and AssertBlockingIOAllowed would have been better.

http://codereview.chromium.org/3824006/diff/17001/18004#newcode35
base/thread_restrictions.h:35: ThreadRestrictions();  // class for namespacing
only
DISALLOW_IMPLICIT_CONSTRUCTORS?