-
Notifications
You must be signed in to change notification settings - Fork 116
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
Caused by java.lang.NumberFormatException in CleaningUtils #805
Comments
I've been investigating this one trying to reproduce with the logs we have on current The logs say
So, when opening the post and checking for id 34 I can see this content:
I pasted it on my own site and I cannot make it crash as apparently there's nothing weird in there - one thing we may be missing is for example, there may be a possibility that they're opening a locally modified copy of Post with ID==34, but the data we're seeing online is not the same as what they have locally on their device. But looking further, even totally empty posts (with no revision history) showed the failure. Further investigation make me think it's an issue internal to Jsoup, given at the line the crash happens it is only being initialized (without being passed any content yet). Apparently the implementation had some changes in jhy/jsoup@f3035ca#diff-2ae1d9d6a9f9ee450c8a12116dc76658 which is a later version of Jsoup, and it has coincidentally been updated in Aztec as per this commit c0ed37e#diff-c197962302397baf3a4cc36463dce5ea right after the crash started to appear in our app so, I suggest to wait and see if the problem continues to appear in the next WPAndroid beta and alpha. |
I spent some time over the weekend trying to reproduce the problem with no luck. |
Thanks for looking into it @mzorz and @daniloercoli ;). It seems jsoup version 1.11.3 has been released in April last year and since it's a widely used library I believe they'd have known about an internal issue by now. Wdyt? Can't it be related to the recent fix of the #1 crash. There are two things which seems suspicious
Wdyt? |
Sure, I suspect the problem is in the following part of the code |
Thanks, @malinajirka! Yes, that's right, this first appeared in 12.1-rc-3 which included these changes (the crash fix was one of them): |
Definitely I didn't mean the problem would have to also be widely spread and thus noticed elsewhere other than WPAndroid, but rather that it was "contained" within the jsoup parser and also that it is specific to the nested b tags cleaner. |
Thankfully, the crash rate has not grown over the weekend. As it stands I would be happy to cautiously push forward with the 12.1 release, while watching very carefully that this doesn't get out of control.
Would it be possible to do this prior to the release today? Hopefully, that would allow us to fix it more quickly. |
Sure thing let's try adding the logger to gather info 👍 |
Logging the content of the editor is not possible for privacy reasons without additional checks that happen when the editor is fully initialized (We're logging it only for public sites and for public posts). The changes required to make the logging correctly happen in the utils routines (that are called before the editor is up and running) are not that simple to implement them right before the cut. We can add a
|
Thanks, @daniloercoli and @mzorz. I would prefer not to catch and hide the crash. Without I fix, I think it is best to let the crash happen. Let's watch the trend and implement the logging in an upcoming release? |
Ok sounds good @jtreanor! Thank you for looking into this @daniloercoli |
Thank you all for looking into it! ;) Just a quick note: What about logging the exception in the catch block with |
Sounds good! Would you like to submit a PR with that, given the other PR has already been merged? |
I have been quite busy lately. I will be happy to do it, but it might take a while before I get to it. I want to fully understand the context and make sure we actually end up in a valid state if we just consume the exception (and log it). (if anyone else has time and wants to do it, please feel free). |
Don't worry then! I feel at this point we should only gather more info (logging) rather than modifying behavior, for the same reasons you outlined. 👍 |
Thanks everyone for the input on this. 12.1 has been rolling out gradually since Monday and the good news is that this has not been a widespread issue as of yet. The additional logging will help us learn more as this happens again but I don't think we need to do anything else urgently here. |
any updates about that? |
Any updates about this issue? We have faced this issue in the 1.13.1 version too. |
This crash first appeared in 12.1-rc-3 and appears to be increasing in frequency:
5ca33cb5f8b88c296305cf94-fabric
The text was updated successfully, but these errors were encountered: