Last Comment Bug 777203 - WifiWorker broadcasts wifi info to all content processes
: WifiWorker broadcasts wifi info to all content processes
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All All
: -- normal (vote)
: ---
Assigned To: Blake Kaplan (:mrbkap) (please use needinfo!)
:
Mentors:
Depends on:
Blocks: 777188
  Show dependency treegraph
 
Reported: 2012-07-24 18:34 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-08-23 03:47 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
Easy patch (6.77 KB, patch)
2012-07-31 15:40 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
anygregor: review+
Details | Diff | Review
With that (6.59 KB, patch)
2012-08-17 16:11 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
philipp: review+
Details | Diff | Review
Get rid of mid (1.16 KB, patch)
2012-08-17 16:13 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
no flags Details | Diff | Review
Do the other notifications as well (2.58 KB, patch)
2012-08-17 17:19 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
anygregor: review+
Details | Diff | Review
With that (3.00 KB, patch)
2012-08-22 12:40 PDT, Blake Kaplan (:mrbkap) (please use needinfo!)
no flags Details | Diff | Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-07-24 18:34:58 PDT
It should only send information to the process(?) with wifi-control permissions.
Comment 1 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-25 17:54:42 PDT
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.
Comment 2 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-07-31 15:40:32 PDT
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.
Comment 3 Philipp von Weitershausen [:philikon] 2012-07-31 19:09:12 PDT
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.)
Comment 4 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-17 16:11:13 PDT
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.
Comment 5 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-17 16:13:32 PDT
Created attachment 652973 [details] [diff] [review]
Get rid of mid

No need for "manager id" anymore.
Comment 6 Philipp von Weitershausen [:philikon] 2012-08-17 16:24:19 PDT
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.)
Comment 7 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-17 17:08:08 PDT
Comment on attachment 652973 [details] [diff] [review]
Get rid of mid

Let's pretend I never wrote this.
Comment 8 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-17 17:19:36 PDT
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.
Comment 9 Gregor Wagner [:gwagner] 2012-08-17 17:35:49 PDT
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);

{}
Comment 10 Philipp von Weitershausen [:philikon] 2012-08-21 19:44:34 PDT
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.)
Comment 11 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-22 12:40:11 PDT
Created attachment 654331 [details] [diff] [review]
With that

I'm keeping my inline decrements, but the array.some thing was too clever by half.
Comment 12 Blake Kaplan (:mrbkap) (please use needinfo!) 2012-08-22 13:16:20 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.