Skip to content

Conversation

oguzkocer
Copy link
Contributor

Fixes #10494. (maybe) To quote @mzorz from the previous PR:

This is a tricky issue.

@mzorz did a great job explaining the issue in both the original issue and its PR and for quite a while I couldn't even come up with a theory for how we were still having this crash. Now, I have a theory, but it might be a stupid one and a little far fetched. What if the issue is related to Sentry setup?

When we enable the crash logging Sentry might be accessing methods from android.webkit which means we won't have the WebView data directory suffix setup before it. I am not sure how best we can check if that's the case, but I think it's reasonable for Sentry to setup network stuff since they need to send the crash reports.

Now the tricky bit is that, if we make the suggested change in this PR and the app crashes in setWebViewDataDirectorySuffixOnAndroidP we won't be aware of it and related crash reports might disappear. I feel it's very unlikely for that to be the cause of this crash or to result in a crash itself, but it's a risk we'd have to take.

It'd be great to get opinions of a few people for this. What do you think @mzorz @maxme @jkmassel ?

To test:

  • Make sure the app builds and runs OK since we don't know how to reproduce the issue.

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered adding accessibility improvements for my changes.
  • I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

@oguzkocer oguzkocer added this to the 14.2 milestone Feb 3, 2020
@oguzkocer oguzkocer requested review from jkmassel, maxme and mzorz February 3, 2020 18:36
@peril-wordpress-mobile
Copy link

You can test the changes on this Pull Request by downloading the APK here.

@mzorz
Copy link
Contributor

mzorz commented Feb 4, 2020

Thanks for the ping! I think this is worth trying @oguzkocer 👍

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

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

I'm ok with this change and can't imagine why it'd break anything – that said, is this a bug that should be fixed in the Sentry SDK? 🤔

We could submit a patch if so – if I'm understanding right, this seems like a quite unintended side effect?

@oguzkocer
Copy link
Contributor Author

is this a bug that should be fixed in the Sentry SDK?

I don't think so. @mzorz would probably have a better idea, but I think we as the client are the responsible party for ensuring that no two processes try to access the webview data. The way Google implemented this still feels a bit weird to me and I am not even sure this change will fix the issue. If it does, I don't think there are any further actions we need to take.

@jkmassel
Copy link
Contributor

jkmassel commented Feb 5, 2020

The way Google implemented this still feels a bit weird to me

💯

@mzorz
Copy link
Contributor

mzorz commented Feb 5, 2020

I think we as the client are the responsible party for ensuring that no two processes try to access the webview data
[...]
I am not even sure this change will fix the issue. If it does, I don't think there are any further actions we need to take

I think the same, it's up to the client to resolve and this apparently is something we can try without causing further harm

@oguzkocer
Copy link
Contributor Author

@mzorz @jkmassel Thank you for the review!

@oguzkocer oguzkocer merged commit 4cb3d5c into develop Feb 6, 2020
@oguzkocer oguzkocer deleted the try/fixing-multi-process-runtimeexception branch February 6, 2020 22:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

RuntimeException: Unable to create application org.wordpress.android.WordPress: java.lang.RuntimeException: Using W...

3 participants