React-native: Clean up mountSafeCallback

[Ananlabs]

Environment:
OS: Windows 10
Node: 8.9.4
Yarn: 1.3.2
npm: 5.6.0
Watchman: Not Found
Xcode: N/A
Android Studio: Version 3.0.0.0 AI-171.4443003

Packages: (wanted => installed)
react: 16.3.1 => 16.3.1
react-native: 0.55.2 => 0.55.2

"dependencies": {
"axios": "^0.18.0",
"native-base": "^2.4.2",
"react": "16.3.1",
"react-native": "0.55.2",
"react-navigation": "^1.5.11",
"react-redux": "^5.0.7",
"redux": "^3.7.2",
"redux-thunk": "^2.2.0"
}

Is this an issue reactjs itself?

[Lxxyx]

Seems to be react problem.
https://github.com/facebook/react/search?utf8=%E2%9C%93&q=isMounted&type=

[iamdurui]

I also have this

[FredyC]

import { YellowBox } from 'react-native'
YellowBox.ignoreWarnings(['Warning: isMounted(...) is deprecated'])

Explanation: https://github.com/facebook/react-native/issues/18921#issuecomment-382727030

[gaearon]

According to https://github.com/facebook/react-native/issues/18921, here is a reproducing case:

import NativeMethodsMixin from 'NativeMethodsMixin';

// element
<View ref={e => this.renderedElement = e}>

// code
NativeMethodsMixin.measureInWindow.call(this.renderedElement, (x, y, width, height) => {
        this.setState({ xOffset: x, yOffset: y, elementWidth: width, elementHeight: height });
      });

Is your code similar? Can you post your code please?

[gaearon]

Another apparent example: https://github.com/mxck/react-native-material-menu/blob/c755153c46b935089708b30d0dcda53ee57a622c/src/Menu.js#L91.

    this._container.measureInWindow((x, y) => {
      this.setState({ menuState: STATES.SHOWN, top: y, left: x });
    });

where this._container is a ref to View.

[gaearon]

There is a good investigation about the root cause in https://github.com/facebook/react-native/issues/18927#issue-315632048.

[gaearon]

My guess is that https://github.com/facebook/react-native/commit/26734a8473ac2f5715f2b7a016f0cc8a15c6f073#diff-a562e4a7ac9e900c6d2bc644453e9152 is responsible for this. It was using isMounted() through NativeMethodsMixin (which suppressed the warning in combination with createReactClass) but ES6 classes don't support isMounted() so it fires for View now.

[gaearon]

Hmm, no, that can't make sense. It's an old change. 🤔

[gaearon]

Nope, I was right at first. This is indeed caused by https://github.com/facebook/react-native/commit/26734a8473ac2f5715f2b7a016f0cc8a15c6f073.

[slorber]

@gaearon I've met this issue, and the root cause of the warning seems to be due to internal React code. Just checking the existance of the isMounted method does trigger the warning due to proxy get

[gaearon]

Yes, the reason it didn’t fire before was because the code was hitting the first if/else branch. But then https://github.com/facebook/react-native/commit/26734a8473ac2f5715f2b7a016f0cc8a15c6f073 removed createClass from View and so View itself started getting those warnings.

[idcmsbeta]

Are "ref" not allowed also ? i'm using react-native-largelist which also using ref @gaearon .

<LargeList
      ref={ref => (this.listRef = ref)}

[slorber]

@idcmsbeta this works fine for me with no warning

[gaearon]

It's "allowed". The issue is open because the warning has a false positive. This means it fires when it shouldn't.

[yungsters]

I realized I should've shared an update here sooner and apologize.

I've been working on a fix for this since last Friday that involves replacing our use of ReactNative.NativeComponent in favor of the new React.forwardRef API. It's getting close to landing.

[franciskim722]

Will the fix for this be released soon? Or should users revert to a previous version

[yungsters]

The fixes should have already been merged — 3e534b9aab5156adac67762877b2457408fe8934 and e708010d18f938e2d6b6424cfc9485d8e5dd2800.

[gaearon]

Does that mean we should ask @grabbou to cherry-pick them to stable?

[grabbou]

I think that makes sense since at the time of the writing, it's the current stable and I am in the process of working on the next release candidate.

[yungsters]

Hmm, there's actually many commits that contributed to this fix (including some bug fixes post-commit) because I had to refactor a lot of the existing old code. It might be too risky to patch into stable.

[grabbou]

Yeah, that's my conclusion I posted in
react-native-community/react-native-releases.

On Tue, 15 May 2018 at 00:07 Timothy Yung [e-mail] wrote:

Hmm, there's actually many commits that contributed to this fix (including
some bug fixes post-commit) because I had to refactor a lot of the existing
old code. It might be too risky to patch into stable.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react-native/issues/18868#issuecomment-388978168,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACWcxgjnp6aJ64gDJgXQbAhfUL6DFMX6ks5tygAngaJpZM4TVtJY
.

[karanmartian]

is there any update on this issue? somehow with the isMounted warnings my network images are either not loading or loading very slowly. i am on latest 27.0.0 of Expo and I can’t find a way to even ignore this warning. Placing the yellow box warning neglect code in app.js doesn’t work. i dont have an index.js

[jhalborg]

+1 @karanmartian

[dihanmalik]

Still waiting on this to be fixed. It keeps my iMac super laggy because it produce memory leaks every time a component is mounting or unmounting. My development is always interrupted so I need to restart the simulator. Btw I'm using Expo XDE on my case.

[gaearon]

Did you get a chance to read the comments above? They explicitly say it has been fixed on master, but the fix is too complex to port into the stable branch. So you'll have to wait for the next stable release for the fix.

While you're waiting for the fix, you can apply this temporary workaround:

import { YellowBox } from 'react-native'
YellowBox.ignoreWarnings(['Warning: isMounted(...) is deprecated'])

You'll need to remove those lines after updating to the next stable release.

Hope it helps!

[larixer]

@gaearon Disabling YellowBox is only half of the problem. Unfortunately this workaround doesn't really help much in case of Expo. Due to this warning Expo bombards React Native server with symbolicate requests which result in noticeable lags during development.

[gaearon]

Can you please file this with Expo? I'm sure they can figure out a temporary fix for this.

[slorber]

So you'll have a bad experience until next release. If this is really
important for you, you can still downgrade.

Le ven. 18 mai 2018 à 15:39, Victor Vlasenko [e-mail] a
écrit :

@gaearon https://github.com/gaearon Disabling YellowBox is only half of
the problem. Unfortunately this workaround doesn't really help much in case
of Expo. Due to this warning Expo bombards React Native server with
symbolicate requests which result in noticeable lags during development.

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/facebook/react-native/issues/18868#issuecomment-390209532,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAtvPpp2ueIXnsJTITjF5QVs6wQd5I-lks5tzs8YgaJpZM4TVtJY
.

[gaearon]

I pinged some Expo folks about this.

[kosmicbird]

Hope Expo provides a solution soon... the nonstop warnings in XDE client are aggravating and causing major dev environment lag for me. If anyone has a link to Expo updates regarding this, please post so I can follow it obsessively until they fix it

[esamelson]

Hi all - for those using Expo & XDE, we've just published [email protected] to npm which should resolve this issue.

Another quick temporary workaround could be adding something like the following snippet to your root js file:

global.__old_console_warn = global.__old_console_warn || console.warn;
global.console.warn = str => {
  let tst = (str || '') + '';
  if (tst.startsWith('Warning: isMounted(...) is deprecated')) {
    return;
  }
  return global.__old_console_warn.apply(console, [str]);
};

Hope this helps! Sorry for the delay on our end.

[karanmartian]

cool, so I guess this Medium post should be updated accordingly - https://blog.expo.io/expo-sdk-v27-0-0-is-now-available-898bf1e5b0e4 because those searching to upgrade their Expo packages from 26.0.0 will encounter this post.

[karanmartian]

I get these warnings at yarn after upgrading to expo 27.0.2:

warning "expo > [email protected]" has unmet peer dependency "react-native-web@*".
warning "expo > [email protected]" has incorrect peer dependency "react-native@^0.51 || ^0.52 || ^0.53 || ^0.54".
warning "expo > react-native-web-maps > [email protected]" has incorrect peer dependency "[email protected]".
warning "expo > react-native-web-maps > [email protected]" has unmet peer dependency "[email protected]".
warning "expo > react-native-web-maps > react-google-maps > [email protected]" has incorrect peer dependency "react@^0.14.6 || ^15.0.0-0".
warning "react-native > [email protected]" has unmet peer dependency "eslint@^3.17.0 || ^4.0.0".

[dihanmalik]

Finally! My machine is not lagging anymore. Thank you sir @gaearon and @esamelson.

[VicFrolov]

I'm a bit confused how adding

import { YellowBox } from 'react-native'
YellowBox.ignoreWarnings(['Warning: isMounted(...) is deprecated'])

would solve memory issues / leaks. Is this purely for debug? We upgraded our react to 0.55.4 from 0.52.0, and our memory usage went up from 250 to about 600 on iOS, making the app very unstable.

[BenCook28]

I appreciate the responses to this issue and definitely React Native in general, but I still get the same warning, even when included the whole message, 'isMounted(...) is deprecated in plain JavaScript React classes. Instead, make sure to clean up subscriptions and pending requests in componentWillUnmount to prevent memory leaks.' as a string in YellowBox.ignoreWarnings in the root index.js. 'Warning: isMounted(...) is deprecated' didn't remove the warning either.

[gaearon]

@VicFrolov This workaround isn't to "solve memory leaks", it's to avoid a false positive from when the warning comes up for no good reason. This has been fixed on master, and will be in the upcoming 0.56 releases, so you can remove it when you upgrade to 0.56.

Whatever memory leaks you're experiencing are unrelated to this issue. You can file a new one about them.

[gaearon]

@BenCook28 Please create a reproducible example and post somewhere so somebody can take a look.

[BenCook28]

@gaearon Thank you for your response, Dan. I respect you and your team a lot. It might not be worth it for me to get permission to clone the entire work project and post to my personal Github page if the issue is only a false positive, but here's what I tried in the root index.js:

`import { AppRegistry } from 'react-native';
import App from './App';
import { YellowBox } from 'react-native';
YellowBox.ignoreWarnings(['isMounted(...) is deprecated in plain JavaScript React classes. Instead, make sure to clean up subscriptions and pending requests in componentWillUnmount to prevent memory leaks.']);

AppRegistry.registerComponent('GroopMobile', () => App);
`

[FredyC]

@BenCook28 you certainly don't need a whole warning message there, on the contrary, if there some slight difference in wording, it won't work. Now, what do you actually expect to happen? The yellow box is that _annoying_ thing popping on the mobile screen during development. If you want to hide message from console or you are using Expo, you have do https://github.com/facebook/react-native/issues/18868#issuecomment-390369568 also.

[hramos]

Closing as this is fixed in master, and 3e534b9aab5156adac67762877b2457408fe8934 and e708010d18f938e2d6b6424cfc9485d8e5dd2800 are now part of the new 0.56 release candidate.

[peacechen]

This warning is still showing up in RN 0.56

My app is using react-native-collapsible, but no direct calls to isMounted

[yungsters]

@peacechen Can you check the stack trace of the warning to see what is calling the deprecated method?

[slorber]

Just wanted to report I use Expo SDK28 and latest react-native-collapsible and I don't get this warning in my app

[pvinis]

try with just react-native init.

[peacechen]

@yungsters The warning shows up sporadically, which I find odd. I tried to trigger it but haven't been able to today. Other warnings are similarly unpredictable (e.g. "... requires main queue setup...", etc).
I'll post the stack trace next time it rears its head.

[pvinis]

ok here is a stacktrace

and here is the ReactNativeRenderer-dev.js function that calls isMounted

does that help?

[yungsters]

@pvinis — Yes, that helps. The version of YellowBox that you are using indicates to me that you are not on the latest released version of React Native (which means you do not have the fixes).

[pvinis]

ah damn. I'm not on 56, right. didn't get the chance to upgrade yet. hopefully others on 56 that get this can post a stacktrace too.

[eltonk]

Any fix for this issue?

[yungsters]

@eltonk Can you check the stack trace of the warning to see what is calling the deprecated method? Also, please verify that you are on the latest released version of React Native.

[gaearon]

My understanding is that if you see this issue when:

• Something is calling mountSafeCallback (e.g. through measureInWindow)
• The passed context is a component
• That component is an ES6 class component

I have no idea why React Native attempts to call isMounted() on any ES6 classes. It's explicitly deprecated by React.

[gaearon]

In other words https://github.com/facebook/react-native/commit/3e534b9aab5156adac67762877b2457408fe8934 and https://github.com/facebook/react-native/commit/e708010d18f938e2d6b6424cfc9485d8e5dd2800 don't seem to "fix" the issue. They just work around it for built-in components by forwarding their refs to host components. And host components don't have isMounted() defined on them. So mountSafeCallback becomes a no-op (and isn't actually "mount safe" anymore).

So effectively mountSafeCallback seems to lie about its purpose. It just passes host components through without any checks (and thus doesn't trigger warnings for View in 0.56). But if you give it a non-host component (such as a custom ES6 class), it will produce the same warning because it will attempt to call isMounted(). Like we see reported in https://github.com/react-community/react-native-safe-area-view/pull/39.

Given that mountSafeCallback is already unsafe in practice (at least for host components), can we just remove it?

[yungsters]

@gaearon, here are the places where React Native invokes mountSafeCallback: https://github.com/facebook/react/search?q=mountSafeCallback&unscoped_q=mountSafeCallback

All references look like this:

UIManager.measure(..., mountSafeCallback)(...))

The root cause would seem to be that UIManager does not currently expose any way to cancel a scheduled callback. Without this, call sites cannot confidently replace the calls to mountSafeCallback with something like this:

this._measuring = UIManager.measure(..., ...);
// ...
if (this._measuring != null) {
  this._measuring.cancel();
}

[tychota]

@yungsters came to the same conclusion here: https://github.com/react-community/react-native-safe-area-view/pull/39#issuecomment-413598860

It it something that I can to contribute, with some help or it is too complex ?

[gaearon]

The root cause would seem to be that UIManager does not currently expose any way to cancel a scheduled callback. Without this, call sites cannot confidently replace the calls to mountSafeCallback with something like this

This makes sense. I don't argue with this. My argument is that mountSafeCallback already seems to not do anything for built-in components like RCTView / RCTText (to which normal Text and View forward their refs) because they have neither __isMounted nor isMounted() according to their definition. So the whole premise of mountSafeCallback being "safe" already seems questionable.

And for ES6 React classes, the method doesn't exist either — just a getter with warnings.

So the only kind of classes for which mountSafeCallback still provides safety is createClass() components. Or custom classes that manually define a method called isMounted() (seems unlikely). For everything else it's a potentially false-positive inducing noop.

[tychota]

The repro should be https://snack.expo.io/SJ19rQX8X but for some reason it don't reproduce on expo :/ Seems that the error is already ignored in expo !

https://github.com/expo/expo-sdk/blob/35b7536a68859ce9157277229a552f787acd72c1/src/Expo.js#L18-L27

(ccing @esamelson and @fson that did the ignore in expo: they can be interested by the result of this conversation to remove the ignore once the change is merge in RN).

[yungsters]

@gaearon, I think you're right. They do still point out real problems, though. So I think they're worth keeping until we've cleaned up the call sites.

It it something that I can to contribute, with some help or it is too complex ?

@tychota — Thanks for digging into it already. I apologize for not having seen your comment on the other pull request.

If you're down to create the sustainable solution, it would be great if you refactored UIManager (in JS) to be cancelable. The measure methods (and family of methods) should return {cancel: () => void}. (Ideally, it would also cancel the upstream native measure operation, but the asynchronously queued nature of the cancelation will lead to race conditions that require a JS cancelation anyway).

[gaearon]

Let's reopen? The underlying problem still needs a solution.

[tychota]

@yungsters: nice I was fearing that this include Folly modification to properly cancel the native callback (Pff).

Let me have a look, I will sync with you here to be sure I get it write before doing the PRs, Ok ?

[tychota]

So far:

First steps

Add a makeCancelable in https://github.com/facebook/react/blob/45b90d48668dae48aad611819b891d029aa2fb27/packages/react-native-renderer/src/ReactNativeFiberHostComponent.js that look like:

const makeCancelable = (callback) => {
  let hasCanceled = false;

  const wrappedCallback = (...args) => {
    if (hasCanceled) return;
    callback(...args);
  }

  return {
    callback: wrappedCallback,
    cancel() {
      hasCanceled = true;
    },
  };
};

Second Step

Change https://github.com/facebook/react/blob/45b90d48668dae48aad611819b891d029aa2fb27/packages/react-native-renderer/src/ReactNativeFiberHostComponent.js

class ReactNativeFiberHostComponent {
  _children: Array<Instance | number>;
  _nativeTag: number;
+  _pendingCancelableCallbacks: Array<Function>;
  viewConfig: ReactNativeBaseComponentViewConfig;

  constructor(tag: number, viewConfig: ReactNativeBaseComponentViewConfig) {
    this._nativeTag = tag;
    this._children = [];
+ this._pendingCancelableCallbacks = [];
    this.viewConfig = viewConfig;
  }

  // ...
  measure(callback: MeasureOnSuccessCallback) {
+    const cancelableCallback = makeCancelable(callback);
-    UIManager.measure(this._nativeTag, mountSafeCallback(this, callback));
+    UIManager.measure(this._nativeTag, cancelableCallback.callback);
+    this._pendingCancelableCallbacks.push(cancelableCallback);
  }

  measureInWindow(callback: MeasureInWindowOnSuccessCallback) {
+    // same
  }

  measureLayout(
    relativeToNativeNode: number,
    onSuccess: MeasureLayoutOnSuccessCallback,
    onFail: () => void /* currently unused */,
  ) {
+    // same
  }
}

// eslint-disable-next-line no-unused-expressions
(ReactNativeFiberHostComponent.prototype: NativeMethodsMixinType);

export default ReactNativeFiberHostComponent;

Then somewhere in the renderer, maybe in https://github.com/facebook/react/blob/master/packages/react-native-renderer/src/ReactNativeRenderer.js, map over the list of cancellableCalleback and really cancel them ? Not sure how to access _pendingCancelableCallbacks from renderer yet.

@gaearon is the pattern OK ? I'm supposed to store stuff in NativeComponent level ? Or is there a way to store in underlying fibers ?

Any performance issue to expect by storing a list of callbacks and then canceling them. Is it something to expect only when the user does a measure itself so it has a bigger impact perf wise ?

Step 3: Properly remove mountSafeCallback from the codebase

Maybe I need to add a shim for third Party using it ?

Step 4: Investigate the upstream cancelation

Ideally, it would also cancel the upstream native measure operation, but the asynchronously queued nature of the cancelation will lead to race conditions that require a JS cancelation anyway

I'm lost here and I need some pointer (no pun intended ^^).

Three level i can act (as far as i understand):

• folly datastructure to use or create a cancelable datastructure (Task) : does it exist
• cxx react: I found https://github.com/facebook/react-native/blob/da7873563bff945086a70306cc25fa4c048bb84b/ReactCommon/cxxreact/CxxNativeModule.cpp#L21-L33 that represent the C++ object representing a callback right ?
• java or android wrapping of this cxx react callback

What the best way to do ?

In any case i will need to modify the implementation of UIManager so objc and java for current RN (does it has a name) and objc wrapper, android wrapper and cpp for react native Fabric, right cc @shergin for fabric part.

If I modify at somelevel the callback to add a way to cancel it would it be enough for me to call a new function through bridge to cancel all callback when the nativeTag is unmounted.
Where can I store the pending callback of one instance of Component. UIManager is component type specific (one manager for all RCTView right ?). Is there possible to store the array of cancelableCallback to view or shadow view directly ?

My sentence probably doesn't make any sense but I'm super interested to see how it works internally.

[ThomasLarge]

Upgrading to 0.56.0 seems to of fixes this for me

[tychota]

@gaearon can we rename the issue to "Clean up mountSafeCallback" or something that indicate that the issue is swallowed but not properly fixed ?

@yungsters @gaearon what do you think of the above plan ?

[tychota]

Thank you @hramos !

[gaearon]

@tychota I don't think your approach matches what @yungsters suggested. If I understand it right, the suggestion was to make measure() itself return a cancellation function. So that the caller is responsible for cleaning it up — not the class that defines measure(). Can you change to that instead?

[gaearon]

I have a temporary fix for the false positive warning here: https://github.com/facebook/react/pull/13511. We'd still need to leave this open if we get it in — because they underlying issue would still be a problem.

[tychota]

@geaeron, i'm in vacation for one week, will try to adjust when I come back, if that ok for you.

[gaearon]

Okay, React Native 0.57 should no longer have false positives for this warning. You can remove it from your filters.