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

fix: fix the race condition when calling readAsDataURL after new Blob(blobs) #34096

Closed

Conversation

wood1986
Copy link
Contributor

@wood1986 wood1986 commented Jun 29, 2022

Summary

async () => {
  let blobs = [];
  for (let i = 0; i < 4; i++) {
    const res = await fetch();
    blobs = [...blobs, await res.blob()] 
  }
  const blob = new Blob(blobs); // <<<<<<<<<<<<<<< a
  return await new Promise((resolve, reject) => {
    const fileReader = new FileReader();
    fileReader.onload = async () => {
      await RNFS.writeFile(destPath, (fileReader.result as string).split(',')[1], 'base64');
      resolve(destPath);
    };
    fileReader.onabort = () => {
      reject('');
    };
    fileReader.onerror = (event) => {
      reject('');
    };
    fileReader.readAsDataURL(blob); // <<<<<<<<<<<<<<< b
  });
}

Sometime fileReader.readAsDataURL is unable to get blob from the dictionary after new Blob(blobs) and then reject with Unable to resolve data for blob: blobId in iOS or The specified blob is invalid in android. Because line a and b can be run in different thread. new Blob([]) is in progress and fileReader.readAsDataURL accesses the blob dictionary ahead of the blob creation.

The expected behaviour is it should finish new Blob([]) first and then readAsDataURL(blob)

To fix that, there should be a lock inside the method createFromParts. For iOS, It needs to be a recursive_mutex to allow same thread to acquire lock

Changelog

[iOS] [Fixed] - fix the race condition when calling readAsDataURL after new Blob(blobs)

Test Plan

@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jun 29, 2022
@react-native-bot react-native-bot added Bug Platform: iOS iOS applications. labels Jun 29, 2022
@facebook-github-bot
Copy link
Contributor

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@analysis-bot
Copy link

analysis-bot commented Jun 29, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,830,102 +0
android hermes armeabi-v7a 7,216,505 +0
android hermes x86 8,139,009 +0
android hermes x86_64 8,120,768 +0
android jsc arm64-v8a 9,695,700 +0
android jsc armeabi-v7a 8,451,163 +0
android jsc x86 9,646,317 +0
android jsc x86_64 10,244,130 +0

Base commit: 4d62e0d
Branch: main

@analysis-bot
Copy link

analysis-bot commented Jun 29, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 4d62e0d
Branch: main

@javache
Copy link
Member

javache commented Jun 29, 2022

Because line a and b can be run in different thread.

All JS runs on a single thread in React Native. While your fix may be correct, this does not seem like the right root cause of the issue.

Digging into the issue a bit more, I think I understand that on iOS there's no guarantee of ordering between RCTFileReaderModule and RCTBlobManager, which could cause a potential race. Using a recursive_mutex for this is not very elegant though, as there's not really any nested calls. Perhaps you could resolve this by changing RCTFileReaderModule to do in readAsDataURL: dispatch_async(blobManager.methodQueue, ^{ ... })

On Android, all Native Modules run on a shared background thread, so I'm not sure what the issue is there.

@wood1986 wood1986 force-pushed the fix-new-blob-fileReader-readAsDataURL branch from 099c5fb to 0e7e848 Compare June 29, 2022 18:23
@wood1986
Copy link
Contributor Author

wood1986 commented Jun 29, 2022

Thanks @javache

Few things

All JS runs on a single thread in React Native. While your fix may be correct, this does not seem like the right root cause of the issue.

Yes you are right. I know JS side is single thread. I was talking the native side.

Digging into the issue a bit more, I think I understand that on iOS there's no guarantee of ordering between RCTFileReaderModule and RCTBlobManager, which could cause a potential race. Using a recursive_mutex for this is not very elegant though, as there's not really any nested calls. Perhaps you could resolve this by changing RCTFileReaderModule to do in readAsDataURL: dispatch_async(blobManager.methodQueue, ^{ ... })

I fixed and tested. It works

On Android, all Native Modules run on a shared background thread, so I'm not sure what the issue is there.

Sorry for the confusion. I have not seen Android has the race condition but I suspect I might have. For the consistent reason, I made android same as iOS

@facebook-github-bot
Copy link
Contributor

@javache has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @wood1986 in bd12e41.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jun 30, 2022
@wood1986
Copy link
Contributor Author

wood1986 commented Jun 30, 2022

Hey @javache is it possible to include this fix in 0.69.2?

@javache
Copy link
Member

javache commented Jul 4, 2022

Hey @javache is it possible to include this fix in 0.69.2?

Please raise this in reactwg/react-native-releases#24

kelset pushed a commit that referenced this pull request Jul 12, 2022
…(blobs) (#34096)

Summary:
```js
async () => {
  let blobs = [];
  for (let i = 0; i < 4; i++) {
    const res = await fetch();
    blobs = [...blobs, await res.blob()]
  }
  const blob = new Blob(blobs); // <<<<<<<<<<<<<<< a
  return await new Promise((resolve, reject) => {
    const fileReader = new FileReader();
    fileReader.onload = async () => {
      await RNFS.writeFile(destPath, (fileReader.result as string).split(',')[1], 'base64');
      resolve(destPath);
    };
    fileReader.onabort = () => {
      reject('');
    };
    fileReader.onerror = (event) => {
      reject('');
    };
    fileReader.readAsDataURL(blob); // <<<<<<<<<<<<<<< b
  });
}
```

Sometime `fileReader.readAsDataURL` is unable to get blob from the dictionary after `new Blob(blobs)` and then reject with `Unable to resolve data for blob: blobId` in iOS or `The specified blob is invalid` in android. Because line `a` and `b` can be run in different thread. `new Blob([])` is in progress and `fileReader.readAsDataURL` accesses the blob dictionary ahead of the blob creation.

The expected behaviour is it should finish new Blob([]) first and then readAsDataURL(blob)

To fix that, there should be a lock inside the method `createFromParts`. For iOS, It needs to be a recursive_mutex to allow same thread to acquire lock

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - fix the race condition when calling readAsDataURL after new Blob(blobs)

Pull Request resolved: #34096

Reviewed By: cipolleschi

Differential Revision: D37514981

Pulled By: javache

fbshipit-source-id: 4bf84ece99871276ecaa5aa1849b9145ff44dbf4
Kudo pushed a commit to expo/react-native that referenced this pull request Jul 21, 2022
…(blobs) (facebook#34096)

Summary:
```js
async () => {
  let blobs = [];
  for (let i = 0; i < 4; i++) {
    const res = await fetch();
    blobs = [...blobs, await res.blob()]
  }
  const blob = new Blob(blobs); // <<<<<<<<<<<<<<< a
  return await new Promise((resolve, reject) => {
    const fileReader = new FileReader();
    fileReader.onload = async () => {
      await RNFS.writeFile(destPath, (fileReader.result as string).split(',')[1], 'base64');
      resolve(destPath);
    };
    fileReader.onabort = () => {
      reject('');
    };
    fileReader.onerror = (event) => {
      reject('');
    };
    fileReader.readAsDataURL(blob); // <<<<<<<<<<<<<<< b
  });
}
```

Sometime `fileReader.readAsDataURL` is unable to get blob from the dictionary after `new Blob(blobs)` and then reject with `Unable to resolve data for blob: blobId` in iOS or `The specified blob is invalid` in android. Because line `a` and `b` can be run in different thread. `new Blob([])` is in progress and `fileReader.readAsDataURL` accesses the blob dictionary ahead of the blob creation.

The expected behaviour is it should finish new Blob([]) first and then readAsDataURL(blob)

To fix that, there should be a lock inside the method `createFromParts`. For iOS, It needs to be a recursive_mutex to allow same thread to acquire lock

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - fix the race condition when calling readAsDataURL after new Blob(blobs)

Pull Request resolved: facebook#34096

Reviewed By: cipolleschi

Differential Revision: D37514981

Pulled By: javache

fbshipit-source-id: 4bf84ece99871276ecaa5aa1849b9145ff44dbf4
(cherry picked from commit 112d678)
Kudo pushed a commit to expo/react-native that referenced this pull request Jul 21, 2022
…(blobs) (facebook#34096)

Summary:
```js
async () => {
  let blobs = [];
  for (let i = 0; i < 4; i++) {
    const res = await fetch();
    blobs = [...blobs, await res.blob()]
  }
  const blob = new Blob(blobs); // <<<<<<<<<<<<<<< a
  return await new Promise((resolve, reject) => {
    const fileReader = new FileReader();
    fileReader.onload = async () => {
      await RNFS.writeFile(destPath, (fileReader.result as string).split(',')[1], 'base64');
      resolve(destPath);
    };
    fileReader.onabort = () => {
      reject('');
    };
    fileReader.onerror = (event) => {
      reject('');
    };
    fileReader.readAsDataURL(blob); // <<<<<<<<<<<<<<< b
  });
}
```

Sometime `fileReader.readAsDataURL` is unable to get blob from the dictionary after `new Blob(blobs)` and then reject with `Unable to resolve data for blob: blobId` in iOS or `The specified blob is invalid` in android. Because line `a` and `b` can be run in different thread. `new Blob([])` is in progress and `fileReader.readAsDataURL` accesses the blob dictionary ahead of the blob creation.

The expected behaviour is it should finish new Blob([]) first and then readAsDataURL(blob)

To fix that, there should be a lock inside the method `createFromParts`. For iOS, It needs to be a recursive_mutex to allow same thread to acquire lock

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[iOS] [Fixed] - fix the race condition when calling readAsDataURL after new Blob(blobs)

Pull Request resolved: facebook#34096

Reviewed By: cipolleschi

Differential Revision: D37514981

Pulled By: javache

fbshipit-source-id: 4bf84ece99871276ecaa5aa1849b9145ff44dbf4
(cherry picked from commit 112d678)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. Platform: iOS iOS applications. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants