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

Jump to bottom

Bugfix/issue 388 #408

Merged
merged 9 commits into from Mar 10, 2017
Merged

Bugfix/issue 388 #408

merged 9 commits into from Mar 10, 2017

Conversation

askirk
Copy link
Contributor

@askirk askirk commented Jan 31, 2017

Protecting against OOM error reported in issue #388

@askirk
Surround byte array allocation with Try/Catch
4b839b5 
If we catch an OOM error, set JsonSize, JsonData, and BulkData to 0/null.
@askirk
Add Logging for OOM Error
f31fdca
@askirk askirk self-assigned this Jan 31, 2017
joeygrover
joeygrover reviewed Feb 8, 2017
View reviewed changes
Copy link
Member

@joeygrover joeygrover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to need more unit tests. We need to understand what happens if a BFH can't be created from the data provided

joeygrover
joeygrover reviewed Feb 8, 2017
View reviewed changes
sdl_android_lib/src/com/smartdevicelink/protocol/BinaryFrameHeader.java Outdated
msg.setBulkData(_bulkData);
}
} catch (OutOfMemoryError e){
Log.e("SdlProtocol", "OOM error due to junk data in BinaryFrameHeader");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A TAG variable should be created for this with value of BinaryFrameHeader. Also, Error message should be a little more generic. Unable to process data to form header or similar.

askirk and others added 2 commits February 9, 2017 14:31
@askirk
Editing Logging
8b88d16 
Still need to add unit tests or test for actual case
@joeygrover
Fixed TAG in BinaryFrameHeader to be camel case
bf90258
@askirk
Copy link
Contributor Author

askirk commented Feb 10, 2017 •
edited

The method in question, parseBinaryHeader(), is called in 4 places:

  1. https://github.com/smartdevicelink/sdl_android/blob/master/sdl_android_lib/src/com/smartdevicelink/util/DebugTool.java#L308
  2. https://github.com/smartdevicelink/sdl_android/blob/master/sdl_android_lib/src/com/smartdevicelink/protocol/WiProProtocol.java#L283
  3. https://github.com/smartdevicelink/sdl_android/blob/master/sdl_android_lib/src/com/smartdevicelink/protocol/WiProProtocol.java#L431
  4. https://github.com/smartdevicelink/sdl_android/blob/master/sdl_android_lib/src/com/smartdevicelink/transport/SdlRouterService.java#L1414

joeygrover
joeygrover reviewed Feb 21, 2017
View reviewed changes
Copy link
Member

@joeygrover joeygrover left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BFH parsing is now working as expected. However, in the MessageFrameAssembler methods need to check if the returned value was null and simply return if it is.

askirk and others added 2 commits February 22, 2017 10:20
@askirk
Handling a null BFH
3b0e844
@joeygrover
Merge branch 'develop' of https://github.com/smartdevicelink/sdl_android
e15fa2b 
 into bugfix/issue_388

# Conflicts:
#	sdl_android_tests/src/com/smartdevicelink/protocol/WiProProtocolTests.java
@joeygrover joeygrover merged commit 2944eee into develop Mar 10, 2017
@joeygrover joeygrover deleted the bugfix/issue_388 branch March 10, 2017 14:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joeygrover joeygrover joeygrover left review comments

Assignees

@askirk askirk

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@askirk @joeygrover