Closed Bug 1126509 Opened 6 years ago Closed 6 years ago

Throw if fixIterator got an unknown object

Categories

(MailNews Core :: Backend, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird38 fixed)

RESOLVED FIXED
Thunderbird 38.0
Tracking Status
thunderbird38 --- fixed

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
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.
Attached patch patch (obsolete) — Splinter Review
Aryx, please push to try server, all tests. Thanks.
Flags: needinfo?(archaeopteryx)
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.
Attachment #8555527 - Flags: review?(Pidgeot18)
Flags: in-testsuite+
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.
Attachment #8555527 - Flags: review?(Pidgeot18) → review-
Attached patch patch v2 (obsolete) — Splinter Review
OK, done.
Attachment #8555527 - Attachment is obsolete: true
Attachment #8564527 - Flags: review?(Pidgeot18)
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.
Attachment #8564527 - Flags: review?(Pidgeot18) → review-
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #8564527 - Attachment is obsolete: true
Attachment #8567188 - Flags: review?(Pidgeot18)
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?
Attached patch patch v3.1Splinter Review
OK.
Attachment #8567188 - Attachment is obsolete: true
Attachment #8567188 - Flags: review?(Pidgeot18)
Attachment #8567639 - Flags: review?(Pidgeot18)
Attachment #8567639 - Flags: review?(Pidgeot18) → review+
Thanks.
Keywords: checkin-needed
Pushed as http://hg.mozilla.org/comm-central/rev/5778f2901e57
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
You need to log in before you can comment on or make changes to this bug.