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

Out of Memory (OOM) since updating CDN to Loader v8 #13939

Closed
3 tasks done
DarthSonic opened this issue Oct 10, 2024 · 11 comments · Fixed by getsentry/sentry#78993
Closed
3 tasks done

Out of Memory (OOM) since updating CDN to Loader v8 #13939

DarthSonic opened this issue Oct 10, 2024 · 11 comments · Fixed by getsentry/sentry#78993

Comments

@DarthSonic
Copy link

DarthSonic commented Oct 10, 2024

Is there an existing issue for this?

How do you use Sentry?

Sentry Saas (sentry.io)

Which SDK are you using?

Sentry Browser Loader

SDK Version

8.33.1

Framework Version

No response

Link to Sentry event

No response

Reproduction Example/SDK Setup

window.sentryOnLoad = function () {
    Sentry.init({
        environment: "production",
        debug: "false",
        release: "fooBuildNumber",
        beforeSend: function(event, hint) {
            try {
                const error = hint.originalException;
                const message = hint.syntheticException;

                if (error && error.message && (error.message.startsWith("AI: 61 message") || error.message.startsWith("%cOpenTok:") || error.message.includes("OpenTok"))) {
                    event.fingerprint = ['ignore-event']; // falls Event nicht verworfen wird
                    sentryDiscardEvent = true; // Event verwerfen!
                } else if (message && message.message && (message.message.startsWith("AI: 61 message") || message.message.startsWith("%cOpenTok:") || message.message.includes("OpenTok"))) {
                    event.fingerprint = ['ignore-event']; // falls Event nicht verworfen wird
                    sentryDiscardEvent = true; // Event verwerfen!
                }
            } finally {
                if (sentryDiscardEvent || latestBuildNumber.startsWith("debug"))
                    return null; // Event verwerfen!
                else
                    return event;
            }
        },
        sendDefaultPii: true,
        ignoreErrors: [
            // Random plugins/extensions
            'top.GLOBALS',
            // See: http://blog.errorception.com/2012/03/tale-of-unfindable-js-error.html
            'originalCreateNotification',
            'canvas.contentDocument',
            'MyApp_RemoveAllHighlights',
            'http://tt.epicplay.com',
            'Can\'t find variable: ZiteReader',
            'jigsaw is not defined',
            'ComboSearch is not defined',
            'http://loading.retry.widdit.com/',
            'atomicFindClose',
            // Facebook borked
            'fb_xd_fragment',
            // ISP "optimizing" proxy - `Cache-Control: no-transform` seems to
            // reduce this. (thanks @@acdha)
            // See http://stackoverflow.com/questions/4113268
            'bmi_SafeAddOnload',
            'EBCallBackMessageReceived',
            // See http://toolbar.conduit.com/Developer/HtmlAndGadget/Methods/JSInjection.aspx
            'conduitPage',
            '/.*OpenTok.*$/',
            '/AI: 61 message.*$/'
        ],
        denyUrls: [
            // Facebook flakiness
            /graph\.facebook\.com/i,
            // Facebook blocked
            /connect\.facebook\.net\/en_US\/all\.js/i,
            // Woopra flakiness
            /eatdifferent\.com\.woopra-ns\.com/i,
            /static\.woopra\.com\/js\/woopra\.js/i,
            // Chrome extensions
            /extensions\//i,
            /^chrome:\/\//i,
            // Other plugins
            /127\.0\.0\.1:4001\/isrunning/i, // Cacaoweb
            /webappstoolbarba\.texthelp\.com\//i,
            /metrics\.itunes\.apple\.com\.edgesuite\.net\//i,
            /localdev\.clevermatch\.com/i, // local developer web,
            /localdev\.clevermatch\.com:44333/i // local developer web (with port number)
        ]
    });

    Sentry.setUser({ "id": "fooUserId" });
    Sentry.setTag("Language", "de");
    Sentry.setTag("RequestLanguage", "de");

    Sentry.configureScope(function(scope) {
        scope.setLevel("error");
    });

    Promise.all([
        Sentry.lazyLoadIntegration("captureConsoleIntegration"),
        Sentry.lazyLoadIntegration("contextLinesIntegration")
    ]).then(([captureConsole, contextLines]) => {
        Sentry.addIntegration(
            captureConsole({
                levels: ['error'] // ,'warn'
            })
        );
        Sentry.addIntegration(
            contextLines()
        );
    });
};
xt>

Steps to Reproduce

  1. Goto https://hire.clevermatch.com/de/ap/2847398e-eefd-4728-83f1-4eed954cfb04/erfahrener-finanzexperte-mit-controlling-fokus/?c=819ae10a-e1f9-4db5-b00b-750884b964e8
  2. Click the blue button on the top right "Kandidate kennenlernen".
  3. Click "Abbrechen" button on the lower right of the modal popup.
  4. repeat that step one or two times.
  5. if no "oom" yet, wait some time

It also happens for other (non-public URLs). One additional public URL is https://hire.clevermatch.com/de/account/login/. I do not really have reproduction steps for that URL, but it seems only waiting for some time will trigger OOM.

Expected Result

Load sentry without leading to "out of memory" in browser.

Actual Result

Some Sentry requests "outstanding" and "out of memory" in browser.

Image

Image

@mydea
Copy link
Member

mydea commented Oct 11, 2024

Hey, thank you for writing in.

Could you also provide the config you had on v7 that was working? Just to see if I can spot something.

One thing I noticed when trying this is that there was a CORS error for loading the scripts. Do you have content policies correctly set up to allow loading scripts from browser.sentry-cdn.com?

I would guess that what's happening is an infinite loop, so something in your config code is triggering an error which triggers an error etc. 🤔

Looking through your code, can it be that sentryDiscardEvent is undefined? At least it is missing in the snippet. This could lead to the finally block failing and leading to an error, which would explain this, possibly. Could you try this:

beforeSend: function(event, hint) {
          try {
              // Assuming latestBuildNumber is set outside somewhere??
              if (latestBuildNumber.startsWith("debug")) {
                return null;
              }

              const error = hint.originalException;
              const message = hint.syntheticException;

              if (error && error.message && (error.message.startsWith("AI: 61 message") || error.message.startsWith("%cOpenTok:") || error.message.includes("OpenTok"))) {
                  event.fingerprint = ['ignore-event']; // falls Event nicht verworfen wird
                  return null; // Event verwerfen!
              } else if (message && message.message && (message.message.startsWith("AI: 61 message") || message.message.startsWith("%cOpenTok:") || message.message.includes("OpenTok"))) {
                  event.fingerprint = ['ignore-event']; // falls Event nicht verworfen wird
                  return null; // Event verwerfen!
              }
          } finally {
             // If an error happens somewhere in this function, handle it gracefully
             return event;
          }
      },

@DarthSonic
Copy link
Author

DarthSonic commented Oct 11, 2024

Hi,

we put a nonce for CSP to the script already but we are currently not preventing script sources by CSP, only frame source:

<script nonce="fooNonce"
        src="https://js.sentry-cdn.com/b586a4e25e5149428f80ba006a982f6f.min.js"
        crossorigin="anonymous"></script>

Never seen a CSP error for sentry so far.

sentryDiscardEvent (and latestBuildNumber) is set and only missing in snippet: const sentryDiscardEvent = @(AzureSettings.SentryEnvironment.IsIn(SentryEnvironment.Debug, SentryEnvironment.Unknown) ? "true" : "false");

I cannot see any error that is thrown repeatedly in the console.

We did not use loader v7, we used CDN integration. So then window.sentryOnLoad event wrapper was not existing before and the integrations where configured by "integrations" properties and scripts loaded over CDN. Everything else was the same. So I doubt it is an error that is thrown repeatedly as it would have happend before.

@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 11, 2024
@DarthSonic
Copy link
Author

DarthSonic commented Oct 11, 2024

Here is some example console with sentry debug log enabled:
Image

I also added a counter to how often that "finally" block is reached and called. In this case it was never triggered (ad denyUrls did prevent that), but OOM does happen.

@DarthSonic
Copy link
Author

DarthSonic commented Oct 11, 2024

P.S.: When I remove Sentry.Init from code, there is no "OOM" happening.

The "OOM" seems to happen when this URL is called:
https://o315191.ingest.us.sentry.io/api/1795169/envelope/?sentry_key=fooKey&sentry_version=7&sentry_client=sentry.javascript.browser%2F8.34.0

Maybe that "sentry_version" parameter is wrong? Sentry sets it itself.

Loader is set to v8 in configuration:
Image

@mydea
Copy link
Member

mydea commented Oct 11, 2024

Ahh, I think I know what it may be: configureScope does not exist anymore in v8. Can you replace that with:

Sentry.getCurrentScope().setLevel("warning");

@DarthSonic
Copy link
Author

Ahh, I think I know what it may be: configureScope does not exist anymore in v8. Can you replace that with:

Sentry.getCurrentScope().setLevel("warning");

This was the solution! I removed it anyway as it is not required anymore.

Thanks for your support.

@DarthSonic
Copy link
Author

P.S.: Maybe there should be some migration document ;-)

@DarthSonic
Copy link
Author

Oh no.... Rejoiced too soon... It just takes longer for the OOM to appear, but it's still coming,

@DarthSonic DarthSonic reopened this Oct 11, 2024
@getsantry getsantry bot moved this to Waiting for: Product Owner in GitHub Issues with 👀 3 Oct 11, 2024
@mydea
Copy link
Member

mydea commented Oct 11, 2024

P.S.: Maybe there should be some migration document ;-)

You can find migration docs here: https://docs.sentry.io/platforms/javascript/migration/v7-to-v8/ :)

Oh no.... Rejoiced too soon... It just takes longer for the OOM to appear, but it's still coming,

Hmm, damn... I opened a PR here to prevent the former case for the future: getsentry/sentry#78993

Hm, i am pretty sure the error is then due to the captureConsole integration. probably something logs an error, which is captured to sentry, which somehow leads to an error being logged. Could you try removing this:

 Sentry.addIntegration(
            captureConsole({
                levels: ['error'] // ,'warn'
            })
        );

and see if that affects it? And if not, try to remove as much as you can from the sentryOnLoad function until it stops failing? But the captureConsole integration is the most probably cause I'd say... 🤔

@DarthSonic
Copy link
Author

Okay. After further investigation it now seems to be a newly triggered error by Cloudflare Turnstile (in some rare cases), that will throw an error repeatedly, which is causing the "OOM" now. That did not happen before as the Sentry integration already stopped the page rendering with its "OOM".

So, the solution is to remove the deprecated configureScope.

@mydea
Copy link
Member

mydea commented Oct 11, 2024

OK, great to hear that!

mydea added a commit to getsentry/sentry that referenced this issue Oct 14, 2024
If a user has an error in their `sentryOnLoad` function for the Loader
Script, we do try-catch it today, but we stop any further processing,
leading to possible issues down the line (e.g. errors not being sent to
Sentry etc).

This PR changes this so that we catch errors in this first, and continue
if it happens. This means that we'll still do the default
`Sentry.init()` and send the error that triggered lazy loading to Sentry
- this was previously swallowed by the catch.

Closes getsentry/sentry-javascript#13939

Test added here:
getsentry/sentry-javascript#13952
cmanallen pushed a commit to getsentry/sentry that referenced this issue Oct 23, 2024
If a user has an error in their `sentryOnLoad` function for the Loader
Script, we do try-catch it today, but we stop any further processing,
leading to possible issues down the line (e.g. errors not being sent to
Sentry etc).

This PR changes this so that we catch errors in this first, and continue
if it happens. This means that we'll still do the default
`Sentry.init()` and send the error that triggered lazy loading to Sentry
- this was previously swallowed by the catch.

Closes getsentry/sentry-javascript#13939

Test added here:
getsentry/sentry-javascript#13952
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

2 participants