552037 - If I extend Array.prototype in my extension then the folder pane is empty.

Status: RESOLVED WONTFIX

(Thunderbird :: Folder and Message Lists, defect)

Description

[Steven Gerber]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1.8) Gecko/20100227 Lightning/1.0b2pre Thunderbird/3.0.3

I get "acct.incomingServer is undefined":
Discovering folders for account failed with exception: TypeError: acct.incomingServer is undefined
Warning: reference to undefined property acct.incomingServer
Source file: chrome://messenger/content/folderPane.js
Line: 1192

I traced it to iteratorUtils.jsm:
/**
 * This function takes a javascript Array object and returns an xpcom array
 * of the desired type. It will *not* work if you extend Array.prototype.
 *
 * @param aArray      the array to convert to an xpcom array
 * @param aInterface  the type of xpcom array to convert
 *
 * @note The returned array is *not* dynamically updated.  Changes made to the
 *       js array after a call to this function will not be reflected in the
 *       xpcom array.
 */
function toXPCOMArray(aArray, aInterface) {


Reproducible: Always

Steps to Reproduce:
1. Extend Array.prototype in my extension.
   Array.prototype.sort_numeric_ascending = srglib.arrays.sort_numeric_ascending;
   Array.prototype.sort_numeric_descending = srglib.arrays.sort_numeric_descending;
2. Start TB

Actual Results:  
Errors in console and folder pane is empty
Discovering folders for account failed with exception: TypeError: acct.incomingServer is undefined
Warning: reference to undefined property acct.incomingServer
Source file: chrome://messenger/content/folderPane.js
Line: 1192


Is there a workaround?
I will supply my code (I cannot upload/attach via this form).

Thanks,
Steven

Comment 1

[Ludovic Hirlimann [:Usul]]

(In reply to comment #0)

> Is there a workaround?
> I will supply my code (I cannot upload/attach via this form).

Why it's too big ?

Adding people who might know on cc.

Comment 2

[Steven Gerber]

No, not really.
When I submitted the bug, the main submit page did not allow an upload/attachment.
I will upload a file: srglib_arrays.js.
Things break when I uncomment lines 56 & 57.

Also, note TB function toXPCOMArray(aArray, aInterface) in TB module iteratorUtils.jsm:
/**
 * This function takes a javascript Array object and returns an xpcom array
 * of the desired type. It will *not* work if you extend Array.prototype.
 *
 * @param aArray      the array to convert to an xpcom array
 * @param aInterface  the type of xpcom array to convert
 *
 * @note The returned array is *not* dynamically updated.  Changes made to the
 *       js array after a call to this function will not be reflected in the
 *       xpcom array.
 */
function toXPCOMArray(aArray, aInterface) {

Comment 3

[Steven Gerber]

Attachment: Array processing module from my library

Comment 4

[Andrew Sutherland [:asuth] (he/him)]

Do not extend Array.prototype outside of a JS module.  JS modules get their own copy of Array, so it's okay.

Having said that, a fix to toXPCOMArray to make it use Iterator and be immune to this kind of thing would be accepted.

Comment 5

[Phil Ringnalda (:philor)]

Though it would be totally insufficient to make extending Array.prototype safe, since you have to deal with both certain Tb developers (hi asuth!) and other extension authors thinking that they can get away with using for each over arrays.

Comment 6

[Andrew Sutherland [:asuth] (he/him)]

I enjoy your comments, philor, but I'm confused as to what your point is here.

All new code I write uses Iterator like it's going out of style, but we have to assume there is lots of code in the tree, some of it no doubt written by me, that does not play it safe.  Unless someone is going to undertake an audit to fix all of that code, we should strongly discourage extension developers from extending Array.prototype since it will not always break things in ways this obvious.

Are you suggesting we should undertake the audit?

Comment 7

[Phil Ringnalda (:philor)]

No, more the opposite: I don't think a patch to toXPCOMArray() would be a particularly good idea, since this bug is a sterling example of how it successfully persuades people that they should not extend Array.prototype in an extension. Even if we removed every for each over an array in our code, fired me as a reviewer since I'll never catch them all, and wrote an automated test to continuously audit mozilla-central since I'd be very surprised if every reviewer there catches every single one, it still would make no real difference at all, since we would only be making it possible for people to write extensions that would surprise them by destroying other extensions that use for each over arrays.

Though, rereading comment 0, I'm not actually sure whether Steven was filing "Please fix toXPCOMArray so that I can happily extend Array.prototype and cause myself a support nightmare when my extension causes other extensions and less obvious bits of core code to explode" (a bug which I'd say ought to be wontfix), or whether "Is there a workaround?" was the key phrase and he was actually filing "Please tell me how to get the benefit of extending Array.prototype without actually doing so."

Comment 8

[Andrew Sutherland [:asuth] (he/him)]

philor, do you think we should go so far as to add something like the following (untested code) to a javascript file loaded by messenger.xul?:

window.addEventListener("load", function() {
  setTimeout(function() {
    let arr = [];
    for each (let blah in arr) {
      alert("An extension has modified Array.prototype. " +
            "This may break Thunderbird or your other extensions. " +
            "Please disable your extensions and restart.");
    }
  }, 0);
}, false);

Comment 9

[Andrew Sutherland [:asuth] (he/him)]

er, I left out a break statement in there, but you get the idea.

Comment 10

[John Bieling]

Wayne put me on CC for this. WebExtensions can no longer access core components directly, so this is not an issue anymore. Furthermore, iteratorUtils.jsm and (IIRC) XPCOM arrays have been removed from the tree.

I guess we can close this as wontfix.