New issue
Advanced search Search tips

Issue 649632 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner: ----
Closed: Dec 2017
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug

Blocking:
issue 632361



Sign in to add a comment

Kill prefetch prerenders more aggressively

Project Member Reported by mattcary@chromium.org, Sep 23 2016

Issue description

Prerenders for prefetching probably live to long. Example: the prerenderer in NoStatePrefetchBrowserTest.PrefetchReusedColdHistograms times out, when it probably should have been killed/gracefully exit.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Oct 26 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/66351bd0a0fc8740749663fe765a301e1a175538

commit 66351bd0a0fc8740749663fe765a301e1a175538
Author: pasko <pasko@chromium.org>
Date: Wed Oct 26 13:47:46 2016

[NoStatePrefetch] Kill renderer after preload scanning

The design doc is linked in the first bug. Main goal: remove Prerender.

Currently NoState Prefetch uses the same logic to manage renderers as the
PrerenderManager. Prefetch code is still living in PrerenderManager, separating
the code from there is planned after we finalize on how much of the
functionality of PrerenderManager is needed.

This is the first tweak in the renderer lifetime: the renderer asks to be killed
as soon as the main resource is fully preload-scanned and all possible
subresources are requested, special Prerendering FinalStatus is recorded to allow:
* testing
* provide a hint that a new prefetch can be started soon

Browsertest changes:

* TestPrerender now keeps FinalStatus for longer, to be able to verify that it
  is correct even after the TestPrerenderContents is destroyed.

* Prefetch browsertests wait only for creation of PrerenderContents, Prerender
  tests have more waiting for page loads on top of that

* PrerenderTestUrlImpl is removed to eliminate one hop through protected method
  in the parent class, which also simplified the prefetch side of the tests: no
  need to mention how many page loads to wait for

* Disabled two tests that load a non-HTML document, correctly killing the
  renderer will be done in later changes.

BUG=632361, 649632 

Review-Url: https://codereview.chromium.org/2411863002
Cr-Commit-Position: refs/heads/master@{#427678}

[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_final_status.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_final_status.h
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_manager.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_manager.h
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_message_filter.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_message_filter.h
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_test_utils.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/browser/prerender/prerender_test_utils.h
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/common/prerender_messages.h
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/renderer/prerender/prerender_dispatcher.cc
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/chrome/renderer/prerender/prerender_dispatcher.h
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/third_party/WebKit/Source/web/tests/PrerenderingTest.cpp
[modify] https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538/third_party/WebKit/public/platform/WebPrerenderingSupport.h

Project Member

Comment 2 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f69bf24f0861768f03628206668ddb84f55dc8d4

commit f69bf24f0861768f03628206668ddb84f55dc8d4
Author: foolip <foolip@chromium.org>
Date: Thu Oct 27 09:08:20 2016

Revert of [NoStatePrefetch] Kill renderer after preload scanning (patchset #12 id:240001 of https://codereview.chromium.org/2411863002/ )

Reason for revert:
"NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
shortly after this landed.

BUG= 659697 

Original issue's description:
> [NoStatePrefetch] Kill renderer after preload scanning
>
> The design doc is linked in the first bug. Main goal: remove Prerender.
>
> Currently NoState Prefetch uses the same logic to manage renderers as the
> PrerenderManager. Prefetch code is still living in PrerenderManager, separating
> the code from there is planned after we finalize on how much of the
> functionality of PrerenderManager is needed.
>
> This is the first tweak in the renderer lifetime: the renderer asks to be killed
> as soon as the main resource is fully preload-scanned and all possible
> subresources are requested, special Prerendering FinalStatus is recorded to allow:
> * testing
> * provide a hint that a new prefetch can be started soon
>
> Browsertest changes:
>
> * TestPrerender now keeps FinalStatus for longer, to be able to verify that it
>   is correct even after the TestPrerenderContents is destroyed.
>
> * Prefetch browsertests wait only for creation of PrerenderContents, Prerender
>   tests have more waiting for page loads on top of that
>
> * PrerenderTestUrlImpl is removed to eliminate one hop through protected method
>   in the parent class, which also simplified the prefetch side of the tests: no
>   need to mention how many page loads to wait for
>
> * Disabled two tests that load a non-HTML document, correctly killing the
>   renderer will be done in later changes.
>
> BUG=632361, 649632 
>
> Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> Cr-Commit-Position: refs/heads/master@{#427678}

TBR=jochen@chromium.org,csharrison@chromium.org,droger@chromium.org,mattcary@chromium.org,mkwst@chromium.org,pasko@chromium.org
# Skipping CQ checks because original CL landed less than 1 days ago.
NOPRESUBMIT=true
NOTREECHECKS=true
NOTRY=true
BUG=632361, 649632 

Review-Url: https://codereview.chromium.org/2452313002
Cr-Commit-Position: refs/heads/master@{#427974}

[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_final_status.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_final_status.h
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_manager.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_manager.h
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_message_filter.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_message_filter.h
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_test_utils.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/browser/prerender/prerender_test_utils.h
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/common/prerender_messages.h
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/renderer/prerender/prerender_dispatcher.cc
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/chrome/renderer/prerender/prerender_dispatcher.h
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/third_party/WebKit/Source/web/tests/PrerenderingTest.cpp
[modify] https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4/third_party/WebKit/public/platform/WebPrerenderingSupport.h

Project Member

Comment 3 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/07afa9f65f71178f9be52e2738303eee3c9fdb0d

commit 07afa9f65f71178f9be52e2738303eee3c9fdb0d
Author: pasko <pasko@chromium.org>
Date: Thu Oct 27 15:01:15 2016

Reland: [NoStatePrefetch] Kill renderer after preload scanning

Remove the inherently flaky test
NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch and revert the
following revert:

> Reason for revert:
> "NoStatePrefetchBrowserTest.OpenTaskManagerBeforePrefetch" became flaky
> shortly after this landed.
>
> BUG= 659697 
> Original issue's description:
> > [NoStatePrefetch] Kill renderer after preload scanning
> >
> > The design doc is linked in the first bug. Main goal: remove Prerender.
> >
> > Currently NoState Prefetch uses the same logic to manage renderers as the
> > PrerenderManager. Prefetch code is still living in PrerenderManager, separating
> > the code from there is planned after we finalize on how much of the
> > functionality of PrerenderManager is needed.
> >
> > This is the first tweak in the renderer lifetime: the renderer asks to be killed
> > as soon as the main resource is fully preload-scanned and all possible
> > subresources are requested, special Prerendering FinalStatus is recorded to allow:
> > * testing
> > * provide a hint that a new prefetch can be started soon
> >
> > Browsertest changes:
> >
> > * TestPrerender now keeps FinalStatus for longer, to be able to verify that it
> >   is correct even after the TestPrerenderContents is destroyed.
> >
> > * Prefetch browsertests wait only for creation of PrerenderContents, Prerender
> >   tests have more waiting for page loads on top of that
> >
> > * PrerenderTestUrlImpl is removed to eliminate one hop through protected method
> >   in the parent class, which also simplified the prefetch side of the tests: no
> >   need to mention how many page loads to wait for
> >
> > * Disabled two tests that load a non-HTML document, correctly killing the
> >   renderer will be done in later changes.
> >
> > BUG=632361,  649632 
> >
> > Committed: https://crrev.com/66351bd0a0fc8740749663fe765a301e1a175538
> > Cr-Commit-Position: refs/heads/master@{#427678}
> Committed: https://crrev.com/f69bf24f0861768f03628206668ddb84f55dc8d4
> Cr-Commit-Position: refs/heads/master@{#427974}

TBR=jochen@chromium.org,csharrison@chromium.org,mattcary@chromium.org,mkwst@chromium.org,pasko@chromium.org
BUG=632361,  649632 

Review-Url: https://codereview.chromium.org/2455653005
Cr-Commit-Position: refs/heads/master@{#428027}

[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_browsertest.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_final_status.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_final_status.h
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_manager.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_manager.h
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_message_filter.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_message_filter.h
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_test_utils.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/browser/prerender/prerender_test_utils.h
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/common/prerender_messages.h
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/renderer/prerender/prerender_dispatcher.cc
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/chrome/renderer/prerender/prerender_dispatcher.h
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/third_party/WebKit/Source/web/tests/PrerenderingTest.cpp
[modify] https://crrev.com/07afa9f65f71178f9be52e2738303eee3c9fdb0d/third_party/WebKit/public/platform/WebPrerenderingSupport.h

Project Member

Comment 4 by bugdroid1@chromium.org, Oct 27 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/55713eceac5ddad41cbf6be7b4f4864dffcb8722

commit 55713eceac5ddad41cbf6be7b4f4864dffcb8722
Author: pasko <pasko@chromium.org>
Date: Thu Oct 27 15:57:15 2016

[NoStatePrefetch] Re-enable non-html tests

After the prefetchFinished() notification was moved to
Document::finishedParsing(), it actually started executing even for non-HTML
documents, which kills the renderer properly, and hence the tests can be
re-enabled.

Initially prefetchFinished() was called HTMLDocumentParser::end() which made
these tests fail, but then the call was moved to the better place during review.

BUG=632361, 649632 

Review-Url: https://codereview.chromium.org/2454783002
Cr-Commit-Position: refs/heads/master@{#428035}

[modify] https://crrev.com/55713eceac5ddad41cbf6be7b4f4864dffcb8722/chrome/browser/prerender/prerender_nostate_prefetch_browsertest.cc

Comment 5 by pasko@chromium.org, Dec 21 2017

Status: Fixed (was: Untriaged)

Sign in to add a comment