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.

Sign up for GitHub

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

fix(screenshots): Don't capture zero size screenshots #2459

Merged
merged 10 commits into from
Dec 5, 2022
Merged

fix(screenshots): Don't capture zero size screenshots #2459

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ed15636
fix(screenshots): Don't capture zero size screenshots
krystofwoldrich Nov 29, 2022
7d9c595
Update changelog with pr id
krystofwoldrich Nov 29, 2022
d8f9d5f
Update Sources/Sentry/SentryScreenshot.m
krystofwoldrich Nov 29, 2022
49f2f84
Merge branch 'master' into fix-dont-capture-zero-size-screenshots
brustolin Nov 29, 2022
645d469
Format code
getsentry-bot Nov 29, 2022
9a77f8a
Merge branch 'master' into fix-dont-capture-zero-size-screenshots
brustolin Nov 29, 2022
6022f6a
Merge branch '8.0.0' into fix-dont-capture-zero-size-screenshots
philipphofmann Dec 1, 2022
c8a2689
Add test
philipphofmann Dec 1, 2022
8b99397
fix changelog
philipphofmann Dec 1, 2022
15331d3
Merge remote-tracking branch 'origin/8.0.0' into fix-dont-capture-zer…
krystofwoldrich Dec 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ This version adds a dependency on Swift.
- Increase `SentryCrashMAX_STRINGBUFFERSIZE` to reduce the instances where we're dropping a crash due to size limit (#2465)
- `SentryAppStateManager` correctly unsubscribes from `NSNotificationCenter` when closing the SDK (#2460)
- The SDK no longer reports an OOM when a crash happens after closing the SDK (#2468)
- Don't capture zero size screenshots ([#2459](https://github.com/getsentry/sentry-cocoa/pull/2459))
- Use the preexisting app release version format for profiles (#2470)

### Breaking Changes
Expand Down
7 changes: 6 additions & 1 deletion Sources/Sentry/SentryScreenshot.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,12 @@ - (void)saveScreenShots:(NSString *)path

if ([window drawViewHierarchyInRect:window.bounds afterScreenUpdates:false]) {
UIImage *img = UIGraphicsGetImageFromCurrentImageContext();
[result addObject:UIImagePNGRepresentation(img)];
if (img.size.width > 0 || img.size.height > 0) {
Copy link
Member

Choose a reason for hiding this comment

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

m: Can we please add tests for this new functionality? As this code is not really testable you could extract this code into a method, expose it via a private category that you would have to add similar as we have it, for example, for SentryClient+Private.h, and then call it from tests. I can also push the basic solution to your branch if you want @krystofwoldrich.

Copy link
Member Author

Choose a reason for hiding this comment

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

You can push the solution to this branch and I will work with that. It will definitely help me.

Besides exposing it with the private header. Are there any examples in the test on how to mock functions, likely I will have to mock UIGraphicsGetImageFromCurrentImageContext.

Copy link
Member

Choose a reason for hiding this comment

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

The code was already testable. I added a test.

NSData *bytes = UIImagePNGRepresentation(img);
if (bytes && bytes.length > 0) {
Comment on lines +51 to +52
Copy link
Member

Choose a reason for hiding this comment

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

l: If the image has 0 height or width, it's already empty. I think we don't need to check for bytes && bytes.length > 0 here.

Copy link
Contributor

Choose a reason for hiding this comment

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

Its not the size of the view that makes the image 0 length, its not being able to render the image.

[result addObject:bytes];
}
}
}

UIGraphicsEndImageContext();
Expand Down
11 changes: 11 additions & 0 deletions Tests/SentryTests/SentryScreenShotTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,18 @@ class SentryScreenShotTests: XCTestCase {

XCTAssertEqual(image?.size.width, 10)
XCTAssertEqual(image?.size.height, 10)
}

func test_ZeroSizeScreenShot_GetsDiscarded() {
let testWindow = TestWindow(frame: CGRect(x: 0, y: 0, width: 0, height: 0))
fixture.uiApplication.windows = [testWindow]

guard let data = self.fixture.sut.appScreenshots() else {
XCTFail("Could not make window screenshot")
return
}

XCTAssertEqual(0, data.count, "No screenshot should be taken, cause the image has zero size.")
}

class TestSentryUIApplication: SentryUIApplication {
Expand Down