Skip to content
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

[hot_reload] Calling a method for the first time after an update crashes the app #57643

Closed
pavelsavara opened this issue Aug 18, 2021 · 8 comments · Fixed by #57798 or #57799
Closed

[hot_reload] Calling a method for the first time after an update crashes the app #57643

pavelsavara opened this issue Aug 18, 2021 · 8 comments · Fixed by #57798 or #57799
Assignees
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Milestone

Comments

@pavelsavara
Copy link
Member

pavelsavara commented Aug 18, 2021

If a hot reload delta is applied to a method M that has never been called before, and then that method is called for the first time after the update, the runtime will assert.

A common variant of this is doing a browser refresh of a Blazor WebAssembly app that had some changes applied. When the runtime restarts, dotnet watch replays the deltas on a fresh runtime, and after that calling any of the updated methods will cause a crash.


Original report:

with dotnet.6.0.0-rc.1.21411.2.js

@pavelsavara pavelsavara added the arch-wasm WebAssembly architecture label Aug 18, 2021
@ghost
Copy link

ghost commented Aug 18, 2021

Tagging subscribers to 'arch-wasm': @lewing
See info in area-owners.md if you want to be subscribed.

Issue Details

with dotnet.6.0.0-rc.1.21411.2.js

Author: pavelsavara
Assignees: -
Labels:

arch-wasm

Milestone: -

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 18, 2021
@lambdageek lambdageek added the area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc label Aug 18, 2021
@lambdageek lambdageek added this to the 6.0.0 milestone Aug 18, 2021
@lambdageek lambdageek self-assigned this Aug 18, 2021
@jeffschwMSFT jeffschwMSFT removed the untriaged New issue has not been triaged by the area owner label Aug 18, 2021
@lambdageek
Copy link
Member

The interesting thing about "ctrl-R" is that it is like we're in a cold-start situation.

We start the runtime. Immediately apply an update to a method we never called before. We indirectly try to call it.

So we're on some "let's interpret this method for the first time" path.

@lambdageek lambdageek changed the title [wasm][hot-reload] * Assertion at /__w/1/s/src/mono/mono/component/hot_reload.c:869, condition `<disabled>' not met [hot_reload] Calling a method for the first time after an update crashes the app Aug 18, 2021
@lambdageek lambdageek removed the arch-wasm WebAssembly architecture label Aug 18, 2021
@lambdageek
Copy link
Member

Some assertions that may be hit:

  • [wasm][hot-reload] * Assertion at /__w/1/s/src/mono/mono/component/hot_reload.c:869, condition ' not met`
  • * Assertion at /Users/alklig/work/dotnet-runtime/runtime/src/mono/mono/metadata/metadata.c:1321, condition idx >= 0' not met`

Can happen in the "src/mono/samples/mbr/console" sample, too - just change the main Program.cs code to apply the update before calling the test method.

@lambdageek
Copy link
Member

FYI @danroth27

@pavelsavara
Copy link
Member Author

When you are talking about applying deltas, It seems to me that we expect that the state of the browser is in sync with state of the server ? I guess I could have many browser tabs of different age-of-last-reload.

@lambdageek
Copy link
Member

When you are talking about applying deltas, It seems to me that we expect that the state of the browser is in sync with state of the server ? I guess I could have many browser tabs of different age-of-last-reload.

I believe dotnet watch handles that - for each websocket session they keep track of what's been sent over.

lambdageek added a commit to lambdageek/runtime that referenced this issue Aug 20, 2021
The issue is that the ParamList column in EnC deltas is a "suppressed column"
that has the value 0.  So when a method is updated if we use the value
directly, we will break, for example - `mono_metadata_get_param_attrs` which
expects a non-zero index in that column.

CoreCLR solves this by having a set of suppressed columns that are never
updated by deltas.  (CoreCLR's model is to directly mutate the tables of the
baseline image).  In Mono we can eventually do the same thing by writing the
value from the previous generation into the current delta's row.  But right now
since we don't allow parameter modifications, and the only column on a Method
table that we allow to be modified is the RVA - which we look up specially - we
can just always return the baseline image row for the method table.

Fixes dotnet#57643
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 20, 2021
github-actions bot pushed a commit that referenced this issue Aug 20, 2021
The issue is that the ParamList column in EnC deltas is a "suppressed column"
that has the value 0.  So when a method is updated if we use the value
directly, we will break, for example - `mono_metadata_get_param_attrs` which
expects a non-zero index in that column.

CoreCLR solves this by having a set of suppressed columns that are never
updated by deltas.  (CoreCLR's model is to directly mutate the tables of the
baseline image).  In Mono we can eventually do the same thing by writing the
value from the previous generation into the current delta's row.  But right now
since we don't allow parameter modifications, and the only column on a Method
table that we allow to be modified is the RVA - which we look up specially - we
can just always return the baseline image row for the method table.

Fixes #57643
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 20, 2021
@lambdageek
Copy link
Member

Keeping open until the .NET 6 RC1 backport goes in

@lambdageek lambdageek reopened this Aug 20, 2021
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Aug 20, 2021
ericstj pushed a commit that referenced this issue Aug 20, 2021
…57799)

* Add test case

Call a method for the first time after an update has been applied to it.

This will check that the interpreter or JIT does not have to rely on cached
information from the baseline (about the method signature, for example) and
that it can compute it from the delta.

* [hot_reload] Don't look at delta method table rows

The issue is that the ParamList column in EnC deltas is a "suppressed column"
that has the value 0.  So when a method is updated if we use the value
directly, we will break, for example - `mono_metadata_get_param_attrs` which
expects a non-zero index in that column.

CoreCLR solves this by having a set of suppressed columns that are never
updated by deltas.  (CoreCLR's model is to directly mutate the tables of the
baseline image).  In Mono we can eventually do the same thing by writing the
value from the previous generation into the current delta's row.  But right now
since we don't allow parameter modifications, and the only column on a Method
table that we allow to be modified is the RVA - which we look up specially - we
can just always return the baseline image row for the method table.

Fixes #57643

Co-authored-by: Aleksey Kliger <alklig@microsoft.com>
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Aug 20, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Sep 26, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-EnC-mono Hot Reload for WebAssembly, iOS/Android, etc
Projects
None yet
3 participants