[GitHub] cordova-plugin-inappbrowser pull request #215: CB-12560: (android) fix null ...
GitHub user dblood opened a pull request: https://github.com/apache/cordova-plugin-inappbrowser/pull/215 CB-12560: (android) fix null pointer with callback when loading multi⦠### Platforms affected Android ### What does this PR do? When multiple urls are requested sequentially (such as a monitor script), the object reference to callback could be nulled by another thread causing a NullPointerException. ### What testing has been done on this change? I have run the automated tests, and successfully ran the application without the crash. ### Checklist - [X] [Reported an issue](http://cordova.apache.org/contribute/issues.html) in the JIRA database - [X] Commit message follows the format: "CB-3232: (android) Fix bug with resolving file paths", where CB- is the JIRA ID & "android" is the platform affected. - [X] Added automated test coverage as appropriate for this change. You can merge this pull request into a Git repository by running: $ git pull https://github.com/dblood/cordova-plugin-inappbrowser CB-12560 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/cordova-plugin-inappbrowser/pull/215.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #215 commit fc08376ee1bcced9fd37912c7559a9abe0dc520f Author: Douglas Blood Date: 2017-03-10T00:30:08Z CB-12560: (android) fix null pointer with callback when loading multiple urls --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Github user dblood commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/215 Because the core issue behind this null pointer is threading, having a automated test that would work consistently 100% of the time isn't possible and a flaky test is a bad thing and will waste people's time. So, I haven't added any automated tests but I have run all existing tests. Additionally, I did manual testing (including this change being released in a app without crashing). --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Github user dblood commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/215 Of course. I will work on a test script. The library I was using is https://github.com/IdentityModel/oidc-client-js with type of "id_token token" and monitoring on. This would attempt to request a "token/userinfo" endpoint on the server once a second regardless of if the previous request had completed. This allowed the completion handler of both requests to be executed concurrently. A bit of background for why I didn't do an automated test: 1) Although this issue consistently occurred sometime within 300 seconds for me, that is 300 potential requests (or possibly non) that would fail based on my local environment and servers. 2) Though it would prove a null pointer could happen with the old code, a negative test for an exception is already handled by every other test (if the app didn't crash, there wasn't a null pointer). This could be applied to every method everywhere, with every type of runtime exception, and adds no value. 3) I didn't want to address the bigger issue, and the risk around the handling of cancelling an in-progress request or if the response has completed but the callback hasn't completed yet. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Github user dblood commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/215 I've spent many hours trying to build a manual test in a "clean" environment. When I get to the office on Monday I can recreate it there and start pulling code out until I have a minimal code to reproduce the issue. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Github user dblood commented on the issue: https://github.com/apache/cordova-plugin-inappbrowser/pull/215 I dug through the code quite a bit and think I have found why this was so hard to reproduce for me. (restoring the version I had in my repo instantly caused the problem though). I was initially using cordova-plugin-inappbrowser 1.1.1. In version 1.2.1 (specifically 4d9e4884) the 'sendUpdate(obj, false);' was moved into a 'runOnUiThread' Runnable which ensures that they are run serially, and on the same thread. Although this 'fixed' the common NullPointerException when close() was called twice the sendUpdate function still wasn't correctly guarded, only the single instance of when sendUpdate was being called with 'false'. This pull request 'fixes' contextCallback references used within the same function to ensure another thread can't modify the locally scoped reference during function execution. --- If your project is set up for it, you can reply to this email and have your reply appear on GitHub as well. If your project does not have this feature enabled and wishes so, or if the feature is enabled but not working, please contact infrastructure at infrastruct...@apache.org or file a JIRA ticket with INFRA. --- - To unsubscribe, e-mail: dev-unsubscr...@cordova.apache.org For additional commands, e-mail: dev-h...@cordova.apache.org
