NewRelic overwrites window.Promise #187

graemechapman · 2022-02-17T11:47:25Z

Description

As per the title, replacing browser built-ins is generally a bad idea, and can interfere with other code which relies on this functionality.

Steps to Reproduce

Visit a website running NewRelic, and try to use window.Promise.

Expected Behavior

Relevant Logs / Console output

Your Environment

Browser name and version: Chrome 98
Operating System and version: macOS Big Sur

Additional context

n/a

aubreymasten · 2022-02-18T22:20:12Z

Hey @graemechapman, is there a specific issue that you're running into where our Promise instrumentation is interfering with other code?

graemechapman · 2022-02-19T12:44:33Z

Hey @graemechapman, is there a specific issue that you're running into where our Promise instrumentation is interfering with other code?

Hi @aubreymasten I have a class extending from Promise, when run in conjunction with newrelic the additional methods on these class instances are not accessible.

I have been able to work around this for now by grabbing the original promise constructor from window.NREUM.o.PR but obviously this is not ideal.

kucaahbe · 2022-06-29T11:15:25Z

have the same issue as well, when NewRelic kicks in TinyMCE editor can not start:

graemechapman · 2022-06-29T12:12:22Z

have the same issue as well, when NewRelic kicks in TinyMCE editor can not start:

This is an even bigger problem than the one I reported this for, and is likely to have a much more widespread impact.

cwli24 · 2022-07-27T19:56:31Z

@kucaahbe I'm unable to reproduce this issue on a vanilla page's agent for the current (unreleased) version and a production customer's website v1216 agent. I haven't looked into TinyMCE specifically, but a potential cause for conflict as had happened before is other libraries or API implementations that also touches the global Promise object, such as Zone.js for Angular. Would like to table and revisit this after next version release if it's still an issue. Thanks!

cwli24 · 2022-07-27T20:05:14Z

Hey @graemechapman, is there a specific issue that you're running into where our Promise instrumentation is interfering with other code?

Hi @aubreymasten I have a class extending from Promise, when run in conjunction with newrelic the additional methods on these class instances are not accessible.

I have been able to work around this for now by grabbing the original promise constructor from window.NREUM.o.PR but obviously this is not ideal.

Hi @graemechapman, could you clarify a little more about what and where the friction is please? I did not find any issue with calling the instance method of an inherited-from-Promise class--am I misunderstanding?

Specifically, the (spa flavor) agent will wrap around the window.Promise object and alter it; that is the functionality of the object. What problem are you attempting to solve? Do you just want access to the untampered Promise? Or is there some property or method that you cannot find on the altered Promise which should exist? An example would be appreciated, thanks.

kucaahbe · 2022-07-28T16:26:24Z

@kucaahbe I'm unable to reproduce this issue on a vanilla page's agent for the current (unreleased) version and a production customer's website v1216 agent. I haven't looked into TinyMCE specifically, but a potential cause for conflict as had happened before is other libraries or API implementations that also touches the global Promise object, such as Zone.js for Angular. Would like to table and revisit this after next version release if it's still an issue. Thanks!

hello @cwli24 , thanks for the reply. In our case TinyMCE wasn't the cause of the issue: its code uses the Promise.allSettled and that's how we spotted the issue.
On the other hand, I've checked the vanilla page as well and no issues there, so I've checked our site again and found the re-producible case; the issue looks like is some conflict between the NewRelic agent code and ES6 shim library (https://github.com/paulmillr/es6-shim).
This is the test case (please note, didn't place NewRelic agent code snippet there):

<!DOCTYPE html>
<html lang="en">
  <head>
    <meta charset="utf-8">
    <meta http-equiv="X-UA-Compatible" content="IE=edge">
    <meta name="viewport" content="width=device-width, initial-scale=1">

    <title>NewRelic issue #187 test case</title>

    <script>
      if (Promise.allSettled) {
        alert('Promise.allSettled initially exists')
      } else {
        alert('Promise.allSettled does not exist')
      }
    </script>

    <script type="text/javascript">
      NewRelic agent code snippet here
    </script>

    <script src="https://cdn.jsdelivr.net/gh/paulmillr/es6-shim@0.35.6/es6-shim.js"></script>
  </head>

  <body>
    <script>
      console.log('Promise.allSettled:', Promise.allSettled)
      if (Promise.allSettled) {
        alert('Promise.allSettled exists')
      } else {
        alert('Promise.allSettled is overrided')
      }
    </script>
  </body>
</html>

workato-integration · 2022-10-20T16:42:10Z

https://issues.newrelic.com/browse/NEWRELIC-4682

workato-integration · 2022-11-28T20:43:33Z

Jira CommentId: 116274
Commented by phousley:

Since our instrumentation relies on knowing when a promise is created and subsequent methods like then are invoked and whether that invocation succeeds or fails, we have to wrap the native Promise. However, in our doing so, we possibly have created a few issues where 1. window.Promise is no longer being seen as native and 2. other libraries or customer code that extends from window.Promise has issue like what we saw in the https://issues.newrelic.com/browse/NEWRELIC-4683. We are also possibly not correctly wrapping Promise since some of the other methods like allSettled is no longer available.

workato-integration · 2022-11-28T20:50:28Z

Jira CommentId: 116275
Commented by phousley:

We should create a couple test cases:

Compare the static methods of window.NREUM.o._PR and window.Promise and verify those static methods exist

Compare the methods of an instance of window.NREUM.o._PR and window.Promise and verify those methods exist

Create test case that uses handlers with a set context and verify that context is retained within the handler

workato-integration · 2023-01-19T16:42:50Z

Jira CommentId: 138424
Commented by jporter:

Promise methods like 'any' and 'allSettled' are already returned as 'Native Code' when the customer's shim is not loaded into the page, so this bug ticket is actually just an integration issue with whatever shim the reporter was using (and likely others that expect native code). So I am just going to put in a backlog ticket to re-evaluate the way we wrap promise instead of overwriting the global and close this bug ticket.

workato-integration · 2023-01-19T16:47:57Z

Work has been completed on this issue.

samdenty · 2023-02-17T17:11:36Z

This is breaking our application

patrickhousley · 2023-02-17T17:35:18Z

@samdenty Can you look to see what version of the agent is being loaded? Maybe provide a link to your website? I just checked another site that is loading the latest version of our agent (1225) and o instanceof Promise reports true.

samdenty · 2023-02-17T17:41:34Z

new Promise(() => {}) instanceof Promise works
but
(async () => {})() instanceof Promise doesnt

the URL is https://stackblitz.com/~/github.com/withastro/docs (temp disabled until next week when a solution comes up)

cwli24 · 2023-02-17T18:53:49Z

new Promise(() => {}) instanceof Promise works but (async () => {})() instanceof Promise doesnt

the URL is https://stackblitz.com/~/github.com/withastro/docs

Thanks for bringing this up & to our attention. v1225 included a re-working of our Promise wrapping because what we were doing before was directly modifying the native Promise that async/await implements. -- Apologies for causing breakage!

Going forward, our (wrapped) Promise will likely remain that way; we may re-discuss the implementation, though I feel this is the right thing to do. It's very similar to Bluebird's dilemma in that async/await's returned Promise cannot be replaced with our custom Promise.

I'm not sure you're going to get the (revert) solution from this agent, to be frank. However, there are ways on your end to "fix" it:

casting async's native Promise to our Promise using Promise.resolve(asyncFunc())
check it against newrelic.o.PR instead (this is the original Promise object), if instanceof is needed(?)
(obligatory:) using Promise instead of async/await
(last resort:) swap the original Promise back and use it over ours by window.Promise = newrelic.o.PR after agent code -- this will make SPA nonfunctional

samdenty · 2023-02-17T19:29:28Z

we're running a fork of microsoft's vscode codebase directly in the browser. We try to keep modifications to a minimum, so only the last resort option (swapping the original promise back) would work. Maybe we do that hack to keep using new relic? @fvsch

I could imagine that instanceof Promise would be used in third-party scripts that are outside of the control of end-user applications to fix, so would require grepping the source code and re-hosting the script (cause it could be on a cdn)

I'm not personally a fan of overriding globals, I would much prefer this as an option (disabled by default maybe?) - but that's just my opinion 🙂 I'm not deeply familiar with new relic though and what features overriding promise gives

patrickhousley · 2023-02-17T19:43:18Z

@samdenty Do you have a staging environment/site that we can use to debug?

cwli24 · 2023-02-17T20:05:34Z

To clarify, it's not particularly instanceof Promise, but specifically within the context of checking async's return against it that's problematic.
I imagine historically we've swapped out globals for dev convenience so they did not have to change anything vs. replacing all the Promise with something like newrelic.Promise per adoption; this could be subject to change, but I'm afraid at this point it would only offend the vast majority of existing users.

Regardless, the native global is still there (hidden) and left untouched, which to me is better than modifying its prototype, irrespective of the global ref replacement. And unfortunately, that means async is native (since we have no way to wrap the keyword) and its returned promise is native. That's the impasse, and I'm more pressed about
(async()=>{})().then(...)
not using our then than about the instanceof comparison honestly.

samdenty · 2023-02-17T20:21:35Z

I presumed you were already doing this and this bug was an edge case where this didn't work (hence my suggestion to add an option to disable) - but are you defining a Symbol.hasInstance on your custom promise, that returns true when both a native promise and mocked promise is passed

patrickhousley · 2023-02-17T20:46:52Z

@samdenty I am going to open a new issue for this so we can better track it.

graemechapman added bug Something isn't working needs triage labels Feb 17, 2022

aubreymasten added medium Medium Engineering Effort and removed medium Medium Engineering Effort labels Feb 18, 2022

aubreymasten added this to Triage Needed: Unprioritized Features in Browser Instrumentation Engineering Board Mar 15, 2022

aubreymasten added the customer Input label Mar 15, 2022

aubreymasten moved this from Triage Needed: Unprioritized Features to Research Needed in Browser Instrumentation Engineering Board Mar 15, 2022

cwli24 added needs follow up Issue needs follow-up to ensure the problem has been solved by a release, PR, or otherwise. and removed bug Something isn't working needs triage labels Jul 27, 2022

cwli24 added bug Something isn't working and removed needs follow up Issue needs follow-up to ensure the problem has been solved by a release, PR, or otherwise. labels Aug 15, 2022

coreyarnold added the sync2jira label Oct 20, 2022

workato-integration bot closed this as completed Jan 19, 2023

Browser Instrumentation Engineering Board automation moved this from Research Needed to Done: Issues recently completed Jan 19, 2023

patrickhousley mentioned this issue Feb 17, 2023

Agent is not capturing/wrapping native async/await promises #409

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NewRelic overwrites window.Promise #187

NewRelic overwrites window.Promise #187

graemechapman commented Feb 17, 2022

aubreymasten commented Feb 18, 2022

graemechapman commented Feb 19, 2022

kucaahbe commented Jun 29, 2022

graemechapman commented Jun 29, 2022

cwli24 commented Jul 27, 2022

cwli24 commented Jul 27, 2022 •

edited

Loading

kucaahbe commented Jul 28, 2022

workato-integration bot commented Oct 20, 2022

workato-integration bot commented Nov 28, 2022

workato-integration bot commented Nov 28, 2022

workato-integration bot commented Jan 19, 2023

workato-integration bot commented Jan 19, 2023

samdenty commented Feb 17, 2023

patrickhousley commented Feb 17, 2023

samdenty commented Feb 17, 2023 •

edited

Loading

cwli24 commented Feb 17, 2023

samdenty commented Feb 17, 2023 •

edited

Loading

patrickhousley commented Feb 17, 2023

cwli24 commented Feb 17, 2023

samdenty commented Feb 17, 2023 •

edited

Loading

patrickhousley commented Feb 17, 2023

NewRelic overwrites window.Promise #187

NewRelic overwrites window.Promise #187

Comments

graemechapman commented Feb 17, 2022

Description

Steps to Reproduce

Expected Behavior

Relevant Logs / Console output

Your Environment

Additional context

aubreymasten commented Feb 18, 2022

graemechapman commented Feb 19, 2022

kucaahbe commented Jun 29, 2022

graemechapman commented Jun 29, 2022

cwli24 commented Jul 27, 2022

cwli24 commented Jul 27, 2022 • edited Loading

kucaahbe commented Jul 28, 2022

workato-integration bot commented Oct 20, 2022

workato-integration bot commented Nov 28, 2022

workato-integration bot commented Nov 28, 2022

Compare the static methods of window.NREUM.o._PR and window.Promise and verify those static methods exist

Compare the methods of an instance of window.NREUM.o._PR and window.Promise and verify those methods exist

Create test case that uses handlers with a set context and verify that context is retained within the handler

workato-integration bot commented Jan 19, 2023

workato-integration bot commented Jan 19, 2023

samdenty commented Feb 17, 2023

patrickhousley commented Feb 17, 2023

samdenty commented Feb 17, 2023 • edited Loading

cwli24 commented Feb 17, 2023

samdenty commented Feb 17, 2023 • edited Loading

patrickhousley commented Feb 17, 2023

cwli24 commented Feb 17, 2023

samdenty commented Feb 17, 2023 • edited Loading

patrickhousley commented Feb 17, 2023

cwli24 commented Jul 27, 2022 •

edited

Loading

samdenty commented Feb 17, 2023 •

edited

Loading

samdenty commented Feb 17, 2023 •

edited

Loading

samdenty commented Feb 17, 2023 •

edited

Loading