Skip to content
This repository was archived by the owner on Feb 25, 2025. It is now read-only.

[fuchsia] Add more logging for error cases. #29397

Merged
merged 1 commit into from
Nov 4, 2021
Merged

[fuchsia] Add more logging for error cases. #29397

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions shell/platform/fuchsia/flutter/component_v1.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,11 @@ void ComponentV1::OnEngineTerminate(const Engine* shell_holder) {
});

if (found == shell_holders_.end()) {
// This indicates a deeper issue with memory management and should never
// happen.
FML_LOG(ERROR) << "Tried to terminate an unregistered shell holder.";
Copy link
Contributor

@arbreng arbreng Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can just CHECK here, this should never happen and indicates a deeper problem like a memory corruption

Same thing for the V2 one below

Copy link
Contributor Author

@akbiggs akbiggs Oct 29, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DCHECK'd. Let me know if you'd prefer I upgrade it to a real CHECK.

Copy link
Member

@chinmaygarde chinmaygarde Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, you can pipe messages to checks directly instead of logging before the assertion. So FML_DCHECK(foo) << "The message";.

Copy link
Contributor Author

@akbiggs akbiggs Nov 4, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I didn't do this because when I looked at the implementation of FML_DCHECK, it did not look like the log would happen on release builds, only debug builds.

FML_DCHECK(false);

return;
}

Expand All @@ -493,11 +498,15 @@ void ComponentV1::OnEngineTerminate(const Engine* shell_holder) {
auto return_code = shell_holder->GetEngineReturnCode();
if (return_code.has_value()) {
last_return_code_ = {true, return_code.value()};
} else {
FML_LOG(ERROR) << "Failed to get return code from terminated shell holder.";
}

shell_holders_.erase(found);

if (shell_holders_.size() == 0) {
FML_VLOG(-1) << "Killing component because all shell holders have been "
"terminated.";
Kill();
// WARNING: Don't do anything past this point because the delegate may have
// collected this instance via the termination callback.
Expand Down
9 changes: 9 additions & 0 deletions shell/platform/fuchsia/flutter/component_v2.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -526,6 +526,11 @@ void ComponentV2::OnEngineTerminate(const Engine* shell_holder) {
});

if (found == shell_holders_.end()) {
// This indicates a deeper issue with memory management and should never
// happen.
FML_LOG(ERROR) << "Tried to terminate an unregistered shell holder.";
FML_DCHECK(false);

return;
}

Expand All @@ -535,11 +540,15 @@ void ComponentV2::OnEngineTerminate(const Engine* shell_holder) {
auto return_code = shell_holder->GetEngineReturnCode();
if (return_code.has_value()) {
last_return_code_ = {true, return_code.value()};
} else {
FML_LOG(ERROR) << "Failed to get return code from terminated shell holder.";
}

shell_holders_.erase(found);

if (shell_holders_.size() == 0) {
FML_VLOG(-1) << "Killing component because all shell holders have been "
"terminated.";
Kill();
// WARNING: Don't do anything past this point because the delegate may have
// collected this instance via the termination callback.
Expand Down
4 changes: 4 additions & 0 deletions shell/platform/fuchsia/flutter/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,7 @@ void Engine::Initialize(
weak = weak_factory_.GetWeakPtr()]() {
task_runner->PostTask([weak]() {
if (weak) {
FML_LOG(ERROR) << "Terminating from session_error_callback";
weak->Terminate();
}
});
Expand Down Expand Up @@ -339,6 +340,8 @@ void Engine::Initialize(
weak = weak_factory_.GetWeakPtr()]() {
task_runner->PostTask([weak]() {
if (weak) {
FML_LOG(ERROR) << "Terminating from "
"on_session_listener_error_callback";
weak->Terminate();
}
});
Expand Down Expand Up @@ -615,6 +618,7 @@ void Engine::Initialize(
// The engine could have been killed by the caller right after the
// constructor was called but before it could run on the UI thread.
if (weak) {
FML_LOG(ERROR) << "Terminating from on_run_failure";
weak->Terminate();
}
};
Expand Down
12 changes: 7 additions & 5 deletions shell/platform/fuchsia/flutter/gfx_session_connection.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,12 +4,11 @@

#include "gfx_session_connection.h"

#include <lib/async/cpp/time.h>
#include <lib/async/default.h>

#include <lib/async/cpp/task.h>
#include <lib/async/cpp/time.h>
#include <lib/async/default.h>
#include <lib/fit/function.h>
#include <zircon/status.h>

#include "flutter/fml/make_copyable.h"
#include "flutter/fml/time/time_point.h"
Expand Down Expand Up @@ -222,8 +221,11 @@ GfxSessionConnection::GfxSessionConnection(

next_presentation_info_.set_presentation_time(0);

session_wrapper_.set_error_handler(
[callback = session_error_callback](zx_status_t status) { callback(); });
session_wrapper_.set_error_handler([callback = session_error_callback](
zx_status_t status) {
FML_LOG(ERROR) << "scenic::Session error: " << zx_status_get_string(status);
callback();
});

// Set the |fuchsia::ui::scenic::OnFramePresented()| event handler that will
// fire every time a set of one or more frames is presented.
Expand Down