Commit becd697b authored by Albert Chaulk's avatar Albert Chaulk Committed by Commit Bot

Fix crash with NavigationThrottles

There is a crash in ProcessNavigationDecision, accessing an almost null
pointer. From disassembly inspection this is likely accessing the vtable
which means the Throttle got destroyed

Likely code path
- start navigation
- send throttle decision request
- navigation cancelled for some reason, throttle destroyed
- reply arrives

There was no code path to handle nulling the throttle if it got destroyed.
This CL adds this path and a second one to prevent a future
webview dtor -> throttle dtor crash

Bug: b/148126060
Test: None, can't repro crash
Change-Id: I4cb4d5720c114c97d49bae95f0768da0d8c025b1
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2023048Reviewed-by: default avatarDaniel Nicoara <dnicoara@chromium.org>
Commit-Queue: Albert Chaulk <achaulk@chromium.org>
Cr-Commit-Position: refs/heads/master@{#736039}
parent 8c93e15a
......@@ -83,7 +83,8 @@ WebviewController::MaybeGetNavigationThrottle(
if (webview_user_data &&
webview_user_data->controller()->has_navigation_delegate_) {
return std::make_unique<WebviewNavigationThrottle>(
handle, webview_user_data->controller());
handle,
webview_user_data->controller()->weak_ptr_factory_.GetWeakPtr());
}
return nullptr;
}
......@@ -159,6 +160,12 @@ void WebviewController::SendNavigationEvent(
client_->EnqueueSend(std::move(response));
}
void WebviewController::OnNavigationThrottleDestroyed(
WebviewNavigationThrottle* throttle) {
if (current_navigation_throttle_ == throttle)
current_navigation_throttle_ = nullptr;
}
void WebviewController::ClosePage() {
cast_web_contents_->ClosePage();
}
......
......@@ -57,6 +57,7 @@ class WebviewController : public CastWebContents::Delegate,
// navigation handle.
void SendNavigationEvent(WebviewNavigationThrottle* throttle,
content::NavigationHandle* navigation_handle);
void OnNavigationThrottleDestroyed(WebviewNavigationThrottle* throttle);
protected:
content::WebContents* GetWebContents() override;
......
......@@ -13,11 +13,13 @@ namespace chromecast {
WebviewNavigationThrottle::WebviewNavigationThrottle(
content::NavigationHandle* handle,
WebviewController* controller)
: NavigationThrottle(handle),
controller_(controller) {}
base::WeakPtr<WebviewController> controller)
: NavigationThrottle(handle), controller_(std::move(controller)) {}
WebviewNavigationThrottle::~WebviewNavigationThrottle() = default;
WebviewNavigationThrottle::~WebviewNavigationThrottle() {
if (controller_)
controller_->OnNavigationThrottleDestroyed(this);
}
content::NavigationThrottle::ThrottleCheckResult
WebviewNavigationThrottle::WillStartRequest() {
......@@ -29,10 +31,10 @@ WebviewNavigationThrottle::WillStartRequest() {
void WebviewNavigationThrottle::ProcessNavigationDecision(
webview::NavigationDecision decision) {
if (decision != webview::PREVENT) {
this->Resume();
Resume();
return;
}
this->CancelDeferredNavigation(content::NavigationThrottle::CANCEL);
CancelDeferredNavigation(content::NavigationThrottle::CANCEL);
}
const char* WebviewNavigationThrottle::GetNameForLogging() {
......
......@@ -6,6 +6,7 @@
#define CHROMECAST_BROWSER_WEBVIEW_WEBVIEW_NAVIGATION_THROTTLE_H_
#include "base/macros.h"
#include "base/memory/weak_ptr.h"
#include "base/sequenced_task_runner.h"
#include "chromecast/browser/webview/proto/webview.grpc.pb.h"
#include "content/public/browser/navigation_throttle.h"
......@@ -20,7 +21,7 @@ class WebviewController;
class WebviewNavigationThrottle : public content::NavigationThrottle {
public:
WebviewNavigationThrottle(content::NavigationHandle* handle,
WebviewController* controller);
base::WeakPtr<WebviewController> controller);
~WebviewNavigationThrottle() override;
......@@ -34,7 +35,7 @@ class WebviewNavigationThrottle : public content::NavigationThrottle {
private:
scoped_refptr<base::SequencedTaskRunner> response_task_runner_;
WebviewController* controller_;
base::WeakPtr<WebviewController> controller_;
DISALLOW_COPY_AND_ASSIGN(WebviewNavigationThrottle);
};
......
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