Chrome: Crash Report : InstantService::MaybeTranslateInstantPathOnIO

Chrome: Crash Report : InstantService::MaybeTranslateInstantPathOnIO

Fixes crasher bug where the |request| object being served was becoming
stale.  Computes the ID to URL path mapping earlier in the serving
sequence.

BUG=222272
TEST=Manual, observe NTP.  Also observe crash reports cease.
R=estade@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=189423

[dhollowa, 2013-03-20 18:37:16]

- estade (he's OOO)
+ dbeam (thanks!)

[Dan Beam, 2013-03-20 19:26:13]

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
File chrome/browser/ui/webui/ntp/thumbnail_source.cc (right):

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:86: std::string raw_path =
request->url().path().substr(1);
why .substr(1)?

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:90: id_to_url_map_[raw_path] =
path;
nit:

  id_to_url_map_[raw_path] =
      InstantService::MaybeTranslateInstantPathOnIO(request, raw_path);

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:93: }
should there be a NOTREACHED() here or anything?

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
File chrome/browser/ui/webui/ntp/thumbnail_source.h (right):

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:53: // Transient mappings from a
ID-based path to a URL-based path.
an ID-based, an URL-based

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:55: // Mapppings are added in
ShouldServiceRequest() and erased once
Mappings

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:58: // to the |StartDataRequest|
call.
nit: |StartDataRequest()|

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:59: mutable std::map<std::string,
std::string> id_to_url_map_;
why does this need to be mutable vs. just making the method non-const?

[dhollowa, 2013-03-20 19:58:54]

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
File chrome/browser/ui/webui/ntp/thumbnail_source.cc (right):

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:86: std::string raw_path =
request->url().path().substr(1);
On 2013/03/20 19:26:13, Dan Beam wrote:
> why .substr(1)?

Done.  Added comment.

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:90: id_to_url_map_[raw_path] =
path;
On 2013/03/20 19:26:13, Dan Beam wrote:
> nit:
> 
>   id_to_url_map_[raw_path] =
>       InstantService::MaybeTranslateInstantPathOnIO(request, raw_path);

Done.

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.cc:93: }
On 2013/03/20 19:26:13, Dan Beam wrote:
> should there be a NOTREACHED() here or anything?

Not really, no.  It is allowable to request a non-conformant URL, we just don't
want to serve anything back.  This would not be a Chromium programmer error, but
a web-programmer error.  So at most a log warning might be appropriate, but
that's not common in these data-source classes, so I've left it out.

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
File chrome/browser/ui/webui/ntp/thumbnail_source.h (right):

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:53: // Transient mappings from a
ID-based path to a URL-based path.
On 2013/03/20 19:26:13, Dan Beam wrote:
> an ID-based, an URL-based

Done.

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:55: // Mapppings are added in
ShouldServiceRequest() and erased once
On 2013/03/20 19:26:13, Dan Beam wrote:
> Mappings

Done.

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:58: // to the |StartDataRequest|
call.
On 2013/03/20 19:26:13, Dan Beam wrote:
> nit: |StartDataRequest()|

Done.

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:59: mutable std::map<std::string,
std::string> id_to_url_map_;
On 2013/03/20 19:26:13, Dan Beam wrote:
> why does this need to be mutable vs. just making the method non-const?

Because it is altered in the StartDataRequest() call to clear old mappings.

[dhollowa, 2013-03-20 20:02:08]

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
File chrome/browser/ui/webui/ntp/thumbnail_source.h (right):

https://codereview.chromium.org/12529027/diff/1/chrome/browser/ui/webui/ntp/t...
chrome/browser/ui/webui/ntp/thumbnail_source.h:59: mutable std::map<std::string,
std::string> id_to_url_map_;
On 2013/03/20 19:58:54, dhollowa wrote:
> On 2013/03/20 19:26:13, Dan Beam wrote:
> > why does this need to be mutable vs. just making the method non-const?
> 
> Because it is altered in the StartDataRequest() call to clear old mappings.

I realized I didn't really answer your question here... StartDataRequest() is an
inherited method, so this would require modification for all the data sources. 
And given that id_to_url_map_ is a form of cache, mutable seemed quite
appropriate.

[Dan Beam, 2013-03-20 20:05:57]

lgtm

[dhollowa, 2013-03-20 22:01:37]

Committed patchset #2 manually as r189423 (presubmit successful).