Throw if fixIterator got an unknown object

RESOLVED FIXED in Thunderbird 38.0

Status

--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: aceman, Assigned: aceman)

Tracking

Trunk
Thunderbird 38.0
Bug Flags:
in-testsuite +

Thunderbird Tracking Flags

(thunderbird38 fixed)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

4 years ago
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.
(Assignee)

Comment 1

4 years ago
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.
(Assignee)

Comment 2

4 years ago
Created attachment 8555527 [details] [diff] [review]
patch

Aryx, please push to try server, all tests. Thanks.
Flags: needinfo?(archaeopteryx)
(Assignee)

Comment 4

4 years ago
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)
(Assignee)

Updated

4 years ago
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-
(Assignee)

Comment 6

4 years ago
Created attachment 8564527 [details] [diff] [review]
patch v2

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-
(Assignee)

Comment 8

4 years ago
Created attachment 8567188 [details] [diff] [review]
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.
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?
(Assignee)

Comment 10

4 years ago
Created attachment 8567639 [details] [diff] [review]
patch v3.1

OK.
Attachment #8567188 - Attachment is obsolete: true
Attachment #8567188 - Flags: review?(Pidgeot18)
Attachment #8567639 - Flags: review?(Pidgeot18)
Attachment #8567639 - Flags: review?(Pidgeot18) → review+
(Assignee)

Comment 11

4 years ago
Thanks.
Keywords: checkin-needed
Pushed as http://hg.mozilla.org/comm-central/rev/5778f2901e57
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
status-thunderbird38: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 38.0
You need to log in before you can comment on or make changes to this bug.