Handle null values in MetaData.mergeMaps, preventing potential NPE #386
Conversation
MetaData tab values can be HashMaps, whose entries can be null. MetaData.mergeMaps seemed to assume that the Map was always a ConcurrentHashMap, whose values cannot be null, which could lead to a NPE when merging config.metaData and report.metaData for nullable values, such as activeScreen. Adding a null check and conditionally adding values fixes this behaviour.
fractalwrench
changed the title
| if (baseValue != null | ||
| && baseValue instanceof Map | ||
| && overridesValue instanceof Map) { | ||
| if (baseValue instanceof Map && overridesValue instanceof Map) { |
There was a problem hiding this comment.
baseValue != null is redundant when also using instanceof
|
This was originally discovered when setting the codeBundleId in React Native, which in turn sets metadata on the native layer: bugsnag/bugsnag-react-native#291 |
| metaData.addToTab("foo", "bar", nestedMap); | ||
| nestedMap.put("whoops", null); | ||
|
|
||
| Map<String, Object> mergedMap = MetaData.merge(metaData, metaData).store; |
There was a problem hiding this comment.
Can we merge a different metadata? Its weird to read otherwise.
There was a problem hiding this comment.
I have updated the test to generate two different MetaData objects.
| nestedMap.put("whoops", null); | ||
|
|
||
| Map<String, Object> mergedMap = MetaData.merge(metaData, metaData).store; | ||
| assertFalse(mergedMap.containsKey("whoops")); |
There was a problem hiding this comment.
I think this assertion is incorrect. Can you undo the changes to the sdk and check that this fails as you would expect? I would never expect the whoops key to be in this map from reading the test.
There was a problem hiding this comment.
Without the changes, this test fails due to a NPE within MetaData.mergeMaps, which is expected. I have removed the assertFalse statement as I can see it might be confusing. Were there any additional concerns around the assertions?
…bjects when merging
fractalwrench
dismissed stale reviews from kattrali and bengourley
via
b0a11da
|
@snmaynard I believe I have addressed the comments, are you happy with the updates? |
|
I pinged you with my concerns - we can discuss tomorrow! |
fix(gradle): replaced jcenter with mavenCentral
Goal
Prevent NPE crash when attempting to deliver error reports.
Design
MetaDatatab values can beHashMaps, whose entries can be null.MetaData.mergeMapsseemed to assume that theMapwas always aConcurrentHashMap, whose entries cannot be null.On React Native this manifested as a NPE when merging
config.metaDataandreport.metaDatadue toapp.activeScreenbeing intermittently recorded as null. This can occur on any platform which uses bugsnag-android, however.Changeset
Added a null check when adding the base value to the merged map.
Tests
Wrote unit test + mazerunner scenario, and manually verified that valid reports are sent from a React Native example app using a local bugsnag-android artefact.
Review
For the submitter, initial self-review:
For the pull request reviewer(s), this changeset has been reviewed for: