Changed active URL handling in RenderView::OnMessageReceived and RenderView::...

Changed active URL handling in RenderView::OnMessageReceived and RenderView::OnNavigate. Now we call SetActiveURL directly so that we don't unset the URL when falling out of scope.

We can try this in the render process on dev channel for a few weeks. If we're happy with the results, we do the same thing on the plugin process.

BUG=22033
TEST=Trigger a crash in the render process and see if the dump includes the URL

[cpu_(ooo_6.6-7.5), 2009-10-21 19:39:37]

LGTM

Please add sky@ to the review.

[sky, 2009-10-23 16:47:00]

I'm not opposed to this, but I think we can do more. The reason I didn't
initially go this route is I was worried about a renderer serving multiple
pages. In this case the most recent url passed in a message it not necessarily
that helpful. If we're going to go this route we should probably log the most
recent couple of urls.

One big source of crashes the code I added misses out on is timer related
crashes. If we made timers have a handle back to the document in theory we
should be able to dig out the url and use that. Presumably that would give more
accurate results.

I'm also adding Jim to the cc list has he mentioned this code may be impacting
performance. Your change would likely make performance better though as we
wouldn't be constantly changing the value.

Thoughts?

[jschuh, 2009-10-23 17:22:21]

On 2009/10/23 16:47:00, sky wrote:
> I'm not opposed to this, but I think we can do more. The reason I didn't
> initially go this route is I was worried about a renderer serving multiple
> pages. In this case the most recent url passed in a message it not necessarily
> that helpful. If we're going to go this route we should probably log the most
> recent couple of urls.
> 

I agree, but logging multiple URLs means adding fields to the breakpad report.
So, we decided to test out a simpler approach on dev channel, with the rationale
that the reported URLs would usually be useful due to locality of reference in
browsing habits. I figure it's an easy change that's worth a try, but you know
the code a lot better than I do.

> One big source of crashes the code I added misses out on is timer related
> crashes. If we made timers have a handle back to the document in theory we
> should be able to dig out the url and use that. Presumably that would give
more
> accurate results.
> 

I can dig deeper into this and let you know what I find.

> I'm also adding Jim to the cc list has he mentioned this code may be impacting
> performance. Your change would likely make performance better though as we
> wouldn't be constantly changing the value.

I found bug 25366 while looking into this issue. I think that might have been
the culprit.

[sky, 2009-10-23 17:28:18]

It shouldn't necessarily be that bad to add more fields to the crash dump.

I agree with your analysis, lets give it a shot.

LGTM

If you could look into logging urls from timers, that would be great!

Tony is also doing a change to log urls from networking, so I've cc'd him too.

[cevans, 2009-10-23 20:01:00]

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

Justin, you can close this original code review.

We should probably leave the bug open whilst we work out what is going on
with browser and plugin crashes.

On Fri, Oct 23, 2009 at 10:28 AM, <sky@chromium.org> wrote:

> It shouldn't necessarily be that bad to add more fields to the crash dump.
>
> I agree with your analysis, lets give it a shot.
>
> LGTM
>
> If you could look into logging urls from timers, that would be great!
>
> Tony is also doing a change to log urls from networking, so I've cc'd him
> too.
>
>
> http://codereview.chromium.org/307020
>