search

microsoft / react-native-windows

A framework for building native Windows apps with React.
https://microsoft.github.io/react-native-windows/
Other
16.27k stars 1.14k forks source link

Fix and reenable Facebook's MapBuffer code #8000

Open dannyvv opened 3 years ago

dannyvv commented 3 years ago

The MapBuffer class has a double-free bug in the code that makes the unittests crash.

For now we have disabled the unittests (See .ado\jobs\desktop.yml).

The problem is that MapBufferBuilder in the build() method stack allocates the class that does not use proper std:: pointer semantics... So the MapBuffer class is destructed at the end of the method which delete's it's member _data.

The test code just uses the stack allocated value tests it and at the end of the test method i.e. testSimpleIntMap the destructor is called again double-freeing the _data member.

There is also a crash hiding where the v8 isolate is disposed on the wrong thread... Perhaps this is caused by the crash in the MapBuffer object... Not sure. Should be investigated before reenabeling this test though...

chrisglein commented 3 years ago

@dannyvv Does MapBuffer need to be enabled or just its unit tests? I wasn't clear based on reading this. Is MapBuilder a part of folly?

ghost commented 3 years ago

This issue has been automatically marked as stale because it has been marked as requiring author feedback but has not had any activity for 7 days. It will be closed if no further activity occurs within 14 days of this comment.

jonthysell commented 2 years ago

Reopening as this bug still being used to justify not running the ReactCommon.UnitTests in main/0.67.

NickGerleman commented 2 years ago

@JunielKatarn added the "ReactNative.UnitTests" project, but it's effectively Facebook's UTs for Fabric code.

chrisglein commented 2 years ago

May have been caught by AppVerifier, related issue: #6335. Goal here is to move from the whole suite being turned off to disabling this test to fixing and reenabling the test.

chrisglein commented 2 years ago

The latest integration had a payload for MapBuffer. We also should just try running these tests again and see if the error is stale (compared to July of last yera)