Skip to content

GitLab

T
tangled
Commit 03af6a3c authored by Findit's avatar Findit
Browse files
Options

Revert "[Paint Preview] Post task captured callback" 

This reverts commit c669ac6b.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 837649 as the
culprit for flakes in the build cycles as shown on:
https://analysis.chromium.org/p/chromium/flake-portal/analysis/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vYzY2OWFjNmIyZmYzMWU2OTk5N2U1MTM1MzBjZTEwOTVlODI2MTBiYQw

Sample Failed Build: https://ci.chromium.org/b/8860723769678942640

Sample Failed Step: browser_tests on Mac-10.15

Sample Flaky Test: All/PaintPreviewBrowserTest.DontReloadInRenderProcessExit/FileSystem

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}


Change-Id: Id23bade14aba40cafee9748b661597e8c04fb1fd
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1146573
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2595859
Cr-Commit-Position: refs/heads/master@{#837830}
parent 6c2905dd
Hide whitespace changes
Inline Side-by-side
Showing with 12 additions and 100 deletions
chrome/browser/paint_preview/paint_preview_browsertest.cc
View file @ 03af6a3c
......@@ -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.
......@@ -73,9 +46,6 @@ class PaintPreviewBrowserTest
PaintPreviewBrowserTest() = default;
~PaintPreviewBrowserTest() override = default;
PaintPreviewBrowserTest(const PaintPreviewBrowserTest&) = delete;
PaintPreviewBrowserTest& operator=(const PaintPreviewBrowserTest&) = delete;
void SetUp() override {
ASSERT_TRUE(temp_dir_.CreateUniqueTempDir());
......@@ -122,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.
......@@ -173,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) {
......@@ -359,57 +324,6 @@ IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest,
loop.Run();
}
// https://crbug.com/1146573. If a renderer crashes,
// WebContentsObserver::RenderFrameDeleted is called quite early on.
// DecrementCapturerCount can cause the WebContents to be reloaded. This should
// be deferred using PostTask.
IN_PROC_BROWSER_TEST_P(PaintPreviewBrowserTest, DontReloadInRenderProcessExit) {
LoadPage(http_server_.GetURL("a.com", "/hello.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);
base::RunLoop loop;
auto params = MakeParams();
client->CapturePaintPreview(
params, web_contents->GetMainFrame(),
// This callback is now posted so it shouldn't cause a crash.
base::BindOnce(
[](content::WebContents* web_contents, base::UnguessableToken guid,
mojom::PaintPreviewStatus status,
std::unique_ptr<CaptureResult> result) {
EXPECT_EQ(status, mojom::PaintPreviewStatus::kFailed);
EXPECT_EQ(result, nullptr);
web_contents->DecrementCapturerCount(true);
web_contents->DecrementCapturerCount(true);
},
web_contents)
.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.
loop.Run();
}
INSTANTIATE_TEST_SUITE_P(All,
PaintPreviewBrowserTest,
testing::Values(RecordingPersistence::kFileSystem,
......
components/paint_preview/browser/paint_preview_client.cc
View file @ 03af6a3c
......@@ -560,18 +560,16 @@ void PaintPreviewClient::OnFinished(
// At a minimum one frame was captured successfully, it is up to the
// caller to decide if a partial success is acceptable based on what is
// contained in the proto.
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE,
base::BindOnce(std::move(document_data->callback), guid,
document_data->had_error
? mojom::PaintPreviewStatus::kPartialSuccess
: mojom::PaintPreviewStatus::kOk,
std::move(*document_data).IntoCaptureResult()));
std::move(document_data->callback)
.Run(guid,
document_data->had_error
? mojom::PaintPreviewStatus::kPartialSuccess
: mojom::PaintPreviewStatus::kOk,
std::move(*document_data).IntoCaptureResult());
} else {
// A proto could not be created indicating all frames failed to capture.
base::SequencedTaskRunnerHandle::Get()->PostTask(
FROM_HERE, base::BindOnce(std::move(document_data->callback), guid,
mojom::PaintPreviewStatus::kFailed, nullptr));
std::move(document_data->callback)
.Run(guid, mojom::PaintPreviewStatus::kFailed, {});
}
all_document_data_.erase(guid);
}
......
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