fix: Fix adblock memory leak (#877)

Co-authored-by: Patrick Housley <[email protected]>

Author: Joe-Degler

src/common/drain/drain.js

@@ -41,13 +41,14 @@ function curateRegistry (agentIdentifier) {
  * its own named group explicitly, when ready.
  * @param {string} agentIdentifier - A unique 16 character ID corresponding to an instantiated agent.
  * @param {string} featureName - A named group into which the feature's buffered events are bucketed.
+ * @param {boolean} force - Whether to force the drain to occur immediately, bypassing the registry and staging logic.
  */
-export function drain (agentIdentifier = '', featureName = 'feature') {
+export function drain (agentIdentifier = '', featureName = 'feature', force = false) {
   curateRegistry(agentIdentifier)
   // If the feature for the specified agent is not in the registry, that means the instrument file was bypassed.
   // This could happen in tests, or loaders that directly import the aggregator. In these cases it is safe to
   // drain the feature group immediately rather than waiting to drain all at once.
-  if (!agentIdentifier || !registry[agentIdentifier].get(featureName)) return drainGroup(featureName)
+  if (!agentIdentifier || !registry[agentIdentifier].get(featureName) || force) return drainGroup(featureName)
 
   // When `drain` is called, this feature is ready to drain (staged).
   registry[agentIdentifier].get(featureName).staged = true

src/common/drain/drain.test.js

@@ -46,6 +46,15 @@ describe('drain', () => {
     expect(emitSpy).toHaveBeenCalledWith('drain-pumbaa', expect.anything())
   })
 
+  test('immediately drains when force is true', () => {
+    registerDrain('abcd', 'page_view_timing')
+    registerDrain('abcd', 'page_view_event')
+
+    let emitSpy = jest.spyOn(ee.get('abcd'), 'emit')
+    drain('abcd', 'page_view_event', true)
+    expect(emitSpy).toHaveBeenNthCalledWith(1, 'drain-page_view_event', expect.anything())
+  })
+
   test('defaults to "feature" group when not provided one', () => {
     let emitSpy = jest.spyOn(ee.get('abcd'), 'emit')
     drain('abcd')

src/common/event-emitter/contextual-ee.component-test.js

@@ -143,6 +143,19 @@ describe('event-emitter buffer', () => {
     expect(ee.isBuffering(eventType)).toEqual(false)
   })
 
+  test('it should empty the backlog on abort as opposed to replacing it', async () => {
+    const { ee } = await import('./contextual-ee')
+
+    const backlog = {
+      api: ['foo', 'bar', 'baz']
+    }
+    ee.backlog = backlog
+    ee.abort()
+
+    expect(ee.backlog).toBe(backlog)
+    expect(ee.backlog).toEqual({})
+  })
+
   test('it should not buffer after drain', async () => {
     const { ee } = await import('./contextual-ee')
     const { drain } = await import('../drain/drain')

src/common/event-emitter/contextual-ee.js

@@ -144,5 +144,11 @@ function ee (old, debugId) {
 
 function abort () {
   globalInstance.aborted = true
-  globalInstance.backlog = {}
+  // The global backlog can be referenced directly by other emitters,
+  // so we need to delete its contents as opposed to replacing it.
+  // Otherwise, these references to the old backlog would still exist
+  // and the keys will not be garbage collected.
+  Object.keys(globalInstance.backlog).forEach(key => {
+    delete globalInstance.backlog[key]
+  })
 }

src/common/harvest/harvest.js

@@ -142,9 +142,10 @@ export class Harvest extends SharedContext {
 
     if (!opts.unload && cbFinished && submitMethod === submitData.xhr) {
       const harvestScope = this
-      result.addEventListener('load', function () {
+      result.addEventListener('loadend', function () {
         // `this` refers to the XHR object in this scope, do not change this to a fat arrow
-        const cbResult = { sent: true, status: this.status }
+        // status 0 refers to a local error, such as CORS or network failure, or a blocked request by the browser (e.g. adblocker)
+        const cbResult = { sent: this.status !== 0, status: this.status }
         if (this.status === 429) {
           cbResult.retry = true
           cbResult.delay = harvestScope.tooManyRequestsDelay

src/common/harvest/harvest.test.js

@@ -358,14 +358,14 @@ describe('_send', () => {
 
     const result = harvestInstance._send(spec)
     const xhrAddEventListener = jest.mocked(submitDataModule.xhr).mock.results[0].value.addEventListener
-    const xhrLoadHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
+    const xhrLoadEndHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
 
     const xhrState = {
       status: faker.string.uuid()
     }
-    xhrLoadHandler.call(xhrState)
+    xhrLoadEndHandler.call(xhrState)
 
-    expect(xhrAddEventListener).toHaveBeenCalledWith('load', expect.any(Function), expect.any(Object))
+    expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
     expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
     expect(submitMethod).not.toHaveBeenCalled()
     expect(spec.cbFinished).toHaveBeenCalledWith({ ...xhrState, sent: true })
@@ -378,14 +378,14 @@ describe('_send', () => {
 
     const result = harvestInstance._send(spec)
     const xhrAddEventListener = jest.mocked(submitDataModule.xhr).mock.results[0].value.addEventListener
-    const xhrLoadHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
+    const xhrLoadEndHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
 
     const xhrState = {
       status: 429
     }
-    xhrLoadHandler.call(xhrState)
+    xhrLoadEndHandler.call(xhrState)
 
-    expect(xhrAddEventListener).toHaveBeenCalledWith('load', expect.any(Function), expect.any(Object))
+    expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
     expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
     expect(submitMethod).not.toHaveBeenCalled()
     expect(spec.cbFinished).toHaveBeenCalledWith({
@@ -404,14 +404,14 @@ describe('_send', () => {
 
     const result = harvestInstance._send(spec)
     const xhrAddEventListener = jest.mocked(submitDataModule.xhr).mock.results[0].value.addEventListener
-    const xhrLoadHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
+    const xhrLoadEndHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
 
     const xhrState = {
       status: statusCode
     }
-    xhrLoadHandler.call(xhrState)
+    xhrLoadEndHandler.call(xhrState)
 
-    expect(xhrAddEventListener).toHaveBeenCalledWith('load', expect.any(Function), expect.any(Object))
+    expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
     expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
     expect(submitMethod).not.toHaveBeenCalled()
     expect(spec.cbFinished).toHaveBeenCalledWith({
@@ -428,15 +428,15 @@ describe('_send', () => {
 
     const result = harvestInstance._send(spec)
     const xhrAddEventListener = jest.mocked(submitDataModule.xhr).mock.results[0].value.addEventListener
-    const xhrLoadHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
+    const xhrLoadEndHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
 
     const xhrState = {
       status: faker.string.uuid(),
       responseText: faker.lorem.sentence()
     }
-    xhrLoadHandler.call(xhrState)
+    xhrLoadEndHandler.call(xhrState)
 
-    expect(xhrAddEventListener).toHaveBeenCalledWith('load', expect.any(Function), expect.any(Object))
+    expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
     expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
     expect(submitMethod).not.toHaveBeenCalled()
     expect(spec.cbFinished).toHaveBeenCalledWith({
@@ -452,15 +452,15 @@ describe('_send', () => {
 
     const result = harvestInstance._send(spec)
     const xhrAddEventListener = jest.mocked(submitDataModule.xhr).mock.results[0].value.addEventListener
-    const xhrLoadHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
+    const xhrLoadEndHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
 
     const xhrState = {
       status: faker.string.uuid(),
       responseText: faker.lorem.sentence()
     }
-    xhrLoadHandler.call(xhrState)
+    xhrLoadEndHandler.call(xhrState)
 
-    expect(xhrAddEventListener).toHaveBeenCalledWith('load', expect.any(Function), expect.any(Object))
+    expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
     expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
     expect(submitMethod).not.toHaveBeenCalled()
     expect(spec.cbFinished).toHaveBeenCalledWith({
@@ -469,6 +469,28 @@ describe('_send', () => {
       sent: true
     })
   })
+
+  test('should call cbFinished with sent false and status 0 when xhr failed locally', () => {
+    jest.mocked(submitDataModule.getSubmitMethod).mockReturnValue(submitDataModule.xhr)
+    spec.cbFinished = jest.fn()
+
+    const result = harvestInstance._send(spec)
+    const xhrAddEventListener = jest.mocked(submitDataModule.xhr).mock.results[0].value.addEventListener
+    const xhrLoadEndHandler = jest.mocked(xhrAddEventListener).mock.calls[0][1]
+
+    const xhrState = {
+      status: 0
+    }
+    xhrLoadEndHandler.call(xhrState)
+
+    expect(xhrAddEventListener).toHaveBeenCalledWith('loadend', expect.any(Function), expect.any(Object))
+    expect(result).toEqual(jest.mocked(submitDataModule.xhr).mock.results[0].value)
+    expect(submitMethod).not.toHaveBeenCalled()
+    expect(spec.cbFinished).toHaveBeenCalledWith({
+      ...xhrState,
+      sent: false
+    })
+  })
 })
 
 describe('obfuscateAndSend', () => {

src/features/page_view_event/aggregate/index.js

@@ -101,7 +101,7 @@ export class Aggregate extends AggregateBase {
       payload: { qs: queryParameters, body },
       opts: { needResponse: true, sendEmptyBody: true },
       cbFinished: ({ status, responseText }) => {
-        if (status >= 400) {
+        if (status >= 400 || status === 0) {
           // Adding retry logic for the rum call will be a separate change
           this.ee.abort()
           return

src/features/utils/instrument-base.js

@@ -108,7 +108,7 @@ export class InstrumentBase extends FeatureBase {
         warn(`Downloading and initializing ${this.featureName} failed...`, e)
         this.abortHandler?.() // undo any important alterations made to the page
         // not supported yet but nice to do: "abort" this agent's EE for this feature specifically
-        drain(this.agentIdentifier, this.featureName)
+        drain(this.agentIdentifier, this.featureName, true)
         loadedSuccessfully(false)
       }
     }

src/features/utils/instrument-base.test.js

@@ -211,6 +211,7 @@ test('no uncaught async exception is thrown when an import fails', async () => {
 
   expect(warn).toHaveBeenNthCalledWith(2, expect.stringContaining(`Downloading and initializing ${featureName} failed`), expect.any(Error))
   expect(instrument.abortHandler).toHaveBeenCalled()
+  expect(drain).toHaveBeenCalledWith(agentIdentifier, featureName, true)
   await expect(instrument.onAggregateImported).resolves.toBe(false)
   expect(mockOnError).not.toHaveBeenCalled()
 })

src/loaders/api/api.js

@@ -207,7 +207,10 @@ export function setAPI (agentIdentifier, forceDrain) {
     import(/* webpackChunkName: "async-api" */'./apiAsync').then(({ setAPI }) => {
       setAPI(agentIdentifier)
       drain(agentIdentifier, 'api')
-    }).catch(() => warn('Downloading runtime APIs failed...'))
+    }).catch(() => {
+      warn('Downloading runtime APIs failed...')
+      drain(agentIdentifier, 'api', true)
+    })
   }
 
   return apiInterface

tests/assets/adblocker-assets.html

@@ -0,0 +1,21 @@
+<!doctype html>
+<!--
+  Copyright 2020 New Relic Corporation.
+  PDX-License-Identifier: Apache-2.0
+-->
+<html>
+<head>
+  <title>RUM Unit Test</title>
+  {init} {config}
+
+  <script type="text/javascript">
+    // Use proxy to simulate adblock of assets
+    window.NREUM||(NREUM={});NREUM.init=NREUM.init||{};NREUM.init.proxy=NREUM.init.proxy||{};NREUM.init.proxy.assets="http://foobar"
+  </script>
+
+  {loader}
+</head>
+<body>
+This page simulates the agent assets being blocked by using a bad asset proxy setting.
+</body>
+</html>

tests/assets/adblocker-ingest.html

@@ -0,0 +1,33 @@
+<!doctype html>
+<!--
+  Copyright 2020 New Relic Corporation.
+  PDX-License-Identifier: Apache-2.0
+-->
+<html>
+  <head>
+    <script type="text/javascript">
+      var xhrOpen = XMLHttpRequest.prototype.open
+      var xhrSend = XMLHttpRequest.prototype.send
+
+      var urlCache = {}
+      XMLHttpRequest.prototype.open = function () {
+        urlCache[this] = arguments[1]
+        return xhrOpen.apply(this, arguments)
+      }
+      XMLHttpRequest.prototype.send = function () {
+        var url = urlCache[this]
+        var result = xhrSend.apply(this, arguments)
+        if (/\/1\/[\w-_]*\?/.test(url)) {
+          var xhr = this
+          setTimeout(function () { xhr.abort() }, 1)
+        }
+        return result
+      }
+    </script>
+    <title>RUM Unit Test</title>
+    {init} {config} {loader}
+  </head>
+  <body>
+    This page simulates the ingest XHR calls being blocked by aborting the rum call.
+  </body>
+</html>

tests/specs/adblocker.e2e.js

@@ -0,0 +1,72 @@
+import { config } from './session-replay/helpers'
+
+describe('adblocker', () => {
+  beforeEach(async () => {
+    await browser.enableSessionReplay()
+  })
+
+  afterEach(async () => {
+    await browser.destroyAgentSession()
+  })
+
+  it('should abort the global event emitter when rum call is blocked', async () => {
+    await browser.url(await browser.testHandle.assetURL('adblocker-ingest.html', config({ session_replay: { sampling_rate: 100, error_sampling_rate: 100 } })))
+
+    await browser.waitUntil(() => browser.execute(function () {
+      return newrelic.ee.aborted
+    }), {
+      timeoutMsg: 'expected global event emitter to abort'
+    })
+
+    // Simulate adding an item to the event emitter backlog
+    await browser.execute(function () {
+      newrelic.addRelease('foobar', 'bizbaz')
+    })
+
+    const eventEmitterBufferCleared = await browser.execute(function () {
+      var eeCleared = {
+        result: true,
+        errors: []
+      }
+
+      Object.entries(newrelic.ee.backlog).forEach(function (bl) {
+        if (Array.isArray(bl[1]) && bl[1].length > 0) {
+          eeCleared.result = false
+          eeCleared.errors.push('newrelic.ee.backlog[' + bl[0] + '] not cleared: ' + bl[1].length + ' entries')
+        }
+      })
+
+      Object.values(newrelic.initializedAgents).forEach(function (agent) {
+        Object.entries(agent.features).forEach(function (feat) {
+          if (feat[1].ee.backlog !== newrelic.ee.backlog) {
+            eeCleared.result = false
+            eeCleared.errors.push('feature ' + feat[0] + ' backlog is not global backlog')
+          }
+        })
+      })
+
+      return eeCleared
+    })
+
+    expect(eventEmitterBufferCleared.result).toEqual(true)
+    expect(eventEmitterBufferCleared.errors).toEqual([])
+  })
+
+  it('should drain and null out all event emitter buffers when assets fail to load', async () => {
+    await browser.url(await browser.testHandle.assetURL('adblocker-assets.html', config({ session_replay: { sampling_rate: 100, error_sampling_rate: 100 } })))
+
+    await browser.waitUntil(() => browser.execute(function () {
+      var eeBacklogNulled = true
+
+      Object.values(newrelic.ee.backlog).forEach(function (bl) {
+        if (bl !== null) {
+          eeBacklogNulled = false
+        }
+      })
+
+      return eeBacklogNulled
+    }), {
+      timeoutMsg: 'expected global event emitter backlog to be nulled'
+    })
+  })
+})