Increase the depth limit of JSONParser and IPC serialization from 100 to 200.

Increase the depth limit of JSONParser and IPC serialization from 100 to 200.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages:
  Max depth: go/html-depth
  Iframes found at depth: go/html-iframe-depth

To support 99.9% of pages we need (62 + 35) * 2 == 194 levels of nesting.

BUG=673263
Review-Url: https://codereview.chromium.org/2570113002
Cr-Commit-Position: refs/heads/master@{#442542}
Committed: https://chromium.googlesource.com/chromium/src/+/383b174a3c321fc6963c50515745d559222ba709

[Joao da Silva, 2016-12-13 18:16:53]

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

[commit-bot: I haz the power, 2016-12-13 18:17:30]

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

[Joao da Silva, 2016-12-13 18:20:30]

joaodasilva@chromium.org changed reviewers:
+ jam@chromium.org

[Joao da Silva, 2016-12-13 18:20:30]

Hi jam,

our team hit this while working with some large sites like ebay. I've verified
the fix; I'm not 100% sure about security implications. FWIW blink has a similar
limit set to 1000 and v8 has no recursion limit in JSON.parse (other than ~15k
max call frames.)

Please advise. Thanks!

[commit-bot: I haz the power, 2016-12-13 18:41:38]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 18:41:39]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[Joao da Silva, 2016-12-13 18:52:20]

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

[commit-bot: I haz the power, 2016-12-13 18:53:06]

Dry run: 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, 2016-12-13 19:37:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-13 19:37:59]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Joao da Silva, 2016-12-14 10:44:41]

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

[commit-bot: I haz the power, 2016-12-14 10:44:57]

Dry run: 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, 2016-12-14 12:14:16]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-12-14 12:14:17]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[Joao da Silva, 2016-12-14 13:20:04]

joaodasilva@chromium.org changed reviewers:
+ bauerb@chromium.org

[Bernhard Bauer, 2016-12-14 13:24:06]

bauerb@chromium.org changed reviewers:
+ rsesek@chromium.org

[Bernhard Bauer, 2016-12-14 13:24:07]

+rsesek, fellow safe_json OWNER and lucky winner of the ipc/SECURITY_OWNERS
lottery :)

[Robert Sesek, 2016-12-14 16:44:48]

An order of magnitude more nesting seems like a lot. How many nesting levels
does the structure you're parsing have?

I think this has come up before and I was hesitant to change the max depth, but
I cannot find that discussion thread anymore...

[Joao da Silva, 2016-12-14 17:16:24]

1000 is a lot more than we need. Ebay hits 111, for example. 200 would suffice
too. WDYT?

[jam, 2016-12-14 17:50:26]

On 2016/12/13 18:20:30, Joao da Silva wrote:
> Hi jam,
> 
> our team hit this while working with some large sites like ebay. I've verified
> the fix; I'm not 100% sure about security implications. FWIW blink has a
similar
> limit set to 1000 and v8 has no recursion limit in JSON.parse (other than ~15k
> max call frames.)
> 
> Please advise. Thanks!

I defer to base/ owners.

[Robert Sesek, 2016-12-14 19:17:58]

On 2016/12/14 17:16:24, Joao da Silva wrote:
> 1000 is a lot more than we need. Ebay hits 111, for example. 200 would suffice
> too. WDYT?

Yes, I'd prefer to be conservative with modifying this limit. I think either
200, or even 150, would be reasonable.

[Joao da Silva, 2016-12-15 13:11:05]

Description was changed from

==========
Increase the depth limit of JSONParser and IPC serialization.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

BUG=673263
==========

to

==========
Increase the depth limit of JSONParser and IPC serialization.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages: go/html-depth

BUG=673263
==========

[Joao da Silva, 2016-12-15 13:18:25]

On 2016/12/14 19:17:58, Robert Sesek wrote:
> On 2016/12/14 17:16:24, Joao da Silva wrote:
> > 1000 is a lot more than we need. Ebay hits 111, for example. 200 would
suffice
> > too. WDYT?
> 
> Yes, I'd prefer to be conservative with modifying this limit. I think either
> 200, or even 150, would be reasonable.

150 sounds good. PTAL

More details:

A max depth of ~125 supports about 99.9% of pages: go/html-depth

(note that each nesting level yields 2 nested dictionaries in the JSON DOM
representation.)

Our training dataset has a lot more pages on the tail end though, because
popular shopping sites like Ebay are overrepresented.

[Robert Sesek, 2016-12-15 17:38:25]

LGTM. Please put in the CL description that we're changing the max limit from
100 to 150, just to be clear in the history.

[Joao da Silva, 2016-12-16 08:24:03]

Description was changed from

==========
Increase the depth limit of JSONParser and IPC serialization.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages: go/html-depth

BUG=673263
==========

to

==========
Increase the depth limit of JSONParser and IPC serialization from 100 to 150.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages: go/html-depth

BUG=673263
==========

[Joao da Silva, 2016-12-16 09:46:24]

joaodasilva@chromium.org changed reviewers:
+ thakis@chromium.org
- bauerb@chromium.org

[Joao da Silva, 2016-12-16 09:55:42]

Description was changed from

==========
Increase the depth limit of JSONParser and IPC serialization from 100 to 150.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages: go/html-depth

BUG=673263
==========

to

==========
Increase the depth limit of JSONParser and IPC serialization from 100 to 200.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages:
  Max depth: go/html-depth
  Iframes found at depth: go/html-iframe-depth

To support 99.9% of pages we need (62 + 35) * 2 == 194 levels of nesting.

BUG=673263
==========

[Joao da Silva, 2016-12-16 09:58:35]

PTAL

John: approval for ipc/

Nico: approval for base/

Robert: I've increased the limit to 200 because I didn't account for iframe
nesting before. With 194 levels we lose only 0.1% of training pages.

[jam, 2016-12-19 18:16:10]

On 2016/12/16 09:58:35, Joao da Silva wrote:
> PTAL
> 
> John: approval for ipc/
> 
> Nico: approval for base/

lgtm

> 
> Robert: I've increased the limit to 200 because I didn't account for iframe
> nesting before. With 194 levels we lose only 0.1% of training pages.

[Joao da Silva, 2017-01-09 08:27:18]

@thakis ping

[Nico, 2017-01-09 15:06:03]

base/ lgtm

https://codereview.chromium.org/2570113002/diff/120001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2570113002/diff/120001/base/json/json_parser....
base/json/json_parser.cc:27: const int kStackMaxDepth = 200;
maybe add a comment justifying the number ("chosen to support 99.9% of 
documents found in the wild late 2016" of something like that)

[Joao da Silva, 2017-01-10 08:49:12]

https://codereview.chromium.org/2570113002/diff/120001/base/json/json_parser.cc
File base/json/json_parser.cc (right):

https://codereview.chromium.org/2570113002/diff/120001/base/json/json_parser....
base/json/json_parser.cc:27: const int kStackMaxDepth = 200;
On 2017/01/09 15:06:03, Nico wrote:
> maybe add a comment justifying the number ("chosen to support 99.9% of 
> documents found in the wild late 2016" of something like that)

Done.

[Joao da Silva, 2017-01-10 08:49:19]

The CQ bit was checked by joaodasilva@chromium.org

[Joao da Silva, 2017-01-10 08:49:19]

The patchset sent to the CQ was uploaded after l-g-t-m from rsesek@chromium.org,
thakis@chromium.org, jam@chromium.org
Link to the patchset: https://codereview.chromium.org/2570113002/#ps140001
(title: "added comment")

[commit-bot: I haz the power, 2017-01-10 08:49:26]

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-01-10 09:55:40]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484038159295040,
"parent_rev": "cdb6d2b9b6f94fb4ed02c0b315ad56c5bc5b5b89", "commit_rev":
"383b174a3c321fc6963c50515745d559222ba709"}

[commit-bot: I haz the power, 2017-01-10 09:56:10]

Description was changed from

==========
Increase the depth limit of JSONParser and IPC serialization from 100 to 200.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages:
  Max depth: go/html-depth
  Iframes found at depth: go/html-iframe-depth

To support 99.9% of pages we need (62 + 35) * 2 == 194 levels of nesting.

BUG=673263
==========

to

==========
Increase the depth limit of JSONParser and IPC serialization from 100 to 200.

This impacts extension APIs and the debug protocol. Some webpages
have over 100 levels of nested elements. Some internal representations
further use 2 levels per node to represent the DOM data (e.g. one
DictionaryValue for the node and a ListValue containing the children.)

The affected APIs fail silently when this limit is reached.

Percentiles over ~90M random pages:
  Max depth: go/html-depth
  Iframes found at depth: go/html-iframe-depth

To support 99.9% of pages we need (62 + 35) * 2 == 194 levels of nesting.

BUG=673263

Review-Url: https://codereview.chromium.org/2570113002
Cr-Commit-Position: refs/heads/master@{#442542}
Committed:
https://chromium.googlesource.com/chromium/src/+/383b174a3c321fc6963c50515745...
==========

[commit-bot: I haz the power, 2017-01-10 09:56:11]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/383b174a3c321fc6963c50515745...