Closed Bug 1272423 Opened 8 years ago Closed 8 years ago

Message manager should reject large messages

Categories

(Core :: DOM: Content Processes, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
e10s + ---
firefox48 --- wontfix
firefox49 --- wontfix
firefox50 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

Details

Attachments

(1 file, 1 obsolete file)

Once session store is more under control, we should consider making the message manager reject large messages, by throwing a JS exception. You probably should not use it to send 128MB of data at once.

Besides SessionStore another offender for sending lots of data at once appears to be the AddonSDK, with sdk/remote/process/message. Unfortunately we don't have any more visibility into what is being sent from telemetry.

It looks like ublock0 and AdBlockPlus also send messages above 1MB at least sometimes.

This is more of a long-term goal than an immediate priority.
Blocks: e10s-addons
Priority: -- → P1
Olli, what do you think about this idea? Basically, if data.DataLength() >= 128MB in nsFrameMessageManager::SendMessage(), we would just fail instead of trying to send. If we do send, then we'll just end up crashing, as seen in bug 1271102. The drawback of this is that we wouldn't get a crash report, but I guess we could add a separate telemetry for very large messages that were rejected.
Flags: needinfo?(bugs)
Over IRC, Olli said this sounded okay.
Flags: needinfo?(bugs)
Blocks: 1274074
Assignee: nobody → continuation
(In reply to Andrew McCreight [:mccr8] from comment #1)
> Olli, what do you think about this idea? Basically, if data.DataLength() >=
> 128MB in nsFrameMessageManager::SendMessage(), we would just fail instead of
> trying to send. If we do send, then we'll just end up crashing, as seen in
> bug 1271102. The drawback of this is that we wouldn't get a crash report,
> but I guess we could add a separate telemetry for very large messages that
> were rejected.

Could that lead to silent data corruption of any user content?
(In reply to Haik Aftandilian [:haik] from comment #3)
> Could that lead to silent data corruption of any user content?

The message manager is a generic way that JS sends data across processes, so it could cause data loss. For instance, the top source of messages > 1MB is session store, which is saving information about history and tab state. If we drop those messages, it might not be saved. So, that isn't great, but it is probably better than just crashing. I'm going to add telemetry for the messages that we drop due to their large size.
Large IPC messages will hit a fatal assert, so reject message manager
messages earlier. Also, collect telemetry on the large messages.

f? for data collection review
Attachment #8754979 - Flags: feedback?(benjamin)
Comment on attachment 8754979 [details] [diff] [review]
Refuse to send large messages via the message manager.

You need to document what is contained in the key. I'm also a little worried about the message names: are we sure that message names never contain personal data?
Attachment #8754979 - Flags: feedback?(benjamin) → feedback-
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> You need to document what is contained in the key.

The "Names of message manager messages" is the key. Should I more explicitly mention that this is the key?

> I'm also a little worried about the message names: are we sure that message names never contain
> personal data?

MESSAGE_MANAGER_SIZE is already reporting these message names, FYI, so if that's not okay, I'll have to change that, too.  Looking at the existing data, it does reveal some addons you might have installed, as there are messages that start with "AdblockPlus" and "ublock". I don't see anything more identifying that that, and it wouldn't really match how the message manager is usually used, though of course an addon could do anything it wanted to.
What do you think about the message manager names in telemetry? It might be possible to whitelist them or something, but maintaining a whitelist could be troublesome. There's already some message manage name telemetry in 47 (bug 1260908), so if you want me to disable them in beta or whatever I need to do that soon.
Flags: needinfo?(benjamin)
The telemetry payload already includes addon information, so the primary concern is if an addon is including values in what should be a key. That's unlikely by the design of the message manager, but bugs make it possible. It's not a blocker per-se, but it does increase the privacy risk, and so it's something that we should turn off as soon as we don't need it any more (and it probably makes this inappropriate for becoming an opt-out metric).

The histogram description needs to be improved, though: the description should include:
* when the histogram is recorded; is this recorded when the message is sent, or received, or both? Which process(es) is the histogram recorded in?
* You should be explicit about the key format: the key format is the message name passed in to messagemanager.(send|broadcast)AsyncMessage, and doesn't include any extra data about the code or addon which was responsible for sending the message?

How many keys do you expect? As you've seen, keyed histograms with lots of keys tend to blow up the aggregator and so we should perhaps turn off aggregation for this if that's going to be the case.
Flags: needinfo?(benjamin)
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #10)
> It's not a blocker per-se, but it does increase the privacy risk, and so
> it's something that we should turn off as soon as we don't need it any more
> (and it probably makes this inappropriate for becoming an opt-out metric).

That makes sense to me. (I'm assuming "becoming an opt-out metric" is something that wouldn't just happeng automatically.)

Thanks for the detailed comments.

> How many keys do you expect? As you've seen, keyed histograms with lots of
> keys tend to blow up the aggregator and so we should perhaps turn off
> aggregation for this if that's going to be the case.

It is a bit of a mess right now. I was planning on filing a bug about removing trailing digits (maybe all digits?) from the message name which will clean up the mess from ublock. I suppose I'd have to update the description, too.
Depends on: 1275707
Attachment #8754979 - Attachment is obsolete: true
Comment on attachment 8759420 [details] [diff] [review]
Refuse to send large messages via the message manager.

I'll wait a day or two before landing to make sure message manager telemetry isn't spammy, but I might as well put this up for review now.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5db4f853d7e3

f? to bsmedberg for telemetry review
Attachment #8759420 - Flags: review?(amarchesini)
Attachment #8759420 - Flags: feedback?(benjamin)
Comment on attachment 8759420 [details] [diff] [review]
Refuse to send large messages via the message manager.

Review of attachment 8759420 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFrameMessageManager.cpp
@@ +49,5 @@
>  #include "nsPrintfCString.h"
>  #include "nsXULAppAPI.h"
>  #include "nsQueryObject.h"
>  #include <algorithm>
> +#include "chrome/common/ipc_channel.h" // for IPC::Channel::kMaximumMessageSize

alphabetic order. move it up.

@@ +729,5 @@
> +
> +  Telemetry::Accumulate(Telemetry::REJECTED_MESSAGE_MANAGER_MESSAGE,
> +                        messageName);
> +
> +  return false;

Are we sure we want to do this in production too? And not maybe just in debug builds first?
Attachment #8759420 - Flags: review?(amarchesini) → review+
(In reply to Andrea Marchesini (:baku) from comment #14)
> Are we sure we want to do this in production too? And not maybe just in
> debug builds first?

We're already crashing in the IPC layer when we send these messages. I think it is better to keep going, and possibly risk silently dropping some data, than to just crash immediately.
Comment on attachment 8759420 [details] [diff] [review]
Refuse to send large messages via the message manager.

Do you want to make this opt-out so that we can correlate this with addons from the release channel? I figure addons are the mostly likely reason we'll keep seeing this over time.
Attachment #8759420 - Flags: feedback?(benjamin) → feedback+
(In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> Do you want to make this opt-out so that we can correlate this with addons
> from the release channel? I figure addons are the mostly likely reason we'll
> keep seeing this over time.

I don't know what telemetry being opt-out entails, but that sounds okay to me. How do I do that?
Flags: needinfo?(benjamin)
(In reply to Andrew McCreight [:mccr8] from comment #17)
> (In reply to Benjamin Smedberg  [:bsmedberg] from comment #16)
> > Do you want to make this opt-out so that we can correlate this with addons
> > from the release channel? I figure addons are the mostly likely reason we'll
> > keep seeing this over time.
> 
> I don't know what telemetry being opt-out entails, but that sounds okay to
> me. How do I do that?

Add `releaseChannelCollection: "opt-out"` to the probe[1].

[1]: https://developer.mozilla.org/en-US/docs/Mozilla/Performance/Adding_a_new_Telemetry_probe#Declaring_a_Histogram
Thanks. bsmedberg also explained a bit in IRC was opt-out means.
Flags: needinfo?(benjamin)
I'm just going to leave it opt-in for now, so it is easier for me to look at the results of telemetry.
Pushed by amccreight@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a4f4e0aa89a
Refuse to send large messages via the message manager. r=baku
https://hg.mozilla.org/mozilla-central/rev/0a4f4e0aa89a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
The limit was bumped up to 256MB, so I'm going to just let this ride the trains.
Also, the telemetry was not doing what I thought it would do, so I need to double check that.
Flags: needinfo?(continuation)
WONTFIX for 49 so also WONTFIX for 48.
(In reply to Andrew McCreight [:mccr8] from comment #24)
> Also, the telemetry was not doing what I thought it would do, so I need to
> double check that.

Alright, I guess it is working. I just had to disable more sanitization, or something. There are very few reports now, which isn't surprising given that the message size limit was bumped back up to 256MB.
Flags: needinfo?(continuation)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: