Temporary fix for installer/visitedlink crash (crbug.com/171475)

Temporary fix for installer/visitedlink crash (crbug.com/171475)

Force VisitedLinkMaster to be created after installer is finished (to avoid crash), but before browsing starts, to ensure visited link events on the first tab are caught.

TEST= no crash when installing from installer, visited links still work.
BUG=171475, 160025
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=184687

[Cait (Slow), 2013-02-07 15:54:39]

sky: PTAL, this should fix the initial visited links bug (which repros in M25
too), w/o causing the first run crash. 

Thanks,
Cait

[sky, 2013-02-11 17:03:32]

How does this fix the bug? Won't prefs still cause VisitedLinkMaster to be
loaded, which loads history and opens the db before the importer finishes?

[Cait (Slow), 2013-02-11 19:01:55]

On 2013/02/11 17:03:32, sky wrote:
> How does this fix the bug? Won't prefs still cause VisitedLinkMaster to be
> loaded, which loads history and opens the db before the importer finishes?

Apologies for the confusion -- The importer crash was caused by
https://codereview.chromium.org/11928004, a change which I landed on the M24
branch which forced the VisitedLinkMaster to be created as soon as the profile
was created, rather than lazily, when it was needed.

As that change did not get merged into M25, the current status of M25 is that
the importer crash does not repro, but crbug.com/160025 (visited links not
marked as visited) does. 

This CL should fix the visited link bug, w/o causing the importer crash.

[sky, 2013-02-11 21:31:41]

Since Brett reviewed the other, I'm letting him handle this.

[brettw, 2013-02-11 21:44:51]

It's pretty unfortunate to add this code to browser main. What's the long-term
plan for making it work? Is there anything else we can do?

[Cait (Slow), 2013-02-11 22:43:17]

On 2013/02/11 21:44:51, brettw wrote:
> It's pretty unfortunate to add this code to browser main. What's the long-term
> plan for making it work? Is there anything else we can do?

Agree. 

In the long term (read: as of M26), VisitedLinkMaster is no longer a
Profile-keyed-service, and is instead owned by the HistoryService, and gets
created at the same time as the history service
(https://chromiumcodereview.appspot.com/11573060/)-- I have not been able to
repro either issue after this change, so we could think about merging this
change into M25 (not sure how feasible that is, though).

The other option would be to have VisitedLinkMaster wait until the importer is
finished before running its init method, but I'm hesitant to mess about too much
here, since the changes won't have time to bake on the dev channel.

[brettw, 2013-02-11 23:22:18]

Is this change just for the 25 branch or is it going in on trunk as well? Also,
I don't see what part of this change forces it to happen after importing. Can
you be more explicit about that?

[Cait (Slow), 2013-02-12 00:19:07]

On 2013/02/11 23:22:18, brettw wrote:
> Is this change just for the 25 branch or is it going in on trunk as well?
Also,
> I don't see what part of this change forces it to happen after importing. Can
> you be more explicit about that?

This change is for the M25 branch only (not the trunk). 

Since this code is executed after the call to OnImportFinished() (line 1213 of
chrome_browser_main.cc), I believe it is safe to assume that the importer is
done. (The instantiation of VisitedLinkMaster could also be done in
ProfileManager::OnImportFinished, instead of browser_main if you prefer).

[brettw, 2013-02-12 00:21:59]

Referring to
https://code.google.com/p/chromium/issues/detail?id=171475#c20
I thought the bug is that the VisitedLinkMaster was created too early and would
hang onto the profile lock. Is that wrong? With my previous question I was
trying to get to "how does this patch ensure that it doesn't happen before
importing."

[Cait (Slow), 2013-02-12 01:21:38]

On 2013/02/12 00:21:59, brettw wrote:
> Referring to
> https://code.google.com/p/chromium/issues/detail?id=171475#c20
> I thought the bug is that the VisitedLinkMaster was created too early and
would
> hang onto the profile lock. Is that wrong? With my previous question I was
> trying to get to "how does this patch ensure that it doesn't happen before
> importing."

The bug is that if the VisitedLinkManager is created at the same time as the
profile (which is right before the importer runs), it triggers the creation of
the HistoryService, and so the importer and chrome are  both trying to access
the HistoryDB at the same time, and crashing.

The VisitedLinkMaster is only used by the HistoryService, so it will either get
created in chrome_browser_main by this patch (which happens after importing) or
by the HistoryService when the it is first created, which presumably happens
after importing as well.

[sky, 2013-02-12 03:18:45]

Can we not do anything and leave 160025 broke in m25? That seems less risky than
trying to do merge this.

[Cait (Slow), 2013-02-12 15:20:01]

On 2013/02/12 03:18:45, sky wrote:
> Can we not do anything and leave 160025 broke in m25? That seems less risky
than
> trying to do merge this.

I am ok with this approach if others are.

[sky, 2013-02-12 18:02:33]

That's my vote.

  -Scott

On Tue, Feb 12, 2013 at 7:20 AM,  <caitkp@chromium.org> wrote:
> On 2013/02/12 03:18:45, sky wrote:
>>
>> Can we not do anything and leave 160025 broke in m25? That seems less
>> risky
>
> than
>>
>> trying to do merge this.
>
>
> I am ok with this approach if others are.
>
> https://chromiumcodereview.appspot.com/12221069/

[brettw, 2013-02-12 18:35:29]

Can you summarize the impact of that bug? When will visited link coloring fail
exactly?

[Cait (Slow), 2013-02-12 19:23:54]

On 2013/02/12 18:35:29, brettw wrote:
> Can you summarize the impact of that bug? When will visited link coloring fail
> exactly?

It fails in the first tab opened, for the first domain navigated to. The
renderer for this tab is created before the VisitedLinkMaster, and so the
VisitedLinkMaster is unaware of it.

[brettw, 2013-02-12 21:52:42]

I guess I'm less enthusiastic about breaking that and I'd prefer a M25-specific
hack as long as it doesn't have perf impact.

[brettw, 2013-02-12 22:02:11]

Sorry if I don't understand all of this new profile stuff. Is it possible to
listen for RenderProcessHost creation and create the visited link master then
rather than in browser main? I just really hate adding stuff to browser main.

[Cait (Slow), 2013-02-12 22:36:08]

On 2013/02/12 22:02:11, brettw wrote:
> Sorry if I don't understand all of this new profile stuff. Is it possible to
> listen for RenderProcessHost creation and create the visited link master then
> rather than in browser main? I just really hate adding stuff to browser main.

Listening for renderProcessHost creation is the best way to ensure that the
visitedLinkMaster gets created in time -- I'm just not sure who (or what) should
handle the listening and creating.

[brettw, 2013-02-13 21:57:24]

I think I'm OK with this as a temporary fix on branch. Question below.

https://chromiumcodereview.appspot.com/12221069/diff/1/chrome/browser/visited...
File chrome/browser/visitedlink/visitedlink_event_listener.cc (right):

https://chromiumcodereview.appspot.com/12221069/diff/1/chrome/browser/visited...
chrome/browser/visitedlink/visitedlink_event_listener.cc:127:
updaters_[process->GetID()] =
Below in the NOTIFICATION_RENDERER_PROCESS_CREATED we also send the table. Here
we don't. Why don't we need to send the RPH's the table in this case?

[Cait (Slow), 2013-02-14 16:58:13]

https://chromiumcodereview.appspot.com/12221069/diff/1/chrome/browser/visited...
File chrome/browser/visitedlink/visitedlink_event_listener.cc (right):

https://chromiumcodereview.appspot.com/12221069/diff/1/chrome/browser/visited...
chrome/browser/visitedlink/visitedlink_event_listener.cc:127:
updaters_[process->GetID()] =
On 2013/02/13 21:57:24, brettw wrote:
> Below in the NOTIFICATION_RENDERER_PROCESS_CREATED we also send the table.
Here
> we don't. Why don't we need to send the RPH's the table in this case?
We do need to send the table here. This was a mistake on my part. I looked into
sending the table and it looks like this will require changing the init process
quite a bit (the table is not ready yet when the Listener is created).

As it stands, this code isn't doing much, so I've removed it.

[brettw, 2013-02-14 18:50:39]

https://chromiumcodereview.appspot.com/12221069/diff/12001/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/12221069/diff/12001/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:1265: // TODO(caitkp): Hopefully remove
this in M26 when refactoring of VisitedLink
Since you're landing this on the branch only, can this comment be reworded to
say this is a temporary measure only on M25 because it's fixed by yada yada
yada. (This is going on the branch only, right?)

[Cait (Slow), 2013-02-14 20:24:53]

https://chromiumcodereview.appspot.com/12221069/diff/12001/chrome/browser/chr...
File chrome/browser/chrome_browser_main.cc (right):

https://chromiumcodereview.appspot.com/12221069/diff/12001/chrome/browser/chr...
chrome/browser/chrome_browser_main.cc:1265: // TODO(caitkp): Hopefully remove
this in M26 when refactoring of VisitedLink
On 2013/02/14 18:50:39, brettw wrote:
> Since you're landing this on the branch only, can this comment be reworded to
> say this is a temporary measure only on M25 because it's fixed by yada yada
> yada. (This is going on the branch only, right?)

Fixed. and yes, this is going on the branch only.

[brettw, 2013-02-14 21:41:04]

lgtm

[Cait (Slow), 2013-02-26 19:11:10]

Committed patchset #3 manually as r184687 (presubmit successful).