You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@cordova.apache.org by dblood <gi...@git.apache.org> on 2017/03/10 00:37:00 UTC
[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\u2026
### 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-xxxx 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 <bl...@ospri.co.nz>
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by dblood <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by dblood <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/215
@dblood OK, so what do I need to do to test this thing to verify this works? Do you have a monitor script available somewhere? Please don't check off the last box if there's no way to actually test this, it just gets our hopes up.
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/215
Thank you!
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by infil00p <gi...@git.apache.org>.
Github user infil00p commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/215
What tests have you added?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/215
Yeah, I think that's fine (for not writing an automated test) in this case. It is a very difficult scenario to reproduce - as you mentioned, it may take 5 minutes. But, if we could get together a repro case that we could use to reproduce manually, then I think absolutely it is a valuable thing to spend some time on and get to the bottom of.
Thanks for taking the time :)
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by dblood <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by cordova-qa <gi...@git.apache.org>.
Github user cordova-qa commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/215
Cordova CI Build has completed successfully.
**Commit** - [Link](https://github.com/apache/cordova-plugin-inappbrowser/pull/215/commits/fc08376ee1bcced9fd37912c7559a9abe0dc520f)
**Dashboard** - [Link](http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132/)
| Builder Name | Console Output | Test Report | Device Logs |
| :---: | :---: | :---: | :---: |
| [Windows 10 Store]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=windows-10-store/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=windows-10-store/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=windows-10-store/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=windows-10-store/artifact/) |
| [iOS 9.3]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-9.3/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-9.3/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-9.3/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-9.3/artifact/) |
| [iOS 10.0]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-10.0/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-10.0/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-10.0/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=ios-10.0/artifact/) |
| [Android 4.4]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-4.4/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-4.4/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-4.4/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-4.4/artifact/) |
| [Android 5.1]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-5.1/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-5.1/console) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-5.1/testReport/) | [Link]( http://cordova-ci.cloudapp.net:8080/job/cordova-plugin-inappbrowser-pr/132//PLATFORM=android-5.1/artifact/) |
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by dblood <gi...@git.apache.org>.
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org
[GitHub] cordova-plugin-inappbrowser issue #215: CB-12560: (android) fix null pointer...
Posted by filmaj <gi...@git.apache.org>.
Github user filmaj commented on the issue:
https://github.com/apache/cordova-plugin-inappbrowser/pull/215
@dblood in [CB-12560](https://issues.apache.org/jira/browse/CB-12560) you showed some pretty detailed logs and mentioned a monitor script that leverages inappbrowser to check a URL every second. Could you share this code, or a rough semblance of it, so that we could try to reproduce?
---
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 infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@cordova.apache.org
For additional commands, e-mail: dev-help@cordova.apache.org