Fix navigation message relay.

[mehmetf]

Partially reverts the change done in #19981 to make sure back button works on Android. There's a problem with std::move which already existed in the old code if DispatchPlatformMessage returns false. Fixing that is out of scope for this PR. It would also be good to add tests for navigation channel.

[flutter-dashboard]

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact Hixie on the #hackers channel in Chat.

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

[mehmetf]

Fixing that is out of scope for this PR.

Apparently it has to be in scope since clang-tidy is not letting me submit this. I'll attempt.

[mehmetf]

I think this should make clang-tidy happy and match the old behavior.

[flar]

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.

[flar]

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...

[flar]

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?

[mehmetf]

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.

[jason-simmons]

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.

[mehmetf]

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

[flar]

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.

[flar]

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.

[mehmetf]

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.

[gaaclarke]

Man, that was nasty. The fix looks good to me. It's a bit concerning because there is still no test that asserts that it is correct.

I knew i was accepting a change in order with the memory fix but didn't consider that the logic was built around the assumption that a bool returning function always returns one value... nasty.

I can take a look into adding a test later today. Thanks @mehmetf.