New issue
Advanced search Search tips

Issue 178755 link

Starred by 3 users

Issue metadata

Status: Fixed
Owner:
Closed: Feb 2013
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 0
Type: Bug-Regression



Sign in to add a comment

If a prerender of a page that prerenders another page is cancelled, the page will still be prerendered.

Project Member Reported by mmenke@chromium.org, Feb 27 2013

Issue description

Version: 25.0.1364.97
OS: Win7 (probably all)

What steps will reproduce the problem?
1.  Go to a page (say page 1) that prerenders a page (say page 2) that prerenders another page (say page 3).
2.  Remove the prerender link.

What is the expected output? What do you see instead?
Once the link rel tag is removed from page 1, the prerender of page 2 should stop, and page 3 should not be prerendered.

Instead, the prerender of page 2 is stopped, and page 3 is prerendered.  What's worse, if page 3 renders page 4, once page 3 times out, page 4 will be prerendered, and this will go on indefinitely, until a page that doesn't prerender another page is reached.

Strangely, if the original prerender (Page 1 prerendering page 2) was allowed to timeout instead, page 3 will never be prerendered, as expected.

This is sufficiently bad that we may want to disable prerendering in M25 completely, though I suspect the fix will be simple and not too complicated.
 

Comment 1 by mmenke@chromium.org, Feb 27 2013

Labels: -Type-Bug Type-Regression

Comment 2 by mmenke@chromium.org, Feb 27 2013

The issue is that we rely on destruction of a PrerenderContents to destroy the pending prerenders it owns.  With a recent rearchitecture of prerendering, we don't actually create handles until we try and start prerendering.

So now these queued prerenders are only started after the old prerender stops and the RenderView has been destroyed.

One obvious fix (That wouldn't work) is to never start prerenders for destroyed RenderViews, but that isn't sufficient - we try and start waiting prerenders just as we're destroying the old PrerenderContents, so the old PrerenderContents is in the PendingDelete list at the time, but its RenderView still exists.

I believe that not starting prerenders created by prerenders in the pending delete list, *and* checking if the referring RenderView still exists would completely fix the issue.  The existence check may not be strictly necessary, but I worry about races without it.

Comment 3 by mmenke@chromium.org, Feb 27 2013

Looks like we already cancel in the source render view was closed case, so just need to handle the prerendering render view is being deleted case.

Comment 4 by mmenke@chromium.org, Feb 27 2013

Owner: mmenke@chromium.org
Status: Assigned

Comment 5 by mmenke@chromium.org, Feb 28 2013

Labels: Merge-Requested
Perhaps I'm thinking this is higher priority than it really is, but if you have a circular list of prerenders on your site, we can end up sending requests to it for as long as Chrome is open, which I think is pretty bad.

Comment 6 by mmenke@chromium.org, Feb 28 2013

Oops...  commit-bot hasn't posted yet.  Link to CL:  https://codereview.chromium.org/12328149/

I assume we'll want to let it percolate on canary first.
Project Member

Comment 7 by bugdroid1@chromium.org, Feb 28 2013

The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=185084

------------------------------------------------------------------------
r185084 | mmenke@chromium.org | 2013-02-28T00:32:06.506417Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prerender/prerender_browsertest.cc?r1=185084&r2=185083&pathrev=185084
   M http://src.chromium.org/viewvc/chrome/trunk/src/chrome/browser/prerender/prerender_manager.cc?r1=185084&r2=185083&pathrev=185084

Don't start prerenders for prerendered pages queued for deletion.

This caused prerenders that were cancelled for some reason
to be able to launch new prerenders.

BUG= 178755 
Review URL: https://codereview.chromium.org/12328149
------------------------------------------------------------------------

Comment 8 by k...@google.com, Feb 28 2013

Labels: -Merge-Requested Merge-Approved
Project Member

Comment 9 by bugdroid1@chromium.org, Feb 28 2013

Labels: -Merge-Approved merge-merged-1364
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=185279

------------------------------------------------------------------------
r185279 | mmenke@chromium.org | 2013-02-28T18:46:15.869032Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/chrome/browser/prerender/prerender_manager.cc?r1=185279&r2=185278&pathrev=185279
   M http://src.chromium.org/viewvc/chrome/branches/1364/src/chrome/browser/prerender/prerender_browsertest.cc?r1=185279&r2=185278&pathrev=185279

Merge 185084
> Don't start prerenders for prerendered pages queued for deletion.
> 
> This caused prerenders that were cancelled for some reason
> to be able to launch new prerenders.
> 
> BUG= 178755 
> Review URL: https://codereview.chromium.org/12328149

TBR=mmenke@chromium.org
Review URL: https://codereview.chromium.org/12379019
------------------------------------------------------------------------
Labels: -Mstone-25 -merge-merged-1364 Mstone-26 Merge-Requested
Thanks, kerz!
Labels: -Merge-Requested Merge-Approved
Project Member

Comment 12 by bugdroid1@chromium.org, Feb 28 2013

Labels: -Merge-Approved merge-merged-1410
The following revision refers to this bug:
    http://src.chromium.org/viewvc/chrome?view=rev&revision=185363

------------------------------------------------------------------------
r185363 | mmenke@chromium.org | 2013-02-28T23:24:30.059437Z

Changed paths:
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/prerender/prerender_manager.cc?r1=185363&r2=185362&pathrev=185363
   M http://src.chromium.org/viewvc/chrome/branches/1410/src/chrome/browser/prerender/prerender_browsertest.cc?r1=185363&r2=185362&pathrev=185363

Merge 185084
> Don't start prerenders for prerendered pages queued for deletion.
> 
> This caused prerenders that were cancelled for some reason
> to be able to launch new prerenders.
> 
> BUG= 178755 
> Review URL: https://codereview.chromium.org/12328149

TBR=mmenke@chromium.org
Review URL: https://codereview.chromium.org/12377033
------------------------------------------------------------------------
Status: Fixed
Thanks!
Labels: Action-FeedbackNeeded
mmenke@, Could you please provide us the test URL, to verify this issue? if we can.

Thanks in advance!
Sure, here's the URL:  https://www.corp.google.com/~mmenke/prerender_launcher.html (It has 4 pages - prerender_first.html, prerender_bad.html, and prerender_good.html).  There must not be another prerender active when it's started.

Displays red text at the bottom after ~5 seconds on failure, displays green text after ~2 seconds on success.  Relies on a pair of timeouts, but I believe they're generous enough to not cause issues.
The test relies on there only being one prerender allowed per site.  This may change in the future.  An automated test that's robust against that is more tricky.  You can open up about:net-internals#prerender when the test runs (Or just after running it), and make sure that you see prerender_first and prerender_good but not prerender_bad in the list.  That list is updated on a 5 second timer.
Project Member

Comment 17 by bugdroid1@chromium.org, Mar 9 2013

Labels: -Action-FeedbackNeeded Needs-Feedback
Project Member

Comment 18 by bugdroid1@chromium.org, Mar 9 2013

Labels: -Type-Regression -Area-Internals -Mstone-26 -Feature-Preload Type-Bug-Regression Cr-Internals M-26 Cr-Conent-Preload

Comment 19 by laforge@google.com, Mar 20 2013

Labels: -Cr-Conent-Preload Cr-Content-Preload
Project Member

Comment 20 by bugdroid1@chromium.org, Apr 5 2013

Labels: Cr-Blink
Project Member

Comment 21 by bugdroid1@chromium.org, Apr 6 2013

Labels: -Cr-Content-Preload Cr-Internals-Preload

Sign in to add a comment