883766 - JavaScript Warning: "reference to undefined property ret.value" {file: "resource://gre/modules/wifi_worker.js" line: 71}

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Wifi, defect)

Description

[Kan-Ru Chen [:kanru] (UTC+9)]

Found on m-c debug build.

Comment 1

[Ken Chang[:kenkai]]

Vincent, please comments this bug.

Comment 2

[Vincent Chang[:vchang][changyihsin]]

Attachment: Patch v1.0 for m-c

Fixed the typo.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 765279 [details] [diff] [review]
Patch v1.0 for m-c

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

::: dom/wifi/wifi_worker.js
@@ +67,5 @@
>      postMessage({ id: id, status: ret, reply: reply });
>      break;
>    case "wait_for_event":
>      var ret = libhardware_legacy.wait_for_event(cbuf, 4096);
> +    var event = cbuf.readString().substr(0, ret);

This has been wrong for a while and (fortunately) that bug was hidden by this one. readString() does UTF-8 expansion on cbuf, and ret returns the number of bytes returned. That means that using ret after readString() is wrong. See also bug 819164. We should probably (to be safe) do something like:

if (ret > 0 && ret != 4069) cbuf[ret] = 0;
var event = cbuf.readString(); /* should this use readStringReplaceMalformed? */

I'm not sure which function to use, and this needs to be tested carefully.

Comment 4

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

(In reply to Blake Kaplan (:mrbkap) from comment #3)
> 
> if (ret > 0 && ret != 4069) cbuf[ret] = 0;
> var event = cbuf.readString(); /* should this use
> readStringReplaceMalformed? */
> 
> I'm not sure which function to use, and this needs to be tested carefully.

I think in most cases, we should use readStringReplaceMalformed().

If returned string is UTF-8 encoded, it works as same as readString().
If returned string contains invalid char, mostly non-UTF8-encoded SSID, it will replace these chars with malformed chars, instead of throwing an error and break ril_worker.

Comment 5

[Vincent Chang[:vchang][changyihsin]]

Attachment: m-c Patch v1.1

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 774469 [details] [diff] [review]
m-c Patch v1.1

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

::: dom/wifi/wifi_worker.js
@@ +69,5 @@
>    case "wait_for_event":
>      var ret = libhardware_legacy.wait_for_event(cbuf, 4096);
> +    // Make sure the cbuf is null-terminated.
> +    if (ret > 0 && ret != 4069) {
> +      cbuf[ret] = 0;

The maximum length is 4096, not 4069.

We should probably still send the event if the string was the maximum length, though (so only the "cbuf[ret] = 0;" should be hidden under the if statement.

Comment 7

[Vincent Chang[:vchang][changyihsin]]

> Review of attachment 774469 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/wifi/wifi_worker.js
> @@ +69,5 @@
> >    case "wait_for_event":
> >      var ret = libhardware_legacy.wait_for_event(cbuf, 4096);
> > +    // Make sure the cbuf is null-terminated.
> > +    if (ret > 0 && ret != 4069) {
> > +      cbuf[ret] = 0;
> 
> The maximum length is 4096, not 4069.
> 
> We should probably still send the event if the string was the maximum
> length, though (so only the "cbuf[ret] = 0;" should be hidden under the if
> statement.

We might have a problem when return value is 4096 but without 
null terminated in cbuf. If my understanding is correct, the 
cbuf should always be null terminated before we use readStringReplaceMalformed(). 
 
How about this ?  

    // Check array index is correct. 
    if (ret > 0 && ret < 4096) {
      // Make sure the string buffer is null terminated.
      cbuf[ret] = 0;
      // Use readStringReplaceMalformed() to handle non-UTF8-encoded string.
      var event = cbuf.readStringReplaceMalformed();
      postMessage({ id: id, event: event }); 
    }

Comment 8

[Blake Kaplan (:mrbkap) (inactive)]

That looks good to me.

Comment 9

[Vincent Chang[:vchang][changyihsin]]

Attachment: m-c Patch v1.2

Comment 10

[Vincent Chang[:vchang][changyihsin]]

http://hg.mozilla.org/projects/birch/rev/a965d9f77a3d

Comment 11

[Ed Morley [:emorley]]

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