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

Fix navigation message relay. #20193

Merged
merged 2 commits into from
Aug 2, 2020
Merged

Fix navigation message relay. #20193

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f295b9
Fix navigation message relay.
mehmetf Aug 1, 2020
6ec4662
Check whether root isolate is running before sending navigation chann…
mehmetf Aug 1, 2020
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
3 changes: 2 additions & 1 deletion shell/common/engine.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,7 +303,8 @@ void Engine::DispatchPlatformMessage(fml::RefPtr<PlatformMessage> message) {
} else if (channel == kSettingsChannel) {
HandleSettingsPlatformMessage(message.get());
return;
} else if (channel == kNavigationChannel) {
} else if (!runtime_controller_->IsRootIsolateRunning() &&
channel == kNavigationChannel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not move it back to where it was before (after the runtime_controller handling at 313-316) rather than add additional conditionals which make assumptions about the flow? If this is simply to "undo" the mistake in the previous fix then it should conform to the code before the fix as much as possible.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, I looked at the first commit and see the problem with the std::move() now. Looking more at the flow to see how the new proposed logic applies...

Copy link
Contributor

Choose a reason for hiding this comment

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

It appears that the design issue here is that we have a method runtime_controller_->DispatchPlatformMessage that takes a shared pointer and may or may not consumer it as a result of its operation. We should not be giving it our only valid reference to that message if it may decide not to consume it.

Moving the most visible part of the conditional up to this else if clause solves only half of the problem because all we've really double checked is our half of the tests to see if the message is dispatchable by the runtime controller. The actual Dispatch method tests other conditions.

What about moving this message handling clause to after the runtime_controller gets a shot at it, but not using std::move so that we don't fumble the reference if the dispatch fails?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to fix this asap because it is a P0 regression so I wanted this revert to be as trivial as possible. I would like to revert the whole PR but then I would have to turn off the clang-tidy checks and I was trying to work around that.

I am not a C++ programmer. What would calling std::move twice on the same pointer do? I assume that's bad so I also assumed DispatchPlatformMessage rarely (if ever) returned false for a navigation message. Therefore, this new code is equivalent since HandleNavigationPlatformMessage only gets called if !runtime_controller_->IsRootIsolateRunning(); just like before.

What about moving this message handling clause to after the runtime_controller gets a shot at it, but not using std::move so that we don't fumble the reference if the dispatch fails?

I wasn't sure whether I can continue using the reference after std::move was called on it.

Copy link
Member

Choose a reason for hiding this comment

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

I think the logic in this PR is correct, and IMHO it's clearer than the previous implementation.

Most messages are passed to DispatchPlatformMessage, which relays the message to a handler implemented in Dart. However, there are some special message channels where the engine itself handles the message.

If the Dart isolate is not yet running, then the navigation channel will be processed within the engine so it can capture the initial route. I think it makes sense to separate that out as a special case alongside the other channels that are handled by the engine.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @jason-simmons . If you LGTM, I can submit :-)

Copy link
Contributor

Choose a reason for hiding this comment

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

That make sense, but there is another condition inside the Dispatch method which means it might not be dispatched anyway, so technically this new logic is not the same as before. I'll leave it to Jason to LGTM it.

Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand how Refs interact with std::move(), can't this also be solved by moving the code back to where it was and removing the std::move() from the call to DispatchPlatformMessage? I believe the move in that case is just an optimization for "I no longer need this reference any more so rather than ref-counting the handoff here, just take mine" and by removing the move() we add a simple refcount logic so that DPM can be as conditional as it wants as to whether it uses the reference without affecting the validity of the original reference held in this function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @flar. I think that's a more disruptive change (but could be the right change). Ideally, I can submit this to solve the immediate problem and someone else can take a second pass to fix it + add a test for it.

You are right that this is not exactly the same logic as before but as I explained, it comes pretty close without having to disable the clang-tidy checks. I agree that it is probably not the right (long-term) change.

// If there's no runtime_, we may still need to set the initial route.
HandleNavigationPlatformMessage(std::move(message));
return;
Expand Down