Skip to content

GitLab

T
tangled
Commit ffc66df6 authored by Tsuyoshi Horo's avatar Tsuyoshi Horo Committed by Chromium LUCI CQ
Browse files
Options

Revert "Reland "[Paint Preview] Post task captured callback"" 

This reverts commit 2cda8338.

Reason for revert: This test is still flaky.

https://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=All%2FPaintPreviewBrowserTest.DontReloadInRenderProcessExit%2FFileSystem

Original change's description:
> Reland "[Paint Preview] Post task captured callback"
>
> This is a reland of c669ac6b
>
> This is the test only, the fix was landed in https://crrev.com/c/2598087
>
> - the previous test did not reproduce the crash, this one has crash a
>   crash that matches the trace in the wild
>   https://ci.chromium.org/p/chromium/builders/try/linux-dcheck-off-rel/16863?
>   It differs from the previous test by
>   - calling SetNeedsReload (emulating Clank's autorefresh)
>   - navigating to another frame afterwards, triggering Unload
>
> - the previous test timed out on some builders. It's possible that
>   the callback was already called before the test's own RunlLoop
>   started, which would mean that the loop never exits. This is avoided
>   by recording whether it has run or not.
>
> Original change's description:
> > [Paint Preview] Post task captured callback
> >
> > This CL attempts to fix a bug where calling
> > WebContents::DecrementCapturerCount in the callback passed to
> > PaintPreviewClient can result in reloading a crashed render frame.
> > This is problematic and crashes if it occurs inside RenderFrameDeleted
> > and PaintPreviewClient::OnFinished is invoked. By posting the callback
> > it should be invoked after RenderFrameDeleted cleanup is done resulting
> > in DecrementCapturerCount no longer being called inside
> > RenderFrameDeleted.
> >
> > Bug: 1146573
> > Change-Id: Iccb612d89135e058d8fc06de018e7c95cd91657a
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595247
> > Commit-Queue: Calder Kitagawa <ckitagawa@chromium.org>
> > Commit-Queue: Mehran Mahmoudi <mahmoudi@chromium.org>
> > Reviewed-by: Mehran Mahmoudi <mahmoudi@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#837649}
>
> Bug: 1146573, 1160608
>
>
> Cq-Include-Trybots: luci.chromium.try:linux-dcheck-off-rel
> Change-Id: I3ef0a81bb4fa99e2f42c6278ba53931aa934793e
> Cq-Do-Not-Cancel-Tryjobs: true
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2596697
> Reviewed-by: Calder Kitagawa <ckitagawa@chromium.org>
> Commit-Queue: Fergal Daly <fergal@chromium.org>
> Auto-Submit: Fergal Daly <fergal@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#840497}

TBR=mahmoudi@chromium.org,fergal@chromium.org,ckitagawa@chromium.org,fredmello@chromium.org,chromium-scoped@luci-project-accounts.iam.gserviceaccount.com

Change-Id: Ic6a0823155037dfd95e93bf030030d643178f153
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1146573
Bug: 1160608
Cq-Include-Trybots: luci.chromium.try:linux-dcheck-off-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2612674Reviewed-by: default avatarTsuyoshi Horo <horo@chromium.org>
Commit-Queue: Tsuyoshi Horo <horo@chromium.org>
Cr-Commit-Position: refs/heads/master@{#840874}
parent 4924c8d4
Show whitespace changes
Inline Side-by-side
Showing with 4 additions and 106 deletions
chrome/browser/paint_preview/paint_preview_browsertest.cc
View file @ ffc66df6
......@@ -24,44 +24,17 @@
#include "components/paint_preview/common/test_utils.h"
#include "components/ukm/test_ukm_recorder.h"
#include "content/public/browser/notification_types.h"
#include "content/public/browser/render_process_host.h"
#include "content/public/common/content_switches.h"
#include "content/public/test/browser_test.h"
#include "net/dns/mock_host_resolver.h"
#include "net/test/embedded_test_server/embedded_test_server.h"
#include "services/metrics/public/cpp/ukm_builders.h"
#include "third_party/blink/public/common/associated_interfaces/associated_interface_provider.h"
#include "third_party/skia/include/core/SkPicture.h"
#include "third_party/skia/include/core/SkStream.h"
#include "url/gurl.h"
namespace paint_preview {
class NoOpPaintPreviewRecorder : public mojom::PaintPreviewRecorder {
public:
NoOpPaintPreviewRecorder() = default;
~NoOpPaintPreviewRecorder() override = default;
NoOpPaintPreviewRecorder(const NoOpPaintPreviewRecorder&) = delete;
NoOpPaintPreviewRecorder& operator=(const NoOpPaintPreviewRecorder&) = delete;
void CapturePaintPreview(
mojom::PaintPreviewCaptureParamsPtr params,
mojom::PaintPreviewRecorder::CapturePaintPreviewCallback callback)
override {
callback_ = std::move(callback);
}
void BindRequest(mojo::ScopedInterfaceEndpointHandle handle) {
binding_.Bind(mojo::PendingAssociatedReceiver<mojom::PaintPreviewRecorder>(
std::move(handle)));
}
private:
mojom::PaintPreviewRecorder::CapturePaintPreviewCallback callback_;
mojo::AssociatedReceiver<mojom::PaintPreviewRecorder> binding_{this};
};
// Test harness for a integration test of paint previews. In this test:
// - Each RenderFrame has an instance of PaintPreviewRecorder attached.
// - Each WebContents has an instance of PaintPreviewClient attached.
......@@ -69,10 +42,6 @@ class NoOpPaintPreviewRecorder : public mojom::PaintPreviewRecorder {
class PaintPreviewBrowserTest
: public InProcessBrowserTest,
public testing::WithParamInterface<RecordingPersistence> {
public:
PaintPreviewBrowserTest(const PaintPreviewBrowserTest&) = delete;
PaintPreviewBrowserTest& operator=(const PaintPreviewBrowserTest&) = delete;
protected:
PaintPreviewBrowserTest() = default;
~PaintPreviewBrowserTest() override = default;
......@@ -123,15 +92,6 @@ class PaintPreviewBrowserTest
return params;
}
void OverrideInterface(NoOpPaintPreviewRecorder* service) {
blink::AssociatedInterfaceProvider* remote_interfaces =
GetWebContents()->GetMainFrame()->GetRemoteAssociatedInterfaces();
remote_interfaces->OverrideBinderForTesting(
mojom::PaintPreviewRecorder::Name_,
base::BindRepeating(&NoOpPaintPreviewRecorder::BindRequest,
base::Unretained(service)));
}
void WaitForLoadStopWithoutSuccessCheck() {
// In many cases, the load may have finished before we get here. Only wait
// if the tab still has a pending navigation.
......@@ -174,6 +134,10 @@ class PaintPreviewBrowserTest
base::ScopedTempDir temp_dir_;
net::EmbeddedTestServer http_server_;
net::EmbeddedTestServer http_server_different_origin_;
private:
PaintPreviewBrowserTest(const PaintPreviewBrowserTest&) = delete;
PaintPreviewBrowserTest& operator=(const PaintPreviewBrowserTest&) = delete;
};
IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest, CaptureFrame) {
......@@ -360,72 +324,6 @@ IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest,
loop.Run();
}
// https://crbug.com/1146573 reproduction. If a renderer crashes,
// WebContentsObserver::RenderFrameDeleted. Paint preview implements this in an
// observer which in turn calls DecrementCapturerCount which can cause the
// WebContents to be reloaded on Android where we have auto-reload. This reload
// occurs *during* crash handling, leaving the frame in an invalid state and
// leading to a crash when it subsequently unloaded.
// This is fixed by deferring it to a PostTask.
// This also
IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest, DontReloadInRenderProcessExit) {
LoadPage(http_server_.GetURL("a.com", "/title1.html"));
content::WebContents* web_contents = GetWebContents();
// Override remote interfaces with a no-op.
NoOpPaintPreviewRecorder noop_recorder;
OverrideInterface(&noop_recorder);
CreateClient();
auto* client = PaintPreviewClient::FromWebContents(web_contents);
// Do this twice to simulate conditions for crash.
web_contents->IncrementCapturerCount(gfx::Size(), true);
web_contents->IncrementCapturerCount(gfx::Size(), true);
// A callback that causes the frame to reload and end up in an invalid state
// if it is allowed to run during crash handling.
base::RunLoop loop;
auto params = MakeParams();
bool did_run = false;
client->CapturePaintPreview(
params, web_contents->GetMainFrame(),
// This callback is now posted so it shouldn't cause a crash.
base::BindOnce(
[](content::WebContents* web_contents, bool* did_run_ptr,
base::UnguessableToken guid, mojom::PaintPreviewStatus status,
std::unique_ptr<CaptureResult> result) {
EXPECT_EQ(status, mojom::PaintPreviewStatus::kFailed);
EXPECT_EQ(result, nullptr);
// On Android crashed frames are marked as needing reload.
web_contents->GetController().SetNeedsReload();
web_contents->DecrementCapturerCount(true);
web_contents->DecrementCapturerCount(true);
*did_run_ptr = true;
},
web_contents, &did_run)
.Then(loop.QuitClosure()));
// Crash the renderer.
{
base::ScopedAllowBlockingForTesting scope;
content::RenderProcessHost* process =
GetWebContents()->GetMainFrame()->GetProcess();
content::RenderProcessHostWatcher crash_observer(
process, content::RenderProcessHostWatcher::WATCH_FOR_PROCESS_EXIT);
process->Shutdown(0);
crash_observer.Wait();
}
// The browser would have crashed before the loop exited if the callback was
// not posted.
if (!did_run)
loop.Run();
// Now navigate away and ensure that the frame unloads successfully.
LoadPage(http_server_.GetURL("a.com", "/title2.html"));
}
INSTANTIATE_TEST_SUITE_P(All,
PaintPreviewBrowserTest,
testing::Values(RecordingPersistence::kFileSystem,
......
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment