Skip to content

Fix flutter tool crash on upgrade caused by app-jit #111879

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 20, 2022

Conversation

jensjoha
Copy link
Contributor

@jensjoha jensjoha commented Sep 19, 2022

In #111459 we started using app-jit for flutter_tool to improve startup performance. In the PR there was talks about it having been tried before, but been reverted for some reason no one remembered.

I digged a bit and found the revert #30204 which pointed me to #30203 saying that flutter upgrade crashed.

And sure enough that is still the case (at least on Linux):

Building flutter tool...

===== CRASH =====
si_signo=Bus error(7), si_code=2, si_addr=0x7f8ac18b6210
version=2.19.0-215.0.dev (dev) (Sun Sep 18 20:34:31 2022 -0700) on "linux_x64"
pid=156885, thread=157284, isolate_group=main(0x56110610a000), isolate=main(0x561106164800)
os=linux, arch=x64, comp=no, sim=no
isolate_instructions=7f8ac1783000, vm_instructions=56110306f160
  pc 0x00007f8ac18b6210 fp 0x00007f8abddfe4b0 Unknown symbol
  pc 0x00005611031ea73d fp 0x00007f8abddfe550 dart::DartEntry::InvokeCode(dart::Code const&, unsigned long, dart::Array const&, dart::Array const&, dart::Thread*)+0x14d
  pc 0x00005611031ea58c fp 0x00007f8abddfe5b0 dart::DartEntry::InvokeFunction(dart::Function const&, dart::Array const&, dart::Array const&, unsigned long)+0x14c
  pc 0x00005611031ec9ac fp 0x00007f8abddfe600 dart::DartLibraryCalls::HandleMessage(long, dart::Instance const&)+0x14c
  pc 0x0000561103212910 fp 0x00007f8abddfeba0 dart::IsolateMessageHandler::HandleMessage(std::__2::unique_ptr<dart::Message, std::__2::default_delete<dart::Message> >)+0x350
  pc 0x000056110323b22d fp 0x00007f8abddfec10 dart::MessageHandler::HandleMessages(dart::MonitorLocker*, bool, bool)+0x14d
  pc 0x000056110323b90f fp 0x00007f8abddfec60 dart::MessageHandler::TaskCallback()+0x1df
  pc 0x0000561103363f58 fp 0x00007f8abddfece0 dart::ThreadPool::WorkerLoop(dart::ThreadPool::Worker*)+0x148
  pc 0x00005611033643ad fp 0x00007f8abddfed10 dart::ThreadPool::Worker::Main(unsigned long)+0x6d
  pc 0x00005611032d8178 fp 0x00007f8abddfedd0 /path/to/flutter/bin/cache/dart-sdk/bin/dart+0x217f178
-- End of DumpStackTrace
Aborted

What's happening is this:

  1. We run flutter upgrade
  2. It goes to a new commit via git
  3. It then launches another instance of flutter. Notice how this is after git has fetched the new data, so a new tool snapshot is built; also notice how the "original" ("top-most" if you will) flutter is still running.
  4. Dart - when running from an app-jit snapshot - memory maps the snapshot. Step 3 changed the snapshot by replacing the content of the file. Now when it tries to read more it either reads what is essentially "garbage" for this process (maybe a new snapshot version, maybe another place, who knows) and crashes.

This PR fixes the issue by applying the same strategy as is applied for the dart_sdk path when upgrading dart: nothing is overwritten, it's moved and then later deleted. (technically just deleting it seems to work too, but if - for whatever reason - the file should be in use we don't want it to fail).

This way the data dart is currently using to run the "top most" flutter still exists and the process doesn't crash.

@flutter-dashboard
Copy link

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 (don't just cc him here, he won't see it! He's on Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

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

@jonahwilliams
Copy link
Member

I like this approach but CI does not :)

@jensjoha
Copy link
Contributor Author

Interesting. It works on my Windows box --- I'll try some print-debugging on the bot =)

@jensjoha
Copy link
Contributor Author

I think we're good now.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@christopherfujino christopherfujino left a comment

Choose a reason for hiding this comment

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

This works for me on Linux & Windows, so LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants