[wasm] Fix NRE caused by WeakReference collected on JS interop boundary. by pavelsavara · Pull Request #54453 · dotnet/runtime

pavelsavara · 2021-06-19T14:28:01Z

Added GCHandle Normal while JSObject is in flight over JS/Managed interop boundary
Added new unit test for iterator.

ghost · 2021-06-19T14:28:05Z

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

Issue Details

Demonstrates #53872 on simple case

Author:	pavelsavara
Assignees:	-
Labels:	`arch-wasm`, `area-System.Runtime.InteropServices.JavaScript`
Milestone:	-

pavelsavara · 2021-06-19T17:27:09Z

[15:39:23] fail: [FAIL] System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTests.Iterator
[15:39:23] info: System.Exception : Object reference not set to an instance of an object. At attempt=65516
[15:39:23] info: ---- System.NullReferenceException : Object reference not set to an instance of an object.
[15:39:23] info: at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTests.Iterator()
[15:39:23] info: --- End of stack trace from previous location ---
[15:39:23] info: ----- Inner Stack Trace -----
[15:39:23] info: at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTests.ToEnumerable(JSObject iterrator)+MoveNext()
[15:39:23] info: at System.Linq.Enumerable.Count[Object](IEnumerable`1 source)
[15:39:23] info: at System.Runtime.InteropServices.JavaScript.Tests.JavaScriptTests.Iterator()

See also
https://helixre8s23ayyeko0k025g8.blob.core.windows.net/dotnet-runtime-refs-pull-54453-merge-8d232ae9d0ad44ad84/System.Private.Runtime.InteropServices.JavaScript.Tests/console.24bbb537.log?sv=2019-07-07&se=2021-07-09T15%3A26%3A50Z&sr=c&sp=rl&sig=LIMNicCFVPwVfiBNZhgnApW0L%2BFV0NziNhfxzLF5afE%3D

pavelsavara · 2021-06-21T16:09:17Z

Open questions

why original requirements specify ownsHandle == true -> Weak ? [browser][wasm] Implement JavaScript managed object reference counting #37417 is that mistake ?
What behavior depends on the original/specified implementation ?
Would this change lead to handles leaking at places which do not Dispose() it explicitly ?

Alternative implementation, pass false from mono_wasm_register_obj

kg · 2021-06-21T16:14:19Z

I agree that the ownsHandle logic looks wrong, at least based on convention where an ownsHandle=true usually means 'this new object owns the underlying handle'.

pavelsavara · 2021-06-21T16:17:55Z

I just found this comment in the original PR

/// <param name="ownsHandle">Whether or not the handle is owned by the clr or not.</param>

kg · 2021-06-21T16:19:50Z

That doesn't really clarify it at all :-)

pavelsavara · 2021-07-02T12:25:54Z

Note, we could not use the counter on the SafeHandle base class, because it could not be increased after the handle was released once. The same instance of JSObject could be passed from JS space multiple times, even on multiple calls.

pavelsavara · 2021-07-02T12:33:27Z

This doesn't address potential issue with naked pointers together with compacting GC. I have not seen that to manifest yet.
I drafted code for adding in-flight objects here pavelsavara@200ac14.
But I believe that proper solution is to pass GCHandles everywhere, not pointers nor GC roots .

lewing

So basically *InFlight is a strong ref held on top of the weak one across the transitions where we need to keep a strong ref?

pavelsavara · 2021-07-12T17:39:06Z

So basically *InFlight is a strong ref held on top of the weak one across the transitions where we need to keep a strong ref?

Exactly. We keep the weak ref in place, because it's handle id has longer life-cycle.

kg

I only see style issues

pavelsavara · 2021-07-14T08:44:42Z

Failure of Http2_MultipleConnectionsEnabled_InfiniteRequestsCompletelyBlockOneConnection_RemaningRequestsAreHandledByNewConnection is on alpine, not related to browser only change.

lewing · 2021-07-14T15:00:49Z

ci failure looks like #45204 which has been disabled now

lewing · 2021-07-14T15:01:34Z

@pavelsavara can you resolve the conflict

…t over the managed/JS boundary

pavelsavara · 2021-07-14T15:24:18Z

Rebased, let's see how it works together with rooting. It should be just fine.

lewing · 2021-07-14T19:07:19Z

two hits on #55536

lewing · 2021-07-15T16:36:12Z

runtime (Libraries Test Run release mono Linux x64 Debug) is

/root/helix/work/workitem /root/helix/work/workitem
Discovering: System.Net.Quic.Functional.Tests (method display = ClassAndMethod, method display options = None)
Discovered: System.Net.Quic.Functional.Tests (found 102 of 117 test cases)
Starting: System.Net.Quic.Functional.Tests (parallel test collections = on, max threads = 2)
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:02:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:02:16
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:04:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:04:16
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:06:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:06:16
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:08:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:08:16
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:10:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:10:16
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:12:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:12:16
System.Net.Quic.Functional.Tests: [Long Running Test] 'System.Net.Quic.Tests.QuicStreamTests_MsQuicProvider.ReadWrite_Random_Success', Elapsed: 00:14:11
[Long Running Test] 'System.Net.Quic.Tests.MsQuicQuicStreamConformanceTests.ReadWrite_Success', Elapsed: 00:14:16
...
[EXECUTION TIMED OUT]
Exit Code:-3Executor timed out after 900 seconds and was killed
['System.Net.Quic.Functional.Tests' END OF WORK ITEM LOG: Command timed out, and was killed]

which sounds like #55642

lewing · 2021-07-15T16:43:05Z

runtime (Mono llvmaot Pri0 Runtime Tests Run Linux arm64 release) is

Traceback (most recent call last):
File "/root/helix/work/correlation/reporter/run.py", line 13, in <module>
from azure_devops_result_publisher import AzureDevOpsTestResultPublisher
File "/root/helix/work/correlation/reporter/azure_devops_result_publisher.py", line 7, in <module>
from azure.devops.connection import Connection
ModuleNotFoundError: No module named 'azure'
+ export _uploaderExitCode=1
+ date -u +%FT%TZ
2021-07-14T17:05:31Z
+ export PYTHONPATH=
+ exit 1

lewing · 2021-07-15T16:43:46Z

/azp run

azure-pipelines · 2021-07-15T16:43:51Z

You have several pipelines (over 10) configured to build pull requests in this repository. Specify which pipelines you would like to run by using /azp run [pipelines] command. You can specify multiple pipelines using a comma separated list.

lewing · 2021-07-15T17:23:18Z

it hasn't restarted in 40 minutes and non of the failures were relevant to a browser only change.

pavelsavara added arch-wasm WebAssembly architecture area-System.Runtime.InteropServices.JavaScript labels Jun 19, 2021

pavelsavara requested review from kg and lewing June 19, 2021 14:28

pavelsavara changed the title ~~[wasm]New unit test for JS iterator~~ [wasm] Fix NRE caused by WeakReference collected on JS interop boundary.Jun 21, 2021

pavelsavara marked this pull request as ready for review June 21, 2021 15:04

pavelsavara requested a review from marek-safar June 21, 2021 15:06

pavelsavara marked this pull request as draft June 22, 2021 08:29

pavelsavara added the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jun 25, 2021

pavelsavara force-pushed the wasm_iterrator_NRE branch 5 times, most recently from fcbb9ab to dc2108fCompare July 2, 2021 12:07

pavelsavara removed the NO-MERGE The PR is not ready for merge yet (see discussion for detailed reasons) label Jul 2, 2021

pavelsavara marked this pull request as ready for review July 2, 2021 12:27

pavelsavara force-pushed the wasm_iterrator_NRE branch from dc2108f to b799aaaCompare July 12, 2021 10:17

lewing reviewed Jul 12, 2021
View reviewed changes

runfoappBot mentioned this pull request Jul 13, 2021
system.linq.expressions.tests work item #55546
Closed

kg approved these changes Jul 13, 2021
View reviewed changes

pavelsavara requested a review from lewing July 13, 2021 16:42

inFlight ref-counting of strong GCHandle for JSObject, while in fligh…
344041d
…t over the managed/JS boundary

pavelsavara force-pushed the wasm_iterrator_NRE branch from ac1868e to 344041dCompare July 14, 2021 15:22

lewing merged commit 04e6dcc into dotnet:mainJul 15, 2021

radical mentioned this pull request Jul 19, 2021
[wasm] SendReceive_VaryingLengthBuffers_Success failing in CI #53957
Closed

ManickaP mentioned this pull request Jul 20, 2021
[QUIC] Remove AppContext switch from S.N.Quic #56027
Merged

pavelsavara mentioned this pull request Jul 26, 2021
[wasm] JSObject parameters of callback from JS to managed are not released and stay inFlight #56303
Closed

This was referenced Jul 28, 2021
[WinHttpHandler] Long running test: ResponseHeadersRead_SynchronizationContextNotUsedByHandler on Win7/Win8 #54034
Closed
MsQuic tests hang / long running #56487
Closed

pavelsavara deleted the wasm_iterrator_NRE branch July 29, 2021 08:44

ghost locked as resolved and limited conversation to collaborators Aug 28, 2021

Uh oh!

Conversation

pavelsavara commented Jun 19, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Jun 19, 2021

Uh oh!

pavelsavara commented Jun 19, 2021

Uh oh!

pavelsavara commented Jun 21, 2021

Uh oh!

kg commented Jun 21, 2021

Uh oh!

pavelsavara commented Jun 21, 2021

Uh oh!

kg commented Jun 21, 2021

Uh oh!

pavelsavara commented Jul 2, 2021

Uh oh!

pavelsavara commented Jul 2, 2021

Uh oh!

lewing left a comment

Choose a reason for hiding this comment

Uh oh!

pavelsavara commented Jul 12, 2021

Uh oh!

kg left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pavelsavara commented Jul 14, 2021

Uh oh!

lewing commented Jul 14, 2021

Uh oh!

lewing commented Jul 14, 2021

Uh oh!

pavelsavara commented Jul 14, 2021

Uh oh!

lewing commented Jul 14, 2021

Uh oh!

lewing commented Jul 15, 2021• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lewing commented Jul 15, 2021

Uh oh!

lewing commented Jul 15, 2021

Uh oh!

azure-pipelinesBot commented Jul 15, 2021

Uh oh!

lewing commented Jul 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

pavelsavara commented Jun 19, 2021•
edited
Loading

lewing commented Jul 15, 2021•
edited
Loading