Closed Bug 777203 Opened 8 years ago Closed 7 years ago

WifiWorker broadcasts wifi info to all content processes

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
blocking-basecamp +

People

(Reporter: cjones, Assigned: mrbkap)

References

Details

Attachments

(2 files, 3 obsolete files)

It should only send information to the process(?) with wifi-control permissions.
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: ? → +
Attached patch Easy patch (obsolete) — Splinter Review
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.)
Attached patch With thatSplinter Review
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)
Attached patch Get rid of mid (obsolete) — Splinter Review
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+
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)
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.)
Attached patch With thatSplinter Review
I'm keeping my inline decrements, but the array.some thing was too clever by half.
Attachment #652992 - Attachment is obsolete: true
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
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.