Fix crash in release mode of payment manifest parser.

Fix crash in release mode of payment manifest parser.

Before this patch, the payment manifest parser was populating array
elements only in debug mode via DCHECK(addItemToArray()), which caused a
crash in release mode.

The fix is to save a "success" boolean on stack and DCHECK(success)
later.

After this patch, the release mode is not crashing.

In addition, this patch contains the following minor fixes:

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest uri, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329, 710433
Review-Url: https://codereview.chromium.org/2809193002
Cr-Commit-Position: refs/heads/master@{#463720}
Committed: https://chromium.googlesource.com/chromium/src/+/75be1dd2e3cfbb4f04284246d44328295229d031

[please use gerrit instead, 2017-04-11 16:46:35]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 16:47:12]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2017-04-11 16:56:23]

rouslan@chromium.org changed reviewers:
+ gogerald@chromium.org, mathp@chromium.org, tedchoc@chromium.org

[please use gerrit instead, 2017-04-11 16:56:23]

Hey all,

These are speculative fixes to address a crash that's confusing the heck out of
me. Here's the stack trace:

java.lang.NullPointerException: Attempt to invoke virtual method
'java.lang.String java.net.URI.toString()' on a null object reference
org.chromium.components.payments.PaymentManifestDownloader.getUriString(PaymentManifestDownloader.java:8)
org.chromium.components.payments.PaymentManifestDownloader.nativeDownloadWebAppManifest(Native
Method)
org.chromium.components.payments.PaymentManifestDownloader.downloadWebAppManifest(PaymentManifestDownloader.java:6)
org.chromium.chrome.browser.payments.PaymentManifestVerifier.onPaymentMethodManifestParseSuccess(PaymentManifestVerifier.java:57)
org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
org.chromium.base.SystemMessageHandler.handleMessage(SystemMessageHandler.java:7)
android.os.Handler.dispatchMessage(Handler.java:102)
android.os.Looper.loop(Looper.java:154)
android.app.ActivityThread.main(ActivityThread.java:6119)
java.lang.reflect.Method.invoke(Native Method)
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)

This happens only in Canary. Chromium built from source code does not hit this
condition on the same device and same test page
(https://rsolomakhin.github.io/pr/bob/).

The exception appears to mean that URIs that were parsed out of
https://rsolomakhin.github.io/bobpay/payment-manifest.json are null. However,
payment_manifest_parser_host.cc should be checking for that, so I'm thoroughly
confused how that could happen.

Do you have any suggestions? We cannot ship Android payment apps with this issue
:-(

[Ted C, 2017-04-11 17:23:07]

On 2017/04/11 16:56:23, ಠ_ಠ wrote:
> Hey all,
> 
> These are speculative fixes to address a crash that's confusing the heck out
of
> me. Here's the stack trace:
> 
> java.lang.NullPointerException: Attempt to invoke virtual method
> 'java.lang.String java.net.URI.toString()' on a null object reference
>
org.chromium.components.payments.PaymentManifestDownloader.getUriString(PaymentManifestDownloader.java:8)
>
org.chromium.components.payments.PaymentManifestDownloader.nativeDownloadWebAppManifest(Native
> Method)
>
org.chromium.components.payments.PaymentManifestDownloader.downloadWebAppManifest(PaymentManifestDownloader.java:6)
>
org.chromium.chrome.browser.payments.PaymentManifestVerifier.onPaymentMethodManifestParseSuccess(PaymentManifestVerifier.java:57)
> org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
>
org.chromium.base.SystemMessageHandler.handleMessage(SystemMessageHandler.java:7)
> android.os.Handler.dispatchMessage(Handler.java:102)
> android.os.Looper.loop(Looper.java:154)
> android.app.ActivityThread.main(ActivityThread.java:6119)
> java.lang.reflect.Method.invoke(Native Method)
>
com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:886)
> com.android.internal.os.ZygoteInit.main(ZygoteInit.java:776)
> 
> This happens only in Canary. Chromium built from source code does not hit this
> condition on the same device and same test page
> (https://rsolomakhin.github.io/pr/bob/).
> 
> The exception appears to mean that URIs that were parsed out of
> https://rsolomakhin.github.io/bobpay/payment-manifest.json are null. However,
> payment_manifest_parser_host.cc should be checking for that, so I'm thoroughly
> confused how that could happen.
> 
> Do you have any suggestions? We cannot ship Android payment apps with this
issue
> :-(

I "think" this might be the source/fix:
https://codereview.chromium.org/2816573002

DCHECKs are stripped right, so adding URIs seems to not work in anything but
local
debug builds.

Since this is only a DCHECK as well, you're still looking at the possibility of
null when the URI parse fails.  You could consider using Uri (lowercase ri) as
it is looser in terms of parsing if you're already doing it at the gurl level.

Or if you want to not have nulls and drop them on the floor, you should convert
to using a List instead of an array and only add them when they parse.  But that
only works if you are fine with the length of these lists being dynamic.

[please use gerrit instead, 2017-04-11 17:56:13]

Description was changed from

==========
Minor corrections for payment manifest verifier.

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329
==========

to

==========
Fix crash in release mode of payment manifest parser.

Before this patch, the payment manifest parser was populating array
elements only in debug mode via DCHECK(addItemToArray()), which caused a
crash in release mode.

The fix is to save a "success" boolean on stack and DCHECK(success)
later.

After this patch, the release mode is not crashing.

In addition, this patch contains the following minor fixes:

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest uri, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329
==========

[please use gerrit instead, 2017-04-11 17:56:27]

The CQ bit was checked by rouslan@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-11 17:57:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[please use gerrit instead, 2017-04-11 17:57:34]

Description was changed from

==========
Fix crash in release mode of payment manifest parser.

Before this patch, the payment manifest parser was populating array
elements only in debug mode via DCHECK(addItemToArray()), which caused a
crash in release mode.

The fix is to save a "success" boolean on stack and DCHECK(success)
later.

After this patch, the release mode is not crashing.

In addition, this patch contains the following minor fixes:

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest uri, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329
==========

to

==========
Fix crash in release mode of payment manifest parser.

Before this patch, the payment manifest parser was populating array
elements only in debug mode via DCHECK(addItemToArray()), which caused a
crash in release mode.

The fix is to save a "success" boolean on stack and DCHECK(success)
later.

After this patch, the release mode is not crashing.

In addition, this patch contains the following minor fixes:

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest uri, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329, 710433
==========

[please use gerrit instead, 2017-04-11 17:57:57]

rouslan@chromium.org changed reviewers:
- gogerald@chromium.org, tedchoc@chromium.org

[please use gerrit instead, 2017-04-11 17:57:58]

Mathieu, ptal patch 2.

[Mathieu, 2017-04-11 17:59:34]

lgtm, thanks for the good description

[please use gerrit instead, 2017-04-11 18:00:30]

The CQ bit was unchecked by rouslan@chromium.org

[please use gerrit instead, 2017-04-11 18:00:35]

The CQ bit was checked by rouslan@chromium.org

[commit-bot: I haz the power, 2017-04-11 18:01:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-11 19:23:33]

CQ is committing da patch.

Bot data: {"patchset_id": 20001, "attempt_start_ts": 1491933635272170,
"parent_rev": "d975472ce845091000c8fe68a18f8d93d6b1511d", "commit_rev":
"75be1dd2e3cfbb4f04284246d44328295229d031"}

[commit-bot: I haz the power, 2017-04-11 19:24:27]

Description was changed from

==========
Fix crash in release mode of payment manifest parser.

Before this patch, the payment manifest parser was populating array
elements only in debug mode via DCHECK(addItemToArray()), which caused a
crash in release mode.

The fix is to save a "success" boolean on stack and DCHECK(success)
later.

After this patch, the release mode is not crashing.

In addition, this patch contains the following minor fixes:

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest uri, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329, 710433
==========

to

==========
Fix crash in release mode of payment manifest parser.

Before this patch, the payment manifest parser was populating array
elements only in debug mode via DCHECK(addItemToArray()), which caused a
crash in release mode.

The fix is to save a "success" boolean on stack and DCHECK(success)
later.

After this patch, the release mode is not crashing.

In addition, this patch contains the following minor fixes:

1) Match variable names across JNI boundary.
2) Add an assert for null web app manifest uri, because that should not
   happen.
3) Ensure that payment manifest parser host always invokes all
   callbacks.

BUG=683329, 710433

Review-Url: https://codereview.chromium.org/2809193002
Cr-Commit-Position: refs/heads/master@{#463720}
Committed:
https://chromium.googlesource.com/chromium/src/+/75be1dd2e3cfbb4f04284246d443...
==========

[commit-bot: I haz the power, 2017-04-11 19:24:29]

Committed patchset #2 (id:20001) as
https://chromium.googlesource.com/chromium/src/+/75be1dd2e3cfbb4f04284246d443...