ContentBrowserSanityChecker

Introduce ContentBrowserSanityChecker, which currently just adds checks to
validate the sanity of the WebContentsObserver invocations, but which hopefully
will grow to include other too-heavy-for-production-code bug detection logic.

The currently supported check, WebContentsObserverSanityChecker, is a fleshed-
out version of the FrameLifetimeConsistencyChecker previously used in
RenderFrameHost unittests.

This sanity check managed to identify the root cause for bug 438951, so it's
already proved itself useful. Enabling it on all tests should prevent future
regressions of that kind.

To install this on all WebContentses, we need to un-deprecate the
WebContentsImpl::AddCreatedCallback/RemoveCreatedCallback mechanism. To make
that safer, the dangerous methods are moved to the FriendZone (get it?) which is
what I call what Dr. Dobbs calls, stodgily, the "C++ attorney/client idiom"
http://goo.gl/13oS7e. They also now have, cleverly, ForTesting in their names.

Lastly, we change the invocation time of the g_created_callbacks to be at the
end of WebContents::Init, instead of at the end of the ctor, so we're not
inviting tests to muck around with a half-inflated WebContents.

BUG=438951, 444722, 444717
TBR=jochen@chromium.org

Committed: https://crrev.com/f9acfbeeed26e7cd71b4fd0d7fa1754dd80140d5
Cr-Commit-Position: refs/heads/master@{#309571}

[Avi (use Gerrit), 2014-12-19 19:29:53]

avi@chromium.org changed reviewers:
+ avi@chromium.org

[Avi (use Gerrit), 2014-12-19 19:29:53]

I personally think this is a great idea.

My recommendations:
- Land the fix as a separate patch.
- Turn this CL into the addition of the consistency checker plus cleanup of the
existing code.
- Cleanup that I'm thinking of:
-- Rename Add/RemoveCreatedCallback to have ForTesting, so that it triggers on
non-test code using it.
-- Explicitly comment on the friending of the test classes as to why they're
being friended.
-- Remove the comment about those calls being deprecated.

John, I've run into issues where observers exposed from content have
inconsistent calls made on them (remember RenderProcessHostObserver?) and we
need more code that catches mistakes inside of content while it's being
exercised by all the weird code out there in tests.

Since you marked the creation observer calls as "deprecated", I'm presuming you
had a good plan for achieving this kind of testing. But this is the only way _I_
see now how to do it, so I'm willing to give this my approval.

[ncarter (slow), 2014-12-22 19:32:56]

Avi, please have a look. I think I've implemented all your suggestions.

[Avi (use Gerrit), 2014-12-22 22:53:50]

Whoa. I really like this. LGTM

https://codereview.chromium.org/803873004/diff/100001/content/public/test/con...
File content/public/test/content_browser_sanity_checker.cc (right):

https://codereview.chromium.org/803873004/diff/100001/content/public/test/con...
content/public/test/content_browser_sanity_checker.cc:19: << "There's no point
in enabling this twice.";
Don't use "this" here; make the error message self-explanatory.

[ncarter (slow), 2014-12-23 00:15:25]

nick@chromium.org changed reviewers:
+ jochen@chromium.org

[ncarter (slow), 2014-12-23 00:17:04]

I've fixed Avi's suggestion, disabled a test in web_navigation_apitest, and
added links to two bugs about the tests disabled by this CL.

TBR'ing jochen for web_navigation_apitest.cc

https://codereview.chromium.org/803873004/diff/100001/content/public/test/con...
File content/public/test/content_browser_sanity_checker.cc (right):

https://codereview.chromium.org/803873004/diff/100001/content/public/test/con...
content/public/test/content_browser_sanity_checker.cc:19: << "There's no point
in enabling this twice.";
On 2014/12/22 22:53:50, Avi wrote:
> Don't use "this" here; make the error message self-explanatory.

Done.

[ncarter (slow), 2014-12-23 00:17:11]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-12-23 00:18:03]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/803873004/140001

[ncarter (slow), 2014-12-23 00:27:47]

The CQ bit was unchecked by nick@chromium.org

[ncarter (slow), 2014-12-23 00:28:44]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-12-23 00:29:06]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/803873004/160001

[commit-bot: I haz the power, 2014-12-23 02:09:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-23 02:09:41]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[ncarter (slow), 2014-12-23 06:51:54]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-12-23 06:52:37]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/803873004/160001

[commit-bot: I haz the power, 2014-12-23 08:34:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-12-23 08:35:00]

Try jobs failed on following builders:
  android_dbg_tests_recipe on tryserver.chromium.linux
(http://build.chromium.org/p/tryserver.chromium.linux/builders/android_dbg_tes...)

[jochen (gone - plz use gerrit), 2014-12-23 14:01:42]

btw, as I stated on the bug, I think the WebNavigationTest catches a real
failure. disabling it for now is ok, but please make sure the underlying bug
gets fixed.

[ncarter (slow), 2014-12-23 19:10:31]

The CQ bit was checked by nick@chromium.org

[commit-bot: I haz the power, 2014-12-23 19:11:50]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/803873004/160001

[commit-bot: I haz the power, 2014-12-23 19:13:07]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2014-12-23 19:14:18]

Patchset 9 (id:??) landed as
https://crrev.com/f9acfbeeed26e7cd71b4fd0d7fa1754dd80140d5
Cr-Commit-Position: refs/heads/master@{#309571}

[jam, 2015-05-11 14:44:24]

I just saw these test files. They're only used by content, so they should be in
content/test instead of content/public/test

[ncarter (slow), 2015-05-13 17:39:48]

On 2015/05/11 14:44:24, jam wrote:
> I just saw these test files. They're only used by content, so they should be
in
> content/test instead of content/public/test

Thanks John and good catch. 

fwiw -- I put them in public, expecting to need to instantiate and/or configure
them from chrome browsertests. But instantiation for chrome browser_tests is
done for free by hooking BrowserTestBase, and the need for configuration (e.g.,
to disable an invariant check for the purposes of testing) hasn't materialized
yet.

I'll send you a CL. If we do need to expose them for configuration by chrome
tests, we can expose narrow methods for that purpose alone.

[jam, 2015-05-13 17:47:18]

great, thanks

On Wed, May 13, 2015 at 10:39 AM, <nick@chromium.org> wrote:

> On 2015/05/11 14:44:24, jam wrote:
>
>> I just saw these test files. They're only used by content, so they should
>> be
>>
> in
>
>> content/test instead of content/public/test
>>
>
> Thanks John and good catch.
>
> fwiw -- I put them in public, expecting to need to instantiate and/or
> configure
> them from chrome browsertests. But instantiation for chrome browser_tests
> is
> done for free by hooking BrowserTestBase, and the need for configuration
> (e.g.,
> to disable an invariant check for the purposes of testing) hasn't
> materialized
> yet.
>
> I'll send you a CL. If we do need to expose them for configuration by
> chrome
> tests, we can expose narrow methods for that purpose alone.
>
>
> https://codereview.chromium.org/803873004/
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.