Skip to content

Commit 96ec5f1

Browse files
ishikawaShevO27
authored andcommitted
Use Map instead of object in BlobRegistry (facebook#39528)
Summary: issue: facebook#39441 For the following reasons, I have replaced an object used for id management inside BlobRegistry with `Map`. - The polyfill used for `fetch`, [whatwg-fetch](https://github.com/JakeChampion/fetch), returns responses as `Blob` objects. - When a `Blob` is created, it is registered with blobID in the [BlobRegistry](https://github.com/facebook/react-native/blob/main/packages/react-native/Libraries/Blob/BlobRegistry.js), which is not automatically released. - This issue was previously reported in facebook#19248 and was fixed by modifying whatwg-fetch. However, with the implementation of automatic garbage collection in facebook#24405, the implementation was reverted in commit bccc92d, returning to the original behavior. - Although facebook#24405 enables `Blob` objects to be garbage collected, the Blob IDs registered in the BlobRegistry remain, causing the count to increase each time `fetch` is called. - As a result, the `Property storage exceeds 196607 properties` error occurs To address this issue, I have modified the implementation of `BlobRegistry` to use a `Map` instead of an object. By using a `Map`, there is no limit to the number of entries. ## Changelog: [Internal] - [Fixed] - Fixed a bug that caused a "Property storage exceeds 196607 properties" error when sending a certain number of `fetch` requests. Pull Request resolved: facebook#39528 Test Plan: I've added a new tests in `packages/react-native/Libraries/Blob/__tests__/BlobRegistry-test.js` and confirmed the test pass before and after changes. ``` $ yarn run test ... Test Suites: 1 skipped, 219 passed, 219 of 220 total Tests: 2 skipped, 4017 passed, 4019 total Snapshots: 1154 passed, 1154 total Time: 10.525 s Ran all test suites. ✨ Done in 12.52s. ``` Reviewed By: javache Differential Revision: D49423213 Pulled By: NickGerleman fbshipit-source-id: d5f73d7f5e34d1d2c3969b7dfbc45d3e6196aa30
1 parent 4fe01e6 commit 96ec5f1

File tree

2 files changed

+53
-9
lines changed

2 files changed

+53
-9
lines changed

packages/react-native/Libraries/Blob/BlobRegistry.js

Lines changed: 14 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -8,27 +8,32 @@
88
* @format
99
*/
1010

11-
const registry: {[key: string]: number, ...} = {};
11+
const registry: Map<string, number> = new Map();
1212

1313
const register = (id: string) => {
14-
if (registry[id]) {
15-
registry[id]++;
14+
const used = registry.get(id);
15+
16+
if (used != null) {
17+
registry.set(id, used + 1);
1618
} else {
17-
registry[id] = 1;
19+
registry.set(id, 1);
1820
}
1921
};
2022

2123
const unregister = (id: string) => {
22-
if (registry[id]) {
23-
registry[id]--;
24-
if (registry[id] <= 0) {
25-
delete registry[id];
24+
const used = registry.get(id);
25+
26+
if (used != null) {
27+
if (used <= 1) {
28+
registry.delete(id);
29+
} else {
30+
registry.set(id, used - 1);
2631
}
2732
}
2833
};
2934

3035
const has = (id: string): number | boolean => {
31-
return registry[id] && registry[id] > 0;
36+
return registry.get(id) || false;
3237
};
3338

3439
module.exports = {
Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
'use strict';
2+
3+
const BlobRegistry = require('../BlobRegistry');
4+
5+
describe('BlobRegistry', () => {
6+
describe('register', () => {
7+
it('does not throw error', () => {
8+
expect(() => BlobRegistry.register('id1')).not.toThrowError();
9+
});
10+
11+
it('registers new id', () => {
12+
BlobRegistry.register('id2');
13+
expect(BlobRegistry.has('id2')).toBeTruthy();
14+
});
15+
});
16+
17+
describe('unregister', () => {
18+
it('does not throw error', () => {
19+
expect(() => BlobRegistry.unregister('id3')).not.toThrowError();
20+
});
21+
22+
it('remove registered id', () => {
23+
BlobRegistry.register('id4');
24+
BlobRegistry.unregister('id4');
25+
expect(BlobRegistry.has('id4')).toBeFalsy();
26+
});
27+
});
28+
29+
describe('has', () => {
30+
it('returns true for registered id', () => {
31+
BlobRegistry.register('id5');
32+
expect(BlobRegistry.has('id5')).toBeTruthy();
33+
});
34+
35+
it('returns false for unregistered id', () => {
36+
expect(BlobRegistry.has('id6')).toBeFalsy();
37+
});
38+
});
39+
});

0 commit comments

Comments
 (0)