Fix flakiness and re-enable FormAutocompleteTest

Fix flakiness and re-enable FormAutocompleteTest

This CL re-enables FormAutocompleteTest.

Before this CL, FormAutocompleteTest was flaky.
ChromeRenderViewTest::TearDown() shutdowns ExtensionDispatcher, however
tasks from HTMLParserThread could be scheduled in renderer main thread
after the shutdown, resulting in nullptr dereference.

This CL prevents it by adding null checks before calling ContentWatcher
callbacks.

BUG=500851
Committed: https://crrev.com/cd3c684caac071b63ac11a84fbb77115aeb88573
Cr-Commit-Position: refs/heads/master@{#337976}

[kouhei (in TOK), 2015-06-30 05:18:13]

kouhei@chromium.org changed reviewers:
+ jyasskin@chromium.org

[kouhei (in TOK), 2015-06-30 05:18:13]

Would you take a look?

[kouhei (in TOK), 2015-07-05 10:19:53]

kouhei@chromium.org changed reviewers:
+ kalman@chromium.org

[kouhei (in TOK), 2015-07-05 10:19:54]

kalman: Would you take a look?

[not at google - send to devlin, 2015-07-06 16:32:46]

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:364: if (content_watcher_) {
Comment here and below,

// This can happen if the render process has already been shut down.

It's actually puzzling to me that any of this can happen if the render process
has shutdown. Surely that would mean that document elements aren't being
created? In fact is this a testing-only issue?

[kouhei (in TOK), 2015-07-07 00:00:29]

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:364: if (content_watcher_) {
On 2015/07/06 16:32:46, kalman wrote:
> Comment here and below,
> 
> // This can happen if the render process has already been shut down.
ack

> It's actually puzzling to me that any of this can happen if the render process
> has shutdown. Surely that would mean that document elements aren't being
> created? In fact is this a testing-only issue?
Yes. This is a testing-only issue. Unfortunately browser_tests can run event
loops after content_watcher_ ptr is cleared.

The unjoined HTMLParser thread may post task after OnRenderProcessShutdown is
called for tests, and the tests process message queue again in
RenderViewTest::TearDown().
I couldn't find a clean way to synchronize the parser thread.
PS1 tries to resolve it by synchronizing HTMLParser thread by loadHTML(""), but
I'm not sure which is better.

[not at google - send to devlin, 2015-07-07 19:55:51]

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:364: if (content_watcher_) {
Alright, let's go with what you have and contrary to my previous solution,
mention that this null check is because, in testing, the document lifetime
events can happen after the render process shutdown event. Linking to the bug
would be useful.

[kouhei (in TOK), 2015-07-08 09:23:32]

kalman: PTAL.

thakis: Would you OWNER review changes in chrome/? Basically this is just
re-enabling of disabled tests.

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1220813003/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:364: if (content_watcher_) {
On 2015/07/07 19:55:51, kalman wrote:
> Alright, let's go with what you have and contrary to my previous solution,
> mention that this null check is because, in testing, the document lifetime
> events can happen after the render process shutdown event. Linking to the bug
> would be useful.

Done.

[not at google - send to devlin, 2015-07-08 19:35:32]

lgtm

[kouhei (in TOK), 2015-07-09 01:25:40]

kouhei@chromium.org changed reviewers:
+ thakis@chromium.org

[kouhei (in TOK), 2015-07-09 01:25:40]

Thanks!

thakis: Would you OWNER review changes in chrome/? Basically this is just
re-enabling of disabled tests.

[Nico, 2015-07-09 01:27:03]

https://codereview.chromium.org/1220813003/diff/40001/chrome/renderer/autofil...
File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right):

https://codereview.chromium.org/1220813003/diff/40001/chrome/renderer/autofil...
chrome/renderer/autofill/form_autocomplete_browsertest.cc:71: // Flaky:
http://crbug.com/500851.
Remove this comment? Also below.

[kouhei (in TOK), 2015-07-09 01:28:48]

On 2015/07/09 01:27:03, Nico wrote:
>
https://codereview.chromium.org/1220813003/diff/40001/chrome/renderer/autofil...
> File chrome/renderer/autofill/form_autocomplete_browsertest.cc (right):
> 
>
https://codereview.chromium.org/1220813003/diff/40001/chrome/renderer/autofil...
> chrome/renderer/autofill/form_autocomplete_browsertest.cc:71: // Flaky:
> http://crbug.com/500851.
> Remove this comment? Also below.

Done.

[Nico, 2015-07-09 01:31:05]

lgtm

[kouhei (in TOK), 2015-07-09 01:31:56]

The CQ bit was checked by kouhei@chromium.org

[kouhei (in TOK), 2015-07-09 01:31:56]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1220813003/#ps60001
(title: "remove "Flaky" comments")

[commit-bot: I haz the power, 2015-07-09 01:33:44]

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

[commit-bot: I haz the power, 2015-07-09 03:08:54]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-07-09 03:10:37]

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