Cater for platform-specific limitations #540
Conversation
|
I think that crash tests as part of web-platform-tests would make good sense, especially when multiple browsers have had the same bug. We already have a few tests with "crash" in the filename:
Presumably, some crashes would not reproduce if testharness.js is included, so maybe a new test type will be needed eventually. However, maybe it's fine to write crash tests using testharness.js where it's possible. I'd suggest naming them *-crash.html for now, so that it's possible to find them if we later want to impose some kind of structure on this. |
|
Strongly agree we should keep the crashing tests. If we're not going to standardize the limits (which I think might be a good idea?), then I guess those tests would pass whether or not an exception is thrown/the promise is rejected. We should perhaps standardize the exception? |
|
Yeah, if an exception is thrown, then I'd expect a spec to say what kind. |
|
Looks like it's going to be a TypeError for all cases: That's fine with me. |
|
@molly26dalton, could you kindly please check if this works with the Edge team? |
index.html
Outdated
| @@ -2904,6 +2904,16 @@ | |||
| result that would be obtained by the specification's algorithms. | |||
| </p> | |||
| <p> | |||
| User agents impose implementation-specific limits on otherwise | |||
There was a problem hiding this comment.
I don't think we need MAY here. It's a statement of fact that implementations do this.
There was a problem hiding this comment.
Well they are non-conforming unless you allow it with a "may". :-)
index.html
Outdated
| platform-specific limitations. When an input exceeds | ||
| implementation-specific limit, the user agent MUST throw, or, in the | ||
| context of a promise, reject with, a <a>TypeError</a> optionally | ||
| informing the developer of how a particular input exceeded a |
|
I've created the first set of crash tests: Still missing PaymentDetailsModifier tests. Probably made a couple of copy/pasta errors - but would appreciate some feedback. @rsolomakhin, I've reduced the length of inputs on the constructor tests to 1024 or less: Can you pease take a look. Chrome seems happy tho 👍 |
|
LGTM, thx @marcoscaceres |
|
I'm going to move forward on merging this, but would appreciate finding additional tests owners to help with web platform tests (again, huge thanks to @rsolomakhin for getting the testing started and the tests Google provided thus far - they have been extremely helpful!). On the Mozilla side, the more tests I add, the more issues we keep finding - so would really appreciate more help from other browser vendors and other interested parties. I'm not feeling comfortable anymore moving this to CR until we have a complete test suite. It's revealing contentious problems (e.g., #541 and #536). |
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: w3c/payment-request#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}
Through newly added web platform tests, we discovered that it was possible to crash various browsers through unreasonably large inputs (e.g., unrealistically large currency values of sizes of 10102). Or absurdly sized payment request
ids values.This differs across platforms, because of various platform specific limits. E.g., string size limits are different on Android vs on desktop platforms etc.
As such, naturally, user agents will need to guard against unreasonably-large inputs (hence this PR).
Accompanying this change, we will change the web platform tests to limit inputs to a size of 1024 characters. That's large enough to accommodate must ridiculous values (again, e.g., a currency being 1024 characters long), but not risk hitting platform specific limits.
What we might do, is include these stress tests nonetheless, but just have them in try/catch blocks, to ensure the browser doesn't crash - but make no assumption that browsers will be able to handle these large values.
@foolip, @jgraham, @zcorpan, would appreciate your input. Does the change to the web platform tests sound reasonable? Is there precedence we should follow?
N.B.: Firefox and Chrome have been patched or are being patched to fix the crashes. But more evil tests to come! 😈
Preview | Diff