Issue 124393002: Remove calls of PrerenderTracker::TryCancel in ChromeResourceDispatcherHostDelegate::HandleExternal…

Issue 124393002: Remove calls of PrerenderTracker::TryCancel in ChromeResourceDispatcherHostDelegate::HandleExternal… (Closed)

Created:
6 years, 11 months ago by jam

Modified:
6 years, 11 months ago

Reviewers:
tburkard

CC:
chromium-reviews, site-isolation-reviews_chromium.org

Base URL:
svn://chrome-svn/chrome/trunk/src/

Visibility:
Public.

More Reviews

Description

Remove calls of PrerenderTracker::TryCancel in ChromeResourceDispatcherHostDelegate::HandleExternalProtocol. This short-circuited cancelling the prerendering on the IO thread. Instead, I moved cancelling the prerender on the UI thread since that calls ends up going to the UI thread right after. BUG=304341 R=tburkard@chromium.org Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=243095

Patch Set 1 #

Patch Set 2 : fix android build #

Patch Set 3 : fix chromeos test. all other platforms handle null webcontents since its async #

Patch Set 4 : sync #

Total comments: 2

Created: 6 years, 11 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+20 lines, -9 lines)			Patch
	M	chrome/browser/chromeos/external_protocol_dialog.cc	View	1 2 3	1 chunk	+0 lines, -1 line	2 comments	Download
	M	chrome/browser/renderer_host/chrome_resource_dispatcher_host_delegate.cc	View	1 2 3	4 chunks	+20 lines, -8 lines	0 comments	Download

Messages

Total messages: 3 (0 generated)

Expand Messages | Collapse Messages

tburkard

lgtm https://codereview.chromium.org/124393002/diff/520001/chrome/browser/chromeos/external_protocol_dialog.cc File chrome/browser/chromeos/external_protocol_dialog.cc (left): https://codereview.chromium.org/124393002/diff/520001/chrome/browser/chromeos/external_protocol_dialog.cc#oldcode38 chrome/browser/chromeos/external_protocol_dialog.cc:38: DCHECK(web_contents); Why this change, seems like the semantics ...

6 years, 11 months ago (2014-01-06 10:54:38 UTC) #2

jam

6 years, 11 months ago (2014-01-06 15:44:18 UTC) #3

https://codereview.chromium.org/124393002/diff/520001/chrome/browser/chromeos...
File chrome/browser/chromeos/external_protocol_dialog.cc (left):

https://codereview.chromium.org/124393002/diff/520001/chrome/browser/chromeos...
chrome/browser/chromeos/external_protocol_dialog.cc:38: DCHECK(web_contents);
On 2014/01/06 10:54:38, tburkard wrote:
> Why this change, seems like the semantics of |web_contents| hasn't changed by
> the change in the other file.
> 
> I guess this may have been a race condition in the old code already that you
are
> fixing now?

I actually had to change this to fix a crash, so the semantics changed. The
prerendering check on the IO thread must have masked this case. On all other
platforms though, web_contents is NULL tested.

Expand Messages | Collapse Messages