Closed Bug 777206 Opened 7 years ago Closed 7 years ago

SystemMessageInternal broadcasts information to all content processes

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-kilimanjaro +
blocking-basecamp +

People

(Reporter: cjones, Assigned: fabrice)

References

Details

(Keywords: feature, Whiteboard: [LOE:S][WebAPI:P3][qa-])

Attachments

(1 file)

I don't see why it needs to send information to content processes at all, instead of the specifically targeted app process.
blocking-basecamp: --- → +
Whiteboard: [soft]
I can't think of any terribly sensitive information that we're sending through system messages. So I think we can live with this for v1 and just make sure to nuke this asap after.

If you disagree, feel free to renom.
blocking-basecamp: + → -
blocking-kilimanjaro: --- → +
Whiteboard: [soft]
This is not just a security problem, it's also a performance issue.

If we don't fix this, we can only use system messages for trivial little notifications.  What's the current list of users?  Who's going to ensure that we *don't* use system messages for something nontrivial?
blocking-basecamp: - → ?
Chris, feel free to mark this bug a blocker if you think it should be one. I still don't think it's a stop-ship issue but I agree I could be wrong. We currently don't use system messages for any high volume messages (only things like web activities and alarms) so I don't think that performance is a problem, but I could of course be wrong.
Do we use it for exactly web activities and alarms currently?

Who sr's the addition of system messages that content can listen for?
We haven't been doing sr's for anything other than APIs on the gecko side since the start of B2G.

I forget what the name of the gecko function is to send a system message. Fabrice would know.
(In reply to Jonas Sicking (:sicking) from comment #5)
> We haven't been doing sr's for anything other than APIs on the gecko side
> since the start of B2G.

System messages are APIs on the gecko side, no?
The function used to send system messages is "sendMessage()" implemented by the component that provides nsISystemMessagesInternal.idl. Current users are activities and alarm APIs, and we plan to use them to notify incoming telephony and sms events (bug 782488 and bug 782489).
The only APIs that I can remember that we have designed which use system messages is webactivities, alarm and push. But there could be others that I'm forgetting.

Sending a system message is more like invoking an API call though, so that's why I can't guarantee that it hasn't been done in non-sr'ed code.
Sms can be relatively high volume.

Is there a reason we need to broadcast the messages?  Do we already have the receiver information available in manifests?  Trying to gauge the difficulty of fixing this.
No one is arguing that this isn't a bug. The question at hand is if this is a blocker.
No, there doesn't seem to be a need to broadcast these messages. System messages are always intended for a specific app.
Chris Jones, we'll leave this up to you to +.
Whiteboard: [blocked-on-input Chris Jones]
I think the potential for perf/security footgunning with this is just too high.

Re-nom if you think fixing this will compete with finishing a user-visible feature.  For that the calculus is different.
Assignee: nobody → fabrice
blocking-basecamp: ? → +
Whiteboard: [blocked-on-input Chris Jones] → [LOE:?]
Chris, of course it will. Everything we do on the platform side will. I don't really understand the question.
Yeah, it seems pretty obvious that we're talking past each other.  I don't know how to phrase comment 13 any more clearly.
Attached patch patchSplinter Review
I'm using Hello/Goodbye messages to register and unregister message managers and to filter by manifestURL to only dispatch messages where they should go.
Attachment #658613 - Flags: review?(anygregor)
Whiteboard: [LOE:?] → [LOE:?][WebAPI:P3]
Keywords: feature
Whiteboard: [LOE:?][WebAPI:P3] → [LOE:S][WebAPI:P3]
Comment on attachment 658613 [details] [diff] [review]
patch

Why didn't you use register and unregister?
Attachment #658613 - Flags: review?(anygregor) → review+
I did s/Hello/Register and s/Goodbye/Unregister:

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf0512c87af7
https://hg.mozilla.org/mozilla-central/rev/cf0512c87af7
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Comment on attachment 658613 [details] [diff] [review]
patch

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

::: dom/messages/SystemMessageInternal.js
@@ +141,5 @@
> +      this._listeners[aManifestURI].forEach(function sendMsg(aListener) {
> +        aListener.sendAsyncMessage("SystemMessageManager:Message",
> +                                   { type: aType,
> +                                     msg: aMessage,
> +                                     manifest: aManifestURI })

I'm afraid this is wrong. PageA would also receive the message that is aimed to be sent to the pageB, when both pageA and pageB were registered in the this._listeners[aManifestURI] queue for the same App.

Fire bug 801257. I could be wrong. Please feel free to correct me. :)
Whiteboard: [LOE:S][WebAPI:P3] → [LOE:S][WebAPI:P3][qa-]
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.