Remove call of PrerenderTracker::TryCancelOnIOThread in ChromeContentBrowserClient::CanCreateWindow…

Remove call of PrerenderTracker::TryCancelOnIOThread in ChromeContentBrowserClient::CanCreateWindow. I moved cancelling the prerender on the UI thread.

BUG=304341
R=jochen@chromium.org, tburkard@chromium.org

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

[jam, 2014-01-05 21:59:50]

This is mostly a revert of 217182, with the only difference being that I added
an early return in HandleBlockedPopupOnUIThread for the prerendering case. I'm
doing this in the context of removing all
PrerenderTracker::TryCancel/TryCancelOnIOThread calls on the IO thread as that
won't work with out of process iframes. This is now the last call after my other
cls land.

I looked through bug 269449 and 265819 and couldn't reproduce a crash with
offline Gmail with this code. I also set breakpoints on the lines that were
changed in r215881 for these bugs and they were never hit. Is there another
repro I could try?

[tburkard_google.com, 2014-01-06 16:47:16]

lgtm

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

[tburkard, 2014-01-06 16:47:59]

lgtm

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

[jam, 2014-01-06 17:03:25]

After chatting with Timo, I'll wait for Jochen to review since he's most
familiar with this.

[jochen (gone - plz use gerrit), 2014-01-06 19:37:10]

https://codereview.chromium.org/112573008/diff/200001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/112573008/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:107: return false;
if you return false here, you need to adopt route_id and
session_storage_namespace.

At this point, the RV is already created, and if you just drop them on the
floor, you'll hit a bunch of checks later on when the RV starts to send IPCs.

[jam, 2014-01-06 21:45:21]

ptal

https://codereview.chromium.org/112573008/diff/200001/chrome/browser/prerende...
File chrome/browser/prerender/prerender_contents.cc (right):

https://codereview.chromium.org/112573008/diff/200001/chrome/browser/prerende...
chrome/browser/prerender/prerender_contents.cc:107: return false;
On 2014/01/06 19:37:11, jochen wrote:
> if you return false here, you need to adopt route_id and
> session_storage_namespace.

I see, ok thanks, I missed that subtlety.

I've made the content layer handle this, since otherwise every embedder would
have to do this. It's also easier for content to destroy the RenderView since it
has access to these IPCs. Per your comment about background contents, I check
first to see if the RenderViewHost is created, to handle the case where the
embedder returns false but still creates a WebContents.

For the SessionStorageNamespace, the object is deleted after the current method
goes out of scope.

> 
> At this point, the RV is already created, and if you just drop them on the
> floor, you'll hit a bunch of checks later on when the RV starts to send IPCs.

[jochen (gone - plz use gerrit), 2014-01-07 08:10:37]

overall lgtm

https://codereview.chromium.org/112573008/diff/780001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/112573008/diff/780001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1374: Send(new
ViewMsg_Close(route_id));
route_id might be MSG_ROUTING_NONE.

in that case, no view was created.

maybe we should update the comment on ShouldCreateWebContents to clarify that an
embedder has to syncrhonously adopt the route_id if it returns false, or
otherwise the view is closed?

[jam, 2014-01-07 19:37:31]

thanks, done

https://codereview.chromium.org/112573008/diff/780001/content/browser/web_con...
File content/browser/web_contents/web_contents_impl.cc (right):

https://codereview.chromium.org/112573008/diff/780001/content/browser/web_con...
content/browser/web_contents/web_contents_impl.cc:1374: Send(new
ViewMsg_Close(route_id));
On 2014/01/07 08:10:38, jochen wrote:
> route_id might be MSG_ROUTING_NONE.
> 
> in that case, no view was created.
> 
> maybe we should update the comment on ShouldCreateWebContents to clarify that
an
> embedder has to syncrhonously adopt the route_id if it returns false, or
> otherwise the view is closed?

Done.