986349 - WifiWorker.js "this is undefined" exception due to patch of Bug 979841

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Henry Chang [:hchang]]

http://dxr.mozilla.org/mozilla-central/source/dom/wifi/WifiWorker.js#2427

|this| is undefined in the callback of |forEach|:

...
this._domRequest.forEach(function(req) {
  this._sendMessage(req.name + ":Return", false, "Wifi is disabled", req.msg);
});
...

Comment 1

[Henry Chang [:hchang]]

Attachment: Bug986349.patch

Proposed patch

Comment 2

[Chuck Lee [:chucklee, Triggered by NI, Review, and Feedback requests]]

Comment on attachment 8394638 [details] [diff] [review]
Bug986349.patch

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

Nick catch! Thank you.

::: dom/wifi/WifiWorker.js
@@ +2427,2 @@
>        this._sendMessage(req.name + ":Return", false, "Wifi is disabled", req.msg);
> +    }).bind(this));

We don't need add parenthesis around the function definition. just
|function(req) {...}.bind(this)| will do the work.

Comment 3

[Henry Chang [:hchang]]

Thanks for the review!

(In reply to Chuck Lee [:chucklee] from comment #2)
> Comment on attachment 8394638 [details] [diff] [review]
> Bug986349.patch
> 
> Review of attachment 8394638 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nick catch! Thank you.
> 
> ::: dom/wifi/WifiWorker.js
> @@ +2427,2 @@
> >        this._sendMessage(req.name + ":Return", false, "Wifi is disabled", req.msg);
> > +    }).bind(this));
> 
> We don't need add parenthesis around the function definition. just
> |function(req) {...}.bind(this)| will do the work.

It does work but is less expressive to me. The opening parenthesis 
lets the reader know that something unusual is happening. 

A similar case is discussed here: 

http://peter.michaux.ca/articles/an-important-pair-of-parens

Thanks!

Comment 5

[Jason Smith [:jsmith]]

Blocks a blocker.

Leaving qawanted here to find out if this happens on 1.3T, as I think it will happen there.

Comment 6

[Vincent Chang[:vchang][changyihsin]]

This should be nominate to 1.3T as well.

Comment 8

[Jason Smith [:jsmith]]

Bumping to 1.3T since the regressing patch landed on 1.3T as well, so it should be affected here.

Comment 9

[Jason Smith [:jsmith]]

(In reply to Jason Smith [:jsmith] from comment #8)
> Bumping to 1.3T since the regressing patch landed on 1.3T as well, so it
> should be affected here.

Although we technically need a way to flag that this should block 1.3T & 1.4 & be able to land on those branches.

Comment 10

[Henry Chang [:hchang]]

Attachment: Bug986349.patch (r+'d and add r=chulee for checkin)

Comment 11

[Joe Cheng [:jcheng] (please needinfo)]

triage: 1.3T+ for regression

Comment 12

[Carsten Book [:Tomcat]]

landed on b2g-i https://hg.mozilla.org/integration/b2g-inbound/rev/b59cd47f65b3

Comment 13

[Jason Smith [:jsmith]]

Note - we're also going to need this on 1.4. When this lands on trunk, please ask for Aurora approval, as this would have blocked 1.4 as well.

Comment 14

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b59cd47f65b3

Comment 15

[[:fabrice] Fabrice Desré]

https://hg.mozilla.org/releases/mozilla-b2g28_v1_3t/rev/fb19f57373c1

Comment 16

[bhavana bajaj [:bajaj]]

henry,

can you please request approval-mozilla-aurora: ? on this patch attachment and fill the approval request comment details so we can consider this to be backported on gecko 30 and fix this for 1.4.

Comment 17

[Henry Chang [:hchang]]

Comment on attachment 8396170 [details] [diff] [review]
Bug986349.patch (r+'d and add r=chulee for checkin)

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 986349
User impact if declined: Quickly switch on/off wifi would fail
Testing completed (on m-c, etc.): on m-c
Risk to taking this patch (and alternatives if risky): No
String or IDL/UUID changes made by this patch: No

Comment 18

[Kai-Zhen Li [:seinlin][:kli]]

Need to land this in 1.4. Thanks!

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/d40fdf71320e