Can reproduce

Thread 51java.lang.OutOfMemoryError: Failed to allocate a 200000016 byte allocation with 25165824 free bytes and 63MB until OOM, max allowed footprint 226886424, growth limit 268435456
at java.lang.StringFactory.newStringFromBytes	(StringFactory.java:81 )
at java.lang.StringFactory.newStringFromBytes	(StringFactory.java:209 )
at org.chromium.mojo.bindings.Decoder.readString	(Decoder.java:135 )
at org.chromium.payments.mojom.PaymentDetails.decode	(PaymentDetails.java:45 )
at org.chromium.payments.mojom.PaymentRequest_Internal$PaymentRequestInitParams.decode	(PaymentRequest_Internal.java:23 )
at org.chromium.payments.mojom.PaymentRequest_Internal$PaymentRequestInitParams.deserialize	(PaymentRequest_Internal.java:5 )
at org.chromium.payments.mojom.PaymentRequest_Internal$Stub.accept	(PaymentRequest_Internal.java:15 )
at org.chromium.mojo.bindings.RouterImpl$HandleIncomingMessageThunk.accept	(RouterImpl.java:21 )
at org.chromium.mojo.bindings.Connector$WatcherCallback.onResult	(Connector.java:32 )
at org.chromium.mojo.system.impl.WatcherImpl.onHandleReady	(WatcherImpl.java:24 )
at org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce	(Native Method )
at org.chromium.base.SystemMessageHandler.handleMessage	(SystemMessageHandler.java:7 )
at android.os.Handler.dispatchMessage	(Handler.java:105 )
at android.os.Looper.loop	(Looper.java:164 )
at android.app.ActivityThread.main	(ActivityThread.java:6535 )
at java.lang.reflect.Method.invoke	(Native Method )
at com.android.internal.os.Zygote$MethodAndArgsCaller.run	(Zygote.java:240 )
at com.android.internal.os.ZygoteInit.main	(ZygoteInit.java:767 )

Deleted: Test for PaymentRequest Constructor.html 63.8 KB

Test for PaymentRequest Constructor.html 63.8 KB View Download

Crashes Chrome Dev 60.0.3101.4.
Does not crash Chrome Beta 59.0.3071.60.

$ bisect-archived-builds.py -o -u -a arm -g 469221 -b 469814  --unsigned --apk=chromium --chrome-flags="--disable-fre https://w3c-test.org/payment-request/payment-request-constructor.https.html" 
Setting up Catapult in /usr/local/google/home/rouslan/clank/src/clank/tools/catapult_bisect_dep.
Set the environment var CATAPULT_DIR to override Catapult directory.
Updating Catapult...

Already up-to-date.

Done
C   60.505s Main  Bisecting range [469221 (GOOD), 469814 (BAD)].
C   60.508s Main  Downloading build ChromePublic.apk 469517
Revision 469517 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: b
C  131.386s Main  Bisecting range [469221 (GOOD), 469517 (BAD)].
C  131.387s Main  Downloading build ChromePublic.apk 469369
Revision 469369 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: b
C  187.746s Main  Bisecting range [469221 (GOOD), 469369 (BAD)].
C  187.747s Main  Downloading build ChromePublic.apk 469295
Revision 469295 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  248.874s Main  Bisecting range [469295 (GOOD), 469369 (BAD)].
C  248.875s Main  Downloading build ChromePublic.apk 469332
Revision 469332 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  302.306s Main  Bisecting range [469332 (GOOD), 469369 (BAD)].
C  302.306s Main  Downloading build ChromePublic.apk 469350
Revision 469350 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  358.442s Main  Bisecting range [469350 (GOOD), 469369 (BAD)].
C  358.443s Main  Downloading build ChromePublic.apk 469359
Revision 469359 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  421.017s Main  Bisecting range [469359 (GOOD), 469369 (BAD)].
C  421.018s Main  Downloading build ChromePublic.apk 469364
Revision 469364 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  503.297s Main  Bisecting range [469364 (GOOD), 469369 (BAD)].
C  503.298s Main  Downloading build ChromePublic.apk 469366
Revision 469366 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  562.090s Main  Bisecting range [469366 (GOOD), 469369 (BAD)].
C  562.090s Main  Downloading build ChromePublic.apk 469367
Revision 469367 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  615.082s Main  Bisecting range [469367 (GOOD), 469369 (BAD)].
C  615.082s Main  Downloading build ChromePublic.apk 469368
Revision 469368 is [(g)ood/(b)ad/(r)etry/(u)nknown/(q)uit]: g
C  674.450s Main  You are looking for a change made after 469368(GOOD), but before 469369(BAD).

The culprit seems to be https://chromium.googlesource.com/chromium/src/+/dca98e1e37e61a0019ab182bdd7b18748575b6f1

Title: "Verify behavior of PaymentRequest constructor."
Cr-Commit-Position: refs/heads/master@{#469369}

This patch modifies PaymentRequest code. 

The crash on local build:

java.lang.OutOfMemoryError: Failed to allocate a 100000012 byte allocation with 16777216 free bytes and 85MB until OOM
org.chromium.mojo.bindings.Decoder.readBytes(Decoder.java:358)
org.chromium.mojo.bindings.Decoder.readString(Decoder.java:538)
org.chromium.payments.mojom.PaymentDetails.decode(PaymentDetails.java:116)
org.chromium.payments.mojom.PaymentRequest_Internal$PaymentRequestInitParams.decode(PaymentRequest_Internal.java:382)
org.chromium.payments.mojom.PaymentRequest_Internal$PaymentRequestInitParams.deserialize(PaymentRequest_Internal.java:336)
org.chromium.payments.mojom.PaymentRequest_Internal$Stub.accept(PaymentRequest_Internal.java:199)
org.chromium.mojo.bindings.RouterImpl.handleIncomingMessage(RouterImpl.java:238)
org.chromium.mojo.bindings.RouterImpl.access$000(RouterImpl.java:21)
org.chromium.mojo.bindings.RouterImpl$HandleIncomingMessageThunk.accept(RouterImpl.java:33)
org.chromium.mojo.bindings.Connector.readAndDispatchMessage(Connector.java:209)
org.chromium.mojo.bindings.Connector.readOutstandingMessages(Connector.java:172)
org.chromium.mojo.bindings.Connector.onWatcherResult(Connector.java:152)
org.chromium.mojo.bindings.Connector.access$100(Connector.java:25)
org.chromium.mojo.bindings.Connector$WatcherCallback.onResult(Connector.java:142)
org.chromium.mojo.system.impl.WatcherImpl.onHandleReady(WatcherImpl.java:53)
org.chromium.base.SystemMessageHandler.nativeDoRunLoopOnce(Native Method)
org.chromium.base.SystemMessageHandler.handleMessage(SystemMessageHandler.java:41)
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:77

The following code crashes the browser on Android with out of memory error when trying to initialize the Java String for ID:


new PaymentRequest([{                                                           
  supportedMethods: ['basic-card']
}], {
  id: ''.padStart(100000000, '\n method name \t \n '),
  total: {
    label: 'label',
    amount: {
      currency: 'USD',
      value: '1.00'
    }
  }
});

Zach: What should happen from the API perspective? I suggest we should throw an exception for any string that's longer than, let's say, 1024 characters. We should also limit the number of items in a list and the number items in lists to 1024, IMHO. Would we be able to put this into the spec?

Chris: I assume other Mojo surfaces have the same behavior. How can we prevent the out-of-memory error generically? I suggest that we kill the renderer or drop Mojo messages that would result in OOM errors in Android browser process.

(This does not affect desktop.)

+tsepez and rockot, who know Mojo security better than I do. In old-style IPC, there indeed used to be a limit on message size. Did we lose that? If so, that sounds like a bug.

Can we remove the labels restricting access so I can share with Mozilla folks to get their take?

The same message size limit[1] we had with old IPC is still enforced[2] at the lowest I/O layer within Mojo IPC.

[1] https://cs.chromium.org/chromium/src/content/app/mojo/mojo_init.cc?rcl=0503f5125ed2c1f6b1cd8001b1f83731493d5ea7&l=24
[2] https://cs.chromium.org/chromium/src/mojo/edk/system/channel.cc?rcl=2771709f0236929814545a8738053b227d44a170&l=593

You can CC them to grant access.

Yes, but I'd prefer not to have to do that. I don't see why this bug needs to be restricted? I don't see PII.

Removed restrictions.

Patch with a test that reproduces the mojo problem and fixes the bug by enforcing string length limits in the renderer:

https://chromium-review.googlesource.com/c/516882/

rockot@: The patch is showing OOM error in mojo with 100MB string, whereas mojo_init.cc configures the maximum message length to be 128MB. Seems like we skate by under the radar here.

Yeah the limit for IPC message size has always been 128 MB. That does seem unnecessarily large to me, so we could always reduce it.

Though it seems like there are probably tons of ways to get the browser to allocate 100 MB, so I'm not sure that change would be terribly valuable.

Other implementers of PaymentRequest have agreed to put limits on string length, so no need for changes in Mojo.

Why is this marked RB-Beta?  Using the signature from c#0 there are ~0 crashes of this type in the field*.  I'm all for fixing it, but couldn't RB-Stable work?  We ship M60 beta next Thursday...

https://crash.corp.google.com/browse?q=custom_data.ChromeCrashProto.magic_signature_1.name%3D%27%5BAndroid%20Java%20Exception%5D%20java.lang.OutOfMemoryError%20at%20java.lang.StringFactory.newStringFromBytes(StringFactory.java)%27%20AND%20product.name%3D%27Chrome_Android%27&ignore_case=false&enable_rewrite=true&omit_field_name=&omit_field_value=&omit_field_opt=%3D

Confirmed with rouslan@ offline that RB-Stable is OK.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/473dbadd4ff747e39ff9c538b845d35911daa4a3

commit 473dbadd4ff747e39ff9c538b845d35911daa4a3
Author: Rouslan Solomakhin <rouslan@chromium.org>
Date: Mon Jun 05 16:28:16 2017

Don't crash web payments mojo on very long ID.

Before this patch, invoking PaymentRequest with a 10mln character value
for the "id" parameter would crash the browser on Android. The browser
was running out of memory while trying to allocate a Java String.

This patch adds protective limits of 1024 characters to all strings in
PaymentRequest that will be sent to the browser side. Lists also have
protective limits of 1024 items. The stringified "data" object cannot
exceed 1024*1024 characters in length upon serialization.

After this patch, invoking PaymentRequest with a >1024 character value
for the "id" parameter will throw a TypeError exception in JavaScript.

Spec pull request:
https://github.com/w3c/browser-payment-api/pull/540

Bug:  725744 
Change-Id: I25551e7145d9d8b59d111a89a7aa6c343c45600a
Reviewed-on: https://chromium-review.googlesource.com/516882
Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Commit-Position: refs/heads/master@{#476996}
[modify] https://crrev.com/473dbadd4ff747e39ff9c538b845d35911daa4a3/chrome/android/java_sources.gni
[add] https://crrev.com/473dbadd4ff747e39ff9c538b845d35911daa4a3/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestLongIdTest.java
[add] https://crrev.com/473dbadd4ff747e39ff9c538b845d35911daa4a3/chrome/test/data/payments/long_id.js
[add] https://crrev.com/473dbadd4ff747e39ff9c538b845d35911daa4a3/chrome/test/data/payments/payment_request_long_id_test.html
[modify] https://crrev.com/473dbadd4ff747e39ff9c538b845d35911daa4a3/third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-constructor.https.html
[modify] https://crrev.com/473dbadd4ff747e39ff9c538b845d35911daa4a3/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp

Your change meets the bar and is auto-approved for M60. Please go ahead and merge the CL to branch 3112 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), josafat@(ChromeOS), bustamante@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/0c4644806ff9860e9b1f0fa8c119505700d387b7

commit 0c4644806ff9860e9b1f0fa8c119505700d387b7
Author: Rouslan Solomakhin <rouslan@chromium.org>
Date: Mon Jun 05 16:44:38 2017

[Merge M-60] Don't crash web payments mojo on very long ID.

Before this patch, invoking PaymentRequest with a 10mln character value
for the "id" parameter would crash the browser on Android. The browser
was running out of memory while trying to allocate a Java String.

This patch adds protective limits of 1024 characters to all strings in
PaymentRequest that will be sent to the browser side. Lists also have
protective limits of 1024 items. The stringified "data" object cannot
exceed 1024*1024 characters in length upon serialization.

After this patch, invoking PaymentRequest with a >1024 character value
for the "id" parameter will throw a TypeError exception in JavaScript.

Spec pull request:
https://github.com/w3c/browser-payment-api/pull/540

TBR=rouslan@chromium.org

(cherry picked from commit 473dbadd4ff747e39ff9c538b845d35911daa4a3)

Bug:  725744 
Change-Id: I25551e7145d9d8b59d111a89a7aa6c343c45600a
Reviewed-on: https://chromium-review.googlesource.com/516882
Commit-Queue: Rouslan Solomakhin <rouslan@chromium.org>
Reviewed-by: Ganggui Tang <gogerald@chromium.org>
Reviewed-by: Marijn Kruisselbrink <mek@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#476996}
Reviewed-on: https://chromium-review.googlesource.com/524184
Reviewed-by: Rouslan Solomakhin <rouslan@chromium.org>
Cr-Commit-Position: refs/branch-heads/3112@{#152}
Cr-Branched-From: b6460e24cf59f429d69de255538d0fc7a425ccf9-refs/heads/master@{#474897}
[modify] https://crrev.com/0c4644806ff9860e9b1f0fa8c119505700d387b7/chrome/android/java_sources.gni
[add] https://crrev.com/0c4644806ff9860e9b1f0fa8c119505700d387b7/chrome/android/javatests/src/org/chromium/chrome/browser/payments/PaymentRequestLongIdTest.java
[add] https://crrev.com/0c4644806ff9860e9b1f0fa8c119505700d387b7/chrome/test/data/payments/long_id.js
[add] https://crrev.com/0c4644806ff9860e9b1f0fa8c119505700d387b7/chrome/test/data/payments/payment_request_long_id_test.html
[modify] https://crrev.com/0c4644806ff9860e9b1f0fa8c119505700d387b7/third_party/WebKit/LayoutTests/external/wpt/payment-request/payment-request-constructor.https.html
[modify] https://crrev.com/0c4644806ff9860e9b1f0fa8c119505700d387b7/third_party/WebKit/Source/modules/payments/PaymentRequest.cpp