1272707 - Limit the size of pref values sent to the child process

Status: RESOLVED FIXED

(Core :: Preferences: Backend, defect, P1)

Description

[Andrew McCreight [:mccr8]]

Some users have OOM crashes when starting e10s processes, due to trying to create a very large message for preferences, around 100MB. While this could be due to having a huge number of prefs, I think it is more likely that it is due to some pref values being very large, because I haven't seen any OOM crashes creating the array of preferences to send to the child process.

I'd like to try limiting the size of preferences sent to the child to MAX_ADVISABLE_PREF_LENGTH. This is much simpler than sending the preferences in chunks, which won't even help if the problem is very large preference values, and is anyways just a hacky version of bug 1268662.

Note this won't help if the key name is absurdly gigantic, but maybe that's already filtered out somewhere.

This could break addons that are using preferences in a horrible manner, but if they are being updated enough to run code in the child process, hopefully they'll be updated to work around this.

Comment 1

[Andrew McCreight [:mccr8]]

The warning was added in bug 872981, which landed in June 2013.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Limit the size of preference values sent to child processes.

Don't send any preferences that have a string value that is longer than MAX_ADVISABLE_PREF_LENGTH. This is intended to mitigate OOM issues, as I've seen a parent process crash trying to create a 100mb message to send to the child. Such users likely cannot use e10s at all.

This has a test for all combinations of setting the default and user values of a preference to large or small string values, or not setting them at all.

I manually verified that filtering out preferences reduces the size of the IPC::Message that is sent to the child by printing out the size of the reply message in PContentParent::OnMessageReceived().

I'll wait for the try run to finish before I ask for review.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=11616431d9ca

Comment 3

[Andrew McCreight [:mccr8]]

I'm not sure how useful it is to change the warning message to mention the content process.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: Limit the size of preference values sent to child processes.

Fixed a minor typo.

Comment 5

[Andrew McCreight [:mccr8]]

This breaks the tests in dom/tv/test/mochitest/ (part of e10s M7) because they use a preference to give a big chunk of configuration data to the mock testing service, dom/tv/test/mochitest/mock_data.json. I'll try to figure out how to fix the tests to not do that.

Comment 6

[Andrew McCreight [:mccr8]]

By eliminating most whitespace and shortening the length of the dummy text, I was able to reduce the size enough to fit under the limit, so I may just do that, if it is okay with a person working on the TV stuff.

There's also another more generic testing preference, network.proxy.autoconfig_url, which is 6200 characters long, though as far as I can tell that hasn't caused any test failures. I assume this is because networking is run in the parent process. But this does mean I can't just NS_ASSERTION(false) when we fail to send a pref to the child.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Shrink down TV mock data test file so as to not exceed pref size limit.

The next patch will limit the size of preferences sent to content processes to MAX_ADVISABLE_PREF_LENGTH characters. This reduces the size of mock_data.json by eliminating whitespace and shortening some of the dummy text.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Shrink down TV mock data test file so as to not exceed pref size limit.

The next patch will limit the size of preferences sent to content processes to MAX_ADVISABLE_PREF_LENGTH characters. This patch eliminates whitespace by converting to and from a JSON data structure. In addition, I reduced the size of the names and descriptions in mock_data.json.

Comment 9

[Andrew McCreight [:mccr8]]

Comment on attachment 8752608 [details] [diff] [review]
part 1 - Shrink down TV mock data test file so as to not exceed pref size limit.

The changes to mock_data.json are just making the title and descriptions a little shorter to fit into the preference limit I am adding in this bug. This file hasn't changed since it was first added in December 2015, so hopefully this limit will not cause a lot of problems for people.

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fb188364901d&selectedJob=20857868

Comment 10

[Mantaroh Yoshinaga[:mantaroh]]

Comment on attachment 8752608 [details] [diff] [review]
part 1 - Shrink down TV mock data test file so as to not exceed pref size limit.

(In reply to Andrew McCreight [:mccr8] from comment #9)
> Comment on attachment 8752608 [details] [diff] [review]
> part 1 - Shrink down TV mock data test file so as to not exceed pref size
> limit.
> 
> The changes to mock_data.json are just making the title and descriptions a
> little shorter to fit into the preference limit I am adding in this bug.
> This file hasn't changed since it was first added in December 2015, so
> hopefully this limit will not cause a lot of problems for people.
> 
> try run:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=fb188364901d&selectedJob=20857868
Thanks Andrew!

I confirmed your patch. I added this preference for Fennec tests.(bug 1200133 comment 22)

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a97d0c36d993
https://hg.mozilla.org/integration/mozilla-inbound/rev/49b09d2f702f

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/a97d0c36d993
https://hg.mozilla.org/mozilla-central/rev/49b09d2f702f

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8752382 [details] [diff] [review]
Limit the size of preference values sent to child processes.

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: users with large preferences will be unable to load webpages with e10s enabled
[Describe test coverage new/current, TreeHerder]: this code runs all the time, plus I wrote a test for this.
[Risks and why]: Some risk. This could break addons that use giant prefs, but are also somehow adapted for e10s. On the plus side, this will only affect behavior when e10s is enabled.
[String/UUID change made/needed]: none (There is a string change, but I don't think this is something that is translated. I could be wrong.)

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8752608 [details] [diff] [review]
part 1 - Shrink down TV mock data test file so as to not exceed pref size limit.

Approval Request Comment
This is a test-only change needed for the other patch.

Comment 15

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8752382 [details] [diff] [review]
Limit the size of preference values sent to child processes.

Improve addons and e10s, taking it.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/5e792d50b7a1
https://hg.mozilla.org/releases/mozilla-aurora/rev/29a31af63720

Comment 17

[dagger.bugzilla]

There are a few internal Firefox prefs that use preferences in this "horrible manner", including at least extensions.xpiState, extensions.bootstrappedAddons and devtools.devices.url_cache (all of which are over 4kb for me). There are most likely other prefs that are <4kb on my profile but which can be >4kb and which have no provision for handling the >4kb case (browser.uiCustomization.state?).

Can we guarantee that none of these will ever need to be read in a child process? If not, perhaps we should consider temporarily upping this limit to something more like 128 kb-1 MB until we can get around to fixing these prefs.

Also, the extensions.xpiState case is bug 1117858 which has had no activity in the 18 months since it was first filed; if it takes us that long to not fix large prefs in Firefox itself then I'm not sure how reasonable it is to expect extensions to have done it either.

Comment 18

[Andrew McCreight [:mccr8]]

The first two sound like things that would only be used in the parent process. I'm not sure what devtools.devices.url_cache is, but I don't see any references to url_cache in the code base. Maybe it isn't used any more?

My hope is that any addon that is actively maintained enough to do tricky things in the child process will also be active enough to fix breakage. There haven't been any reports of breakage in the last month since this landed, so hopefully it isn't causing issues.