Limit the size of pref values sent to the child process

RESOLVED FIXED in Firefox 48

Status

()

defect
P1
normal
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mccr8, Assigned: mccr8)

Tracking

({addon-compat})

Trunk
mozilla49
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 wontfix, firefox48 fixed, firefox49 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

Assignee

Description

3 years ago
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.
Assignee

Comment 1

3 years ago
The warning was added in bug 872981, which landed in June 2013.
Assignee

Updated

3 years ago
Keywords: addon-compat
Assignee

Comment 2

3 years ago
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
Assignee

Comment 3

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

Updated

3 years ago
Attachment #8752380 - Attachment is obsolete: true
Assignee

Comment 5

3 years ago
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.
Assignee

Comment 6

3 years ago
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.
Assignee

Comment 7

3 years ago
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.
Assignee

Comment 8

3 years ago
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.
Assignee

Updated

3 years ago
Attachment #8752505 - Attachment is obsolete: true
Assignee

Comment 9

3 years ago
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
Attachment #8752608 - Flags: review?(mantaroh)
Assignee

Updated

3 years ago
Attachment #8752382 - Flags: review?(benjamin)

Updated

3 years ago
Attachment #8752382 - Flags: review?(benjamin) → review+
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)
Attachment #8752608 - Flags: review?(mantaroh) → review+

Updated

3 years ago
Priority: -- → P1

Comment 12

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a97d0c36d993
https://hg.mozilla.org/mozilla-central/rev/49b09d2f702f
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee

Comment 13

3 years ago
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.)
Attachment #8752382 - Flags: approval-mozilla-aurora?
Assignee

Comment 14

3 years ago
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.
Attachment #8752608 - Flags: approval-mozilla-aurora?
Comment on attachment 8752382 [details] [diff] [review]
Limit the size of preference values sent to child processes.

Improve addons and e10s, taking it.
Attachment #8752382 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8752608 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Comment 17

3 years ago
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.
Assignee

Comment 18

3 years ago
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.
You need to log in before you can comment on or make changes to this bug.