Skip to content

[NR-485061] Send MobileJSError to new Endpoint#483

Open
ywang-nr wants to merge 15 commits intodevelopfrom
feature/nr485061-JSError
Open

[NR-485061] Send MobileJSError to new Endpoint#483
ywang-nr wants to merge 15 commits intodevelopfrom
feature/nr485061-JSError

Conversation

@ywang-nr
Copy link
Copy Markdown
Contributor

@ywang-nr ywang-nr commented Apr 2, 2026

@ywang-nr ywang-nr changed the title Feature/nr485061 js error [NR-485061] Send MobileJSError to new Endpoint Apr 2, 2026
@ywang-nr ywang-nr requested a review from ndesai-newrelic April 2, 2026 13:00
@ndesai-newrelic
Copy link
Copy Markdown
Contributor

@ywang-nr

Critical Issues

  1. Race condition on jsErrorPayloadString (file I/O + volatile field)
    JSErrorDataController.java:36 — jsErrorPayloadString is volatile but the read-modify-write cycle in sendJSErrorData is not atomic. Multiple concurrent calls can:
  • Read the same cache file simultaneously
  • Overwrite each other's events in jsErrorPayloadString
  • Lose JS error events

The file read at line 64-77 and the write at line 86-87 are unsynchronized. If two JS errors arrive concurrently, one will clobber the other. Consider synchronizing sendJSErrorData or using a lock
around the read-update-write cycle.

  1. NPE in sendJSErrorData when reporter is null
    JSErrorDataController.java:54-57 — JSErrorDataReporter.getInstance() can return null (before initialization or after shutdown), yet line 57 dereferences jsErrorReporter.payloadStore without a null
    check. This will crash.

  2. NPE in getStoredJSErrorData / deleteCacheFile
    Same pattern at lines 116 and 137 — getInstance() return value is dereferenced without null guard.

  3. onHarvestConnected assigns null to volatile field
    JSErrorDataReporter.java:199 — getStoredJSErrorData() can return null, which gets assigned to jsErrorPayloadString. Then in onHarvest() line 207, the != null check passes through to isEmpty() — OK, but
    the field was initialized as "", so assigning null breaks the invariant. Downstream code that relies on non-null will break.


Performance Concerns

  1. Synchronous file I/O on caller thread
    JSErrorDataController.sendJSErrorData reads the cache file synchronously (lines 65-77) on whatever thread the caller is on. If called from the main thread, this blocks the UI. The write is correctly
    offloaded via PayloadController.submitCallable, but the read is not.

  2. Redundant JSON serialization in every harvest cycle
    onHarvest() at line 207-214 converts the entire jsErrorPayloadString to bytes, wraps it in a Payload, stores it, then sends it — every harvest cycle, even if no new errors arrived since the last
    harvest. The onHarvestComplete then deletes the cache and clears the string, but if onHarvest fires before a new error comes in, it will re-send the same data.


Lifecycle Safety

  1. JSErrorDataReporter is never shut down
    PayloadController calls JSErrorDataReporter.initialize() and start() but there's no corresponding shutdown() call in PayloadController.shutdown(). The AgentDataReporter has the same pattern, but the
    new reporter adds harvest listeners (onHarvest, onHarvestComplete, onHarvestConnected) that will keep firing after the reporter should be dead.

8 . Shared PayloadStore between AgentDataReporter and JSErrorDataReporter
Both reporters get agentConfiguration.getPayloadStore() — they share the same store. reportCachedJSErrorData() iterates payloadStore.fetchAll() which returns HEX payloads too. The JSError reporter will
try to send HEX payloads as JSON to the /mobile/errors endpoint, and vice versa.


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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants