1126509 - Throw if fixIterator got an unknown object

Status: RESOLVED FIXED

(MailNews Core :: Backend, enhancement)

Description

[:aceman]

the function fixIterator() in iteratorUtils.jsm can be used in these construct:
1. for (var of fixIterator(obj))
2. for (var in fixIterator(obj))

If an object of unsupported type is sent to fixIterator, it just returns null.

Construct of type 1 throws an error (Exception: null has no properties).
Construct of type 2 silently does nothing. This could hide a programming error in which there is no visible error, but the loop does not actually loop on the passed in object.

So I suggest fixIterator complains loudly about this case so that the caller notices the problem.

Comment 1

[:aceman]

I meant that e.g. when the object has different number of items at runtime (zero is also an option) so the programmer codes this loop (version 2). But if the code is not tested enough, the programmer may not notice the loop is actually doing nothing. I think changing the 'return null' to a 'throw' should not have any perf impact for the common case when a correct object is passed in and this last line is not hit.

I prepare the patch.

Comment 2

[:aceman]

Attachment: patch

Aryx, please push to try server, all tests. Thanks.

Comment 3

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=c2260cb3eb68

Comment 4

[:aceman]

Comment on attachment 8555527 [details] [diff] [review]
patch

Thanks, the try runs seem fine. The 2 failures (X) are there even in runs before my patch.

Comment 5

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8555527 [details] [diff] [review]
patch

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

::: mailnews/base/util/iteratorUtils.jsm
@@ +33,5 @@
>      return Array.slice(aObj);
>    }
>  
> +  // We got something unexpected, notify the caller loudly.
> +  throw "An unsupported object sent to toArray: " + typeof(aObj);

It's bad practice to throw things that aren't instances of Error: you'll get exceptions but no stack traces, which leads to rather useless messages in, say, the error console.

Reporting typeof is also rather useless: this will be object for most things; a toString() would capture class names (e.g., for DOM objects or xpconnect-wrapped objects). For standard JS stuff, it still returns [object Object], but I think that's no longer true for ES6 classes.

Comment 6

[:aceman]

Attachment: patch v2

OK, done.

Comment 7

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8564527 [details] [diff] [review]
patch v2

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

::: mailnews/base/test/unit/test_iteratorUtils.js
@@ +91,5 @@
> +  try {
> +    for (let val in iteratorUtils.fixIterator(tryIterate)) { dump(val); }
> +  } catch (e) {
> +    // An Exception is the correct behaviour here.
> +    thrown = true;

You ought to check that e.message is what you expect here...

@@ +152,5 @@
> +  // Bug 1126509, test that toArray rejects unknown objects.
> +  let thrown = false;
> +  let tryIterate = { item: "An object, that is not supported by toArray." };
> +  try {
> +    let result = iteratorUtils.toArray(tryMakingArray);

...because I'm quite sure this isn't throwing the error you wish it were.

::: mailnews/base/util/iteratorUtils.jsm
@@ +33,5 @@
>      return Array.slice(aObj);
>    }
>  
> +  // We got something unexpected, notify the caller loudly.
> +  throw "An unsupported object sent to toArray: " + typeof(aObj);

You need to fix this one as well.

Comment 8

[:aceman]

Attachment: patch v3

Ok, thanks. I remove special-casing NodeList as it seems it is covered by the "if (ITERATOR_SYMBOL in aObj)" check and Array.from(aObj) seems to return the proper thing for it.

Comment 9

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8567188 [details] [diff] [review]
patch v3

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

::: mailnews/base/test/unit/test_iteratorUtils.js
@@ +91,5 @@
> +  try {
> +    for (let val in iteratorUtils.fixIterator(tryIterate)) { dump(val); }
> +  } catch (e) {
> +    // A specific exception is the correct behaviour here.
> +    if (e.message.startsWith("An unsupported object sent to fixIterator:"))

Why aren't you including the whole message?

Comment 10

[:aceman]

Attachment: patch v3.1

OK.

Comment 11

[:aceman]

Thanks.

Comment 12

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as http://hg.mozilla.org/comm-central/rev/5778f2901e57