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

Uncaught TypeError: Cannot read properties of undefined (reading 'startTime') in getCLS logic #6083

Closed
3 tasks done
Tofandel opened this issue Oct 28, 2022 · 13 comments
Closed
3 tasks done
Assignees

Comments

@Tofandel
Copy link

Tofandel commented Oct 28, 2022

Is there an existing issue for this?

How do you use Sentry?

Self-hosted/on-premise

Which package are you using?

@sentry/vue

SDK Version

7.17.2

Framework Version

7.17.2

Link to Sentry event

https://sentry.tukan.hu/share/issue/cc80369f87ff438e88e5d6caf67987c9/

Steps to Reproduce

Move the mouse around on an element creating a layout shift (eg a row that expands on hover)

Expected Result

No error

Actual Result

Js error on line
https://github.com/getsentry/sentry-javascript/blob/master/packages/tracing/src/browser/web-vitals/getCLS.ts#L64
Uncaught TypeError: Cannot read properties of undefined (reading 'startTime')

image

firstSessionEntry and lastSessionEntry are undefined but being used without any check

@AbhiPrasad
Copy link
Member

Hey @Tofandel, thanks for reporting!

This is related to us upgrading to web vitals v3 #5987, and seems to be a problem with web vitals as well:

https://github.com/GoogleChrome/web-vitals/blob/7f0ed0bfb03c356e348a558a3eda111b498a2a11/src/onCLS.ts#L83-L91

@philipwalton do we have to adjust the logic in web vitals repo - or is this a case that shouldn't be possible?

@AbhiPrasad AbhiPrasad changed the title [Tracing] Js error Uncaught TypeError: Cannot read properties of undefined (reading 'startTime') in getCLS logic Oct 28, 2022
@philipwalton
Copy link

@philipwalton do we have to adjust the logic in web vitals repo - or is this a case that shouldn't be possible?

I don't believe this case should be possible, so I'm not sure how it's happening here.

Referencing the startTime property of firstSessionEntry and lastSessionEntry only ever happens behind a check for sessionValue being truthy, and the only way session sessionValue can be truthy is if session entries have also been set.

I'd need more context for the error report in order to know whether or not it reflects a real case or perhaps just a fluke error.

@tylerzey
Copy link

I'm seeing the same error with @sentry/nextjs. (Hosting a react app on vercel)

@Tofandel
Copy link
Author

@philipwalton it's definitely not a fluke, I get the error every single layout shift 100% of the time

@timfish
Copy link
Collaborator

timfish commented Oct 29, 2022

Looking through the changes for the Web Vitals v3 update I performed, the sessionEntries.length !== 0 check has been lost from here:

Before:

if (
sessionValue &&
sessionEntries.length !== 0 &&
entry.startTime - lastSessionEntry.startTime < 1000 &&
entry.startTime - firstSessionEntry.startTime < 5000
) {

After:

if (
sessionValue &&
entry.startTime - lastSessionEntry.startTime < 1000 &&
entry.startTime - firstSessionEntry.startTime < 5000
) {

This check was added in this PR but as far as I can tell, the sessionEntries.length check has never existed in the Web Vitals source.

Let me do some debugging and I'll get back to you...

@timfish
Copy link
Collaborator

timfish commented Oct 29, 2022

I'm unable to reproduce the error with the most basic example:

<!DOCTYPE html>
<html lang="en">
  <head>
    <title>CLS Test</title>
    <style>
      .test:hover {
        padding-top: 200px;
      }
    </style>
  </head>
  <body>
    <h1 class="test">Hello, world!</h1>

    <script src="https://browser.sentry-cdn.com/7.17.2/bundle.tracing.debug.min.js"></script>
    <script>
      Sentry.init({
        dsn: "__DSN__",
        debug: true,
        release: "tracing-demo@1.2.3-test",
        integrations: [new Sentry.BrowserTracing()],
        tracesSampleRate: 1.0,
      });
    </script>
  </body>
</html>

Any info that may help me reproduce the issue locally would be greatly appreciated!

@Inlustra
Copy link

Inlustra commented Oct 29, 2022

I can confirm that I'm also seeing this.
I'm seeing this with NextJS and @sentry/nextjs also when running in dev mode.

@philipwalton
Copy link

@philipwalton it's definitely not a fluke, I get the error every single layout shift 100% of the time

@Tofandel do you have an repro you could share? Even if not a reduced case, I'd like to try debugging to see how this is happening.

@paraboul
Copy link

Also seeing this reported in production (@sentry/vue).

@Tofandel
Copy link
Author

Humm well now that I reverted to previous version and reupgraded to make a reproduction, I'm trying the same thing as before and it's not happening at all... When before it was always happening 🤔

Looking at the source, it does make sense it wouldn't happen because sessionValue starts at 0 and only gets set when sessionEntries gets set as well.. All variables seem scoped so unless there is some black magic happening with object references. This is a WTF moment

The only thing that makes sense is this is a race condition between the extremely short amount of time sessionValue = entry.value; and sessionEntries = [entry]; are executed, the handleEntries fires another time from another event, so unless it fires 3000 times a second I don't see this happening all the time like it did before

@dahanyosi
Copy link

Same issue here, getting several errors every few minutes

Screen Shot 2022-10-30 at 18 57 20

@AbhiPrasad
Copy link
Member

@timfish for the time being let’s just add a Boolean check to the conditional to unblock users? We can then cut a patch release with that.

We can always revert it later when we investigate more and reproduce it.

@AbhiPrasad
Copy link
Member

This has been released with https://github.com/getsentry/sentry-javascript/releases/tag/7.17.3, closing as such, but maybe we should open an issue in the web vitals repo and continue investigating there?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants