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

[Android] Fix invalid ImageBrush stack overflow with delayed im… #1570

Merged
merged 2 commits into from
Sep 24, 2019

Conversation

jeromelaban
Copy link
Member

GitHub Issue (If applicable): unoplatform/Uno.WindowsCommunityToolkit#59

PR Type

What kind of change does this PR introduce?

  • Bugfix

What is the current behavior?

Using an ImageBrush on a shape may result in a stack overflow if the image has already been loaded.

What is the new behavior?

Reusing a image does not crash the app.

PR Checklist

Please check if your PR fulfills the following requirements:

  • Tested code with current supported SDKs
  • Docs have been added/updated which fit documentation template. (for bug fixes / features)
  • Unit Tests and/or UI Tests for the changes have been added (for bug fixes / features) (if applicable)
  • Wasm UI Tests are not showing unexpected any differences. Validate PR Screenshots Compare Test Run results.
  • Contains NO breaking changes
  • Updated the Release Notes
  • Associated with an issue (GitHub or internal)

Other information

Internal Issue (If applicable):

@jeromelaban jeromelaban added the ready-to-merge Automatically merge the PR once all '.mergify.yml' policies are met label Sep 23, 2019
@ghuntley ghuntley added platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform kind/bug Something isn't working labels Sep 24, 2019
@@ -0,0 +1,5 @@
<?xml version="1.0" encoding="utf-8"?>
<network-security-config>
<base-config cleartextTrafficPermitted="true" >
Copy link
Member

@ghuntley ghuntley Sep 24, 2019

Choose a reason for hiding this comment

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

I don't think we should be shipping with insecure defaults. How about we change the UI test to use https:// instead of http:// and then burn this file + related changes.

Copy link
Member Author

@jeromelaban jeromelaban Sep 24, 2019

Choose a reason for hiding this comment

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

We're not shipping this app. The point of this modification is to restore the many samples that use this kind of urls, and there are many that are not tested properly after the upgrade to Android 9.0.

I'll move this change to another PR, as there are about 270 Urls that need to be validated to determine if they do work in https.

@@ -2,6 +2,6 @@
<manifest xmlns:android="http://schemas.android.com/apk/res/android" package="uno.platform.unosampleapp" android:versionCode="1" android:versionName="1.0" android:installLocation="auto">
<uses-sdk android:minSdkVersion="16" android:targetSdkVersion="28" />
<uses-permission android:name="android.permission.VIBRATE" />
<application android:label="SamplesApp"></application>
<application android:label="SamplesApp" android:networkSecurityConfig="@xml/network_security_config"></application>
Copy link
Member

Choose a reason for hiding this comment

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

🔥 ?

Copy link
Member

@ghuntley ghuntley left a comment

Choose a reason for hiding this comment

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

I don't think we should ship templates that enable http:// by default.

@ghuntley ghuntley added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. and removed ready-to-merge Automatically merge the PR once all '.mergify.yml' policies are met labels Sep 24, 2019
Copy link
Member

@ghuntley ghuntley left a comment

Choose a reason for hiding this comment

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

<3 thx for fixing.

@ghuntley ghuntley changed the title [Android] Fix invalid ImageBrush stack overflow with delayed image reuse [Android] Fix invalid ImageBrush stack overflow with delayed im… Sep 24, 2019
@ghuntley ghuntley merged commit a3e899c into master Sep 24, 2019
@ghuntley ghuntley deleted the dev/jela/v2-0 branch September 24, 2019 21:24
@ghuntley ghuntley removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working platform/android 🤖 Categorizes an issue or PR as relevant to the Android platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants