Skip to content

Commit

Permalink
Fix error when accessing detached targets from IntersectionObserver e…
Browse files Browse the repository at this point in the history
…ntries (facebook#41449)

Summary:

`IntersectionObserver` was incorrectly throwing errors when reporting entries for detached targets. The problem was that we were deriving the target instance from the instance handle that we keep in native, but React removes the connection between them when the instance handle is unmounted.

This fixes the problem by keeping an internal mapping between instance handle and target internally in the intersection observer manager.

Changelog: [internal]

Differential Revision: D51210456
  • Loading branch information
rubennorte authored and facebook-github-bot committed Nov 14, 2023
1 parent b9b4379 commit 510dc8e
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 16 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -11,11 +11,9 @@
// flowlint unsafe-getters-setters:off

import type ReactNativeElement from '../DOM/Nodes/ReactNativeElement';
import type {InternalInstanceHandle} from '../Renderer/shims/ReactNativeTypes';
import type {NativeIntersectionObserverEntry} from './NativeIntersectionObserver';

import DOMRectReadOnly from '../DOM/Geometry/DOMRectReadOnly';
import {getPublicInstanceFromInternalInstanceHandle} from '../DOM/Nodes/ReadOnlyNode';

/**
* The [`IntersectionObserverEntry`](https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserverEntry)
Expand All @@ -29,9 +27,17 @@ export default class IntersectionObserverEntry {
// We lazily compute all the properties from the raw entry provided by the
// native module, so we avoid unnecessary work.
_nativeEntry: NativeIntersectionObserverEntry;

constructor(nativeEntry: NativeIntersectionObserverEntry) {
// There are cases where this cannot be safely derived from the instance
// handle in the native entry (when the target is detached), so we need to
// keep a reference to it directly.
_target: ReactNativeElement;

constructor(
nativeEntry: NativeIntersectionObserverEntry,
target: ReactNativeElement,
) {
this._nativeEntry = nativeEntry;
this._target = target;
}

/**
Expand Down Expand Up @@ -113,15 +119,7 @@ export default class IntersectionObserverEntry {
* The `ReactNativeElement` whose intersection with the root changed.
*/
get target(): ReactNativeElement {
const targetInstanceHandle: InternalInstanceHandle =
// $FlowExpectedError[incompatible-type] native modules don't support using InternalInstanceHandle as a type
this._nativeEntry.targetInstanceHandle;

const targetElement =
getPublicInstanceFromInternalInstanceHandle(targetInstanceHandle);

// $FlowExpectedError[incompatible-cast] we know targetElement is a ReactNativeElement, not just a ReadOnlyNode
return (targetElement: ReactNativeElement);
return this._target;
}

/**
Expand All @@ -135,6 +133,7 @@ export default class IntersectionObserverEntry {

export function createIntersectionObserverEntry(
entry: NativeIntersectionObserverEntry,
target: ReactNativeElement,
): IntersectionObserverEntry {
return new IntersectionObserverEntry(entry);
return new IntersectionObserverEntry(entry, target);
}
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import type IntersectionObserver, {
} from './IntersectionObserver';
import type IntersectionObserverEntry from './IntersectionObserverEntry';

import {getShadowNode} from '../DOM/Nodes/ReadOnlyNode';
import {getInstanceHandle, getShadowNode} from '../DOM/Nodes/ReadOnlyNode';
import * as Systrace from '../Performance/Systrace';
import warnOnce from '../Utilities/warnOnce';
import {createIntersectionObserverEntry} from './IntersectionObserverEntry';
Expand All @@ -40,6 +40,36 @@ const registeredIntersectionObservers: Map<
{observer: IntersectionObserver, callback: IntersectionObserverCallback},
> = new Map();

// We need to keep the mapping from instance handles to targets because when
// targets are detached (their components are unmounted), React resets the
// instance handle to prevent memory leaks and it cuts the connection between
// the instance handle and the target.
const instanceHandleToTargetMap: WeakMap<interface {}, ReactNativeElement> =
new WeakMap();

function getTargetFromInstanceHandle(
instanceHandle: mixed,
): ?ReactNativeElement {
// $FlowExpectedError[incompatible-type] instanceHandle is typed as mixed but we know it's an object and we need it to be to use it as a key in a WeakMap.
const key: interface {} = instanceHandle;
return instanceHandleToTargetMap.get(key);
}

function setTargetForInstanceHandle(
instanceHandle: mixed,
target: ReactNativeElement,
): void {
// $FlowExpectedError[incompatible-type] instanceHandle is typed as mixed but we know it's an object and we need it to be to use it as a key in a WeakMap.
const key: interface {} = instanceHandle;
instanceHandleToTargetMap.set(key, target);
}

function unsetTargetForInstanceHandle(instanceHandle: mixed): void {
// $FlowExpectedError[incompatible-type] instanceHandle is typed as mixed but we know it's an object and we need it to be to use it as a key in a WeakMap.
const key: interface {} = instanceHandle;
instanceHandleToTargetMap.delete(key);
}

/**
* Registers the given intersection observer and returns a unique ID for it,
* which is required to start observing targets.
Expand Down Expand Up @@ -109,6 +139,18 @@ export function observe({
return;
}

const instanceHandle = getInstanceHandle(target);
if (instanceHandle == null) {
console.error(
'IntersectionObserverManager: could not find reference to instance handle from target',
);
return;
}

// Store the mapping between the instance handle and the target so we can
// access it even after the instance handle has been unmounted.
setTargetForInstanceHandle(instanceHandle, target);

if (!isConnected) {
NativeIntersectionObserver.connect(notifyIntersectionObservers);
isConnected = true;
Expand Down Expand Up @@ -148,10 +190,22 @@ export function unobserve(
return;
}

const instanceHandle = getInstanceHandle(target);
if (instanceHandle == null) {
console.error(
'IntersectionObserverManager: could not find reference to instance handle from target',
);
return;
}

NativeIntersectionObserver.unobserve(
intersectionObserverId,
targetShadowNode,
);

// We can guarantee we won't receive any more entries for this target,
// so we don't need to keep the mapping anymore.
unsetTargetForInstanceHandle(instanceHandle);
}

/**
Expand Down Expand Up @@ -188,7 +242,16 @@ function doNotifyIntersectionObservers(): void {
list = [];
entriesByObserver.set(nativeEntry.intersectionObserverId, list);
}
list.push(createIntersectionObserverEntry(nativeEntry));

const target = getTargetFromInstanceHandle(
nativeEntry.targetInstanceHandle,
);
if (target == null) {
console.warn('Could not find target to create IntersectionObserverEntry');
continue;
}

list.push(createIntersectionObserverEntry(nativeEntry, target));
}

for (const [
Expand Down

0 comments on commit 510dc8e

Please sign in to comment.