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)
:
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)
anygregor: review+
Details | Diff | Splinter Review
With that (6.59 KB, patch)
2012-08-17 16:11 PDT, Blake Kaplan (:mrbkap)
philipp: review+
Details | Diff | Splinter Review
Get rid of mid (1.16 KB, patch)
2012-08-17 16:13 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Do the other notifications as well (2.58 KB, patch)
2012-08-17 17:19 PDT, Blake Kaplan (:mrbkap)
anygregor: review+
Details | Diff | Splinter Review
With that (3.00 KB, patch)
2012-08-22 12:40 PDT, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter 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) 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) 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) 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) 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) 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) 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) 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.

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