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: always ensure element before set to weakmap #75012

Merged
merged 5 commits into from
Jan 17, 2025
Merged

Conversation

huozhi
Copy link
Member

@huozhi huozhi commented Jan 17, 2025

What

Regression introduced in #74670, where the element somehow is invalid to be observed by observer or be set as key in to WeakMap. The possible cause is mountInstance is calling with null.

TypeError: Failed to execute 'observe' on 'IntersectionObserver': parameter 1 is not of type 'Element'.
TypeError: Invalid value used as weak map key
    at WeakMap.set (<anonymous>)

The root cause is the useMergedRef could return null in callback, we need to filter them out before passing to mountLinkInstance or unmountLinkInstance

@ijjk ijjk added created-by: Next.js team PRs by the Next.js team. type: next labels Jan 17, 2025
@ijjk
Copy link
Member

ijjk commented Jan 17, 2025

Stats from current PR

Default Build (Increase detected ⚠️)
General Overall increase ⚠️
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
buildDuration 18.3s 15.5s N/A
buildDurationCached 14.5s 12.3s N/A
nodeModulesSize 418 MB 418 MB ⚠️ +1.78 kB
nextStartRea..uration (ms) 396ms 394ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
5306-HASH.js gzip 54 kB 54 kB N/A
8276.HASH.js gzip 169 B 168 B N/A
8377-HASH.js gzip 5.44 kB 5.44 kB N/A
bccd1874-HASH.js gzip 52.9 kB 52.9 kB
framework-HASH.js gzip 57.5 kB 57.5 kB N/A
main-app-HASH.js gzip 240 B 242 B N/A
main-HASH.js gzip 34.3 kB 34.3 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 52.9 kB 52.9 kB
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
polyfills-HASH.js gzip 39.4 kB 39.4 kB
Overall change 39.4 kB 39.4 kB
Client Pages
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 193 B 193 B
amp-HASH.js gzip 512 B 510 B N/A
css-HASH.js gzip 343 B 342 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB
edge-ssr-HASH.js gzip 265 B 265 B
head-HASH.js gzip 363 B 362 B N/A
hooks-HASH.js gzip 393 B 392 B N/A
image-HASH.js gzip 4.57 kB 4.57 kB N/A
index-HASH.js gzip 268 B 268 B
link-HASH.js gzip 2.35 kB 2.34 kB N/A
routerDirect..HASH.js gzip 328 B 328 B
script-HASH.js gzip 397 B 397 B
withRouter-HASH.js gzip 323 B 326 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 3.59 kB 3.59 kB
Client Build Manifests
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
_buildManifest.js gzip 749 B 747 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
index.html gzip 522 B 524 B N/A
link.html gzip 538 B 537 B N/A
withRouter.html gzip 519 B 520 B N/A
Overall change 0 B 0 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
edge-ssr.js gzip 129 kB 129 kB N/A
page.js gzip 208 kB 208 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
middleware-b..fest.js gzip 669 B 665 B N/A
middleware-r..fest.js gzip 155 B 156 B N/A
middleware.js gzip 31.3 kB 31.3 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
274-experime...dev.js gzip 322 B 322 B
274.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 374 kB 374 kB
app-page-exp..prod.js gzip 130 kB 130 kB
app-page-tur..prod.js gzip 143 kB 143 kB
app-page-tur..prod.js gzip 139 kB 139 kB
app-page.run...dev.js gzip 362 kB 362 kB
app-page.run..prod.js gzip 126 kB 126 kB
app-route-ex...dev.js gzip 37.6 kB 37.6 kB
app-route-ex..prod.js gzip 25.6 kB 25.6 kB
app-route-tu..prod.js gzip 25.6 kB 25.6 kB
app-route-tu..prod.js gzip 25.4 kB 25.4 kB
app-route.ru...dev.js gzip 39.2 kB 39.2 kB
app-route.ru..prod.js gzip 25.4 kB 25.4 kB
pages-api-tu..prod.js gzip 9.69 kB 9.69 kB
pages-api.ru...dev.js gzip 11.6 kB 11.6 kB
pages-api.ru..prod.js gzip 9.68 kB 9.68 kB
pages-turbo...prod.js gzip 21.8 kB 21.8 kB
pages.runtim...dev.js gzip 27.6 kB 27.6 kB
pages.runtim..prod.js gzip 21.8 kB 21.8 kB
server.runti..prod.js gzip 916 kB 916 kB
Overall change 2.47 MB 2.47 MB
build cache Overall increase ⚠️
vercel/next.js canary vercel/next.js huozhi/fix-forward Change
0.pack gzip 2.1 MB 2.1 MB ⚠️ +2.13 kB
index.pack gzip 74.7 kB 74.3 kB N/A
Overall change 2.1 MB 2.1 MB ⚠️ +2.13 kB
Diff details
Diff for main-HASH.js

Diff too large to display

Commit: 8331e39

@huozhi huozhi changed the title [do not merge] fix: always ensure element before set to weakmap fix: always ensure element before set to weakmap Jan 17, 2025
@huozhi huozhi marked this pull request as ready for review January 17, 2025 03:14
@huozhi huozhi requested review from acdlite and ztanner January 17, 2025 03:15
@huozhi huozhi requested review from ijjk and eps1lon January 17, 2025 10:04
Copy link
Member

@lubieowoce lubieowoce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm this doesn't look right. observeLinkVisibilityOnMount returns a cleanup, so it should never receive null
(useMergedRef should handle cleanups just fine, we updated it to do that some time ago, but maybe it's buggy?)

Copy link
Member

@lubieowoce lubieowoce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

very weird issue, shouldn't be possible, but LGTM to unblock

@huozhi
Copy link
Member Author

huozhi commented Jan 17, 2025

Updated the comment as after investigation it's from react feeding a null ref rather than the useMergeRef itself returning null

@huozhi huozhi merged commit 7229934 into canary Jan 17, 2025
21 checks passed
@huozhi huozhi deleted the huozhi/fix-forward branch January 17, 2025 14:14
lubieowoce added a commit that referenced this pull request Jan 20, 2025
lubieowoce added a commit that referenced this pull request Jan 20, 2025
lubieowoce added a commit that referenced this pull request Jan 20, 2025
lubieowoce added a commit that referenced this pull request Jan 21, 2025
- Fixes a crash when a Link unmounts (under some VERY specific
conditions)
- Reverts #75012, which implemented a workaround for the crash

### Short explanation

The crash happens because we use a cleanup-returning ref in (the app-dir
version of) Link, and `<Link legacyBehavior>` "leaks" that ref into user
code. If user code does ref-merging incorrectly, i.e. it treats all
callback refs as the old style that gets called with `null`, then our
ref can get called with `null` even though it shouldn't, and crashes.

Technically this crash is not a Next.js bug. But I think a lot user code
uses ref-merging libraries that haven't been updated for 19, and can
crash because of this. So i think the safest course of action is to be
defensive, and change `useMergedRef` to always convert cleanup refs to
normal function refs, so that userspace can't mess up our refs.

### Full explanation

- `observeLinkVisibilityOnMount` inside our Link is a cleanup-returning
ref
- i.e. the new thing added in react 19, `(el: Element) => () =>
void` instead of the old `(el: Element | null) => void`
- user code does  `<Link legacyBehavior><CustomComponent /></Link>`
- `legacyBehavior` means that props get spread onto the child,
including `ref`, so we'll pass the ref created inside Link
to  `CustomComponent`
- AND no `ref` is passed on the `<CustomComponent>`  itself
- with `legacyBehavior`, that'd get extracted and merged with
`observeLinkVisibilityOnMount`
- if there's no user ref, the old implementation of `useMergedRef` would
return `observeLinkVisibilityOnMount` unchanged, thus
passing `CustomComponent` a cleanup ref
- `CustomComponent` uses a buggy ref merging library, i.e. one that
wasn't updated to handle cleanup refs.
- **so `observeLinkVisibilityOnMount` gets treated as an old-style
callback ref, i.e. it gets called with`null`. and stuff explodes,
because it doesn't expect that**

The test in `test/production/next-link-legacybehavior-ref-merging`
reproduces the crash. You can check out the commit that adds it to see
it happen -- I put it before the fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants