WifiWorker broadcasts wifi info to all content processes

RESOLVED FIXED

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: cjones, Assigned: mrbkap)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---

Firefox Tracking Flags

(blocking-basecamp:+)

Details

Attachments

(2 attachments, 3 obsolete attachments)

It should only send information to the process(?) with wifi-control permissions.
(Assignee)

Comment 1

5 years ago
I talked with cjones about this on IRC, and it sounds like it blocks basecamp for security reasons and that it should be fairly straightforward to fix.
Assignee: nobody → mrbkap
blocking-basecamp: --- → ?
blocking-basecamp: ? → +
(Assignee)

Comment 2

5 years ago
Created attachment 647724 [details] [diff] [review]
Easy patch

This patch fixes the responses to requests to not broadcast everywhere. Up next is maintaining a list of managers that we need to notify for broadcast messages.
Attachment #647724 - Flags: review?(anygregor)
Attachment #647724 - Flags: review?(anygregor) → review+
Comment on attachment 647724 [details] [diff] [review]
Easy patch

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

Quick drive-by if you don't mind...

::: dom/wifi/WifiWorker.js
@@ +1818,5 @@
> +    if (msg) {
> +      var target = { rid: msg.rid,
> +                     mid: msg.mid,
> +                     manager: aMessage.target.QueryInterface(Ci.nsIFrameMessageManager) };
> +    }

Perhaps it's just me, but `target` is a somewhat confusing name.

In any case, this is basically picking apart `aMessage` and reassembling it slightly differently which seems a bit wasteful. Why not simply pass `aMessage` along to the helper methods? Or just save the target manager in `aMessage.json` or `msg.data`:

  msg.target = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);

and then all you have to do is pass on `msg`. (If you flatten `msg.data` into `msg`, you can save another object allocation on each thread btw. We're doing this quite a bit in the RIL, effectively only using 1 object per round trip.)
(Assignee)

Comment 4

5 years ago
Created attachment 652969 [details] [diff] [review]
With that

philikon, would you mind looking this over? It doesn't go all the way, but it does get rid of one extra object allocation.
Attachment #647724 - Attachment is obsolete: true
Attachment #652969 - Flags: review?(philipp)
(Assignee)

Comment 5

5 years ago
Created attachment 652973 [details] [diff] [review]
Get rid of mid

No need for "manager id" anymore.
Attachment #652973 - Flags: review?(anygregor)
Comment on attachment 652969 [details] [diff] [review]
With that

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

::: dom/wifi/WifiWorker.js
@@ +1847,5 @@
>    },
>  
>    receiveMessage: function MessageManager_receiveMessage(aMessage) {
> +    let msg = aMessage.json || {};
> +    msg.manager = aMessage.target.QueryInterface(Ci.nsIFrameMessageManager);

(Bug 776825 is going to make this QI'ing unnecessary. \o/)

@@ +1865,3 @@
>          break;
>        case "WifiManager:getState": {
> +        // TODO add aMessage.target to our map of targets.

Please file a follow-up for this and mention the bug number in the comment.

(We did the same for the RIL where we fixed bug 777202 first and are doing bug 783392 as a follow up.)
Attachment #652969 - Flags: review?(philipp) → review+
(Assignee)

Comment 7

5 years ago
Comment on attachment 652973 [details] [diff] [review]
Get rid of mid

Let's pretend I never wrote this.
Attachment #652973 - Attachment is obsolete: true
Attachment #652973 - Flags: review?(anygregor)
(Assignee)

Comment 8

5 years ago
Created attachment 652992 [details] [diff] [review]
Do the other notifications as well

This takes care of the TODO comment. I figured that doing it in this bug was safe enough, since it's part of the same fix.
Attachment #652992 - Flags: review?(anygregor)
Comment on attachment 652992 [details] [diff] [review]
Do the other notifications as well

>+      case "WifiManager:managerFinished": {
>+        for (let i = 0; i < this._domManagers.length; ++i) {
>+          let obj = this._domManagers[i];
>+          if (obj.manager === msg.manager) {
>+            if (--obj.count === 0)
>+              this._domManagers.splice(i, 1);

{}
Attachment #652992 - Flags: review?(anygregor) → review+
Comment on attachment 652992 [details] [diff] [review]
Do the other notifications as well

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

::: dom/wifi/WifiWorker.js
@@ +1875,5 @@
> +            }
> +            return false;
> +          }))
> +        {
> +          this._domManagers.push({ manager: msg.manager, count: 1 });

Please still file a follow up for verifying that the process that `msg.manager` corresponds to actually *has* the wifi permission. We can't do this yet (depends on bug 776832), but it needs to happen.

P.S.: Not sure I like the inline decrements (personally I think it's rather bad style or array.some() call inside an `if` test expression... doesn't exactly make fo readable code.)
(Assignee)

Comment 11

5 years ago
Created attachment 654331 [details] [diff] [review]
With that

I'm keeping my inline decrements, but the array.some thing was too clever by half.
Attachment #652992 - Attachment is obsolete: true
(Assignee)

Comment 12

5 years ago
https://hg.mozilla.org/mozilla-central/rev/8a40c103e16a
https://hg.mozilla.org/mozilla-central/rev/88d7348e0b05

I filed bug 784766 on the first paragraph in comment 3.
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
https://hg.mozilla.org/mozilla-central/rev/517169ca9082
https://hg.mozilla.org/mozilla-central/rev/497c8c08b981
You need to log in before you can comment on or make changes to this bug.