571727 - Safeguard the require function from the operator new

Status: RESOLVED WONTFIX

(Add-on SDK Graveyard :: General, defect)

Description

[:Felipe Gomes (needinfo for replies!)]

As talked on the mailing list, we should somehow safeguard the operator new from being applied to the require function. Two approaches are possible: throwing an error and disallowing it to be used, or magically make it call the correct constructor if it is used.

I wrote a patch for each approach, so we can choose on which is the most wanted.

Comment 1

[:Felipe Gomes (needinfo for replies!)]

Attachment: Throw error

Throw an error when require is invoked as a constructor, as the following example:

let url = new require("url").URL();

Comment 2

[:Felipe Gomes (needinfo for replies!)]

Attachment: Proxy calls

On this version, when require is invoked as a constructor, instead of returning the real module, we return a stub object that will forward any calls on it to the correct module, invoking the methods with new objects.

Comment 3

[:Felipe Gomes (needinfo for replies!)]

Note that the second approach has a leaky abstraction due to not existing a default handler for properties as there is for methods (there is no __noSuchProperty__).  Due to this, the following example won't work:

let URL = new require("url").URL; // no function called, so __noSuchMethod__
                                  // won't know about it
let url = URL(...);


Also, the required module would always invoke the methods as constructors. This has the problem of calling methods that are not meant to be constructors.

Example: say module "url" has two exported functions, A and b, but only A is a constructor, while b is a normal function. It's easy to see there's a mistake here:

let resA = new require("url").A();
let resB = new require("url").b(); // <- bug, b shouldn't be used with new

however, it becomes harder to see it this way:

let url = new require("url"); // stub magic object
let resA = url.A();
let resB = url.b(); // oops



For these reasons, I'd personally prefer to completely disallow _new_ to be used with require, instead of adding magic around the language syntax. It would certainly be cool to do that, but it looks to me that we'd be fixing one obscure cause of bugs and introducing a new one. Unless of course we can think of a different approach to do this that doesn't have these shortcomings.

Comment 4

[Myk Melez [:myk] [@mykmelez]]

The Add-on SDK is no longer a Mozilla Labs experiment and has become a big enough project to warrant its own Bugzilla product, so the "Add-on SDK" product has been created for it, and I am moving its bugs to that product.

To filter bugmail related to this change, filter on the word "looptid".

Comment 5

[Myk Melez [:myk] [@mykmelez]]

Now that we encourage folks to omit `new`, this probably doesn't matter enough to be worth the cost and consequences of implementing a fix here.