Bug 21257 – JS bindings should throw when too few arguments are passed

• Component: WebCore JavaScript
• Status: RESOLVED CONFIGURATION CHANGED

[Aaron Boodman]

The JS bindings in WebKit generally do not throw when too few arguments are passed. For example "document.createElement()" throws in IE and Firefox because it is spec'd to take one argument, but not in WebKit.

Throwing for invalid function calls is important to developers because it allows them to catch problems in their code as early as possible. In the case of document.createElement(), it means that they will clearly see the error 'createElement expects 1 argument', instead of some confusing error later in their code where they tried to use the undefined return value of createElement.

We can easily make this change all at once for all generated bindings. However, a concern with this is that there may be special cases where other browsers do not follow the spec and allow fewer arguments. This would be a problem for compat. We can probably generate code to find these cases using the IDL.

[Sam Weinig]

I am not against this change in principle (matching other browsers makes sense), but I am curious if there are any known sites where our behavior breaks things.  The behavior we have matches that of how functions defined in the ECMAScript act, and how we perceive most JS developers think about JS functions (most don't check that arguments.length is the correct value).  If we do make a change, we should discuss standardizing the behavior in the WebIDL specification, if that has not already been done. 

[Aaron Boodman]

Sam and I talked at length about this at lunch today, but I wanted to repeat here for the record...

(In reply to comment #1)
> I am not against this change in principle (matching other browsers makes
> sense), but I am curious if there are any known sites where our behavior breaks
> things.

It seems unlikely that there will be many sites broken by WebKit's current behavior because:

a) It's rare and inadvisable to *rely* on exceptions being thrown
b) Other browsers throw exceptions in these cases, so in common code paths, WebKit would get the correct argument counts

> The behavior we have matches that of how functions defined in the
> ECMAScript act, and how we perceive most JS developers think about JS functions
> (most don't check that arguments.length is the correct value). 

It's difficult to speak to how most JS developers expect things to work, I can only talk about my experience with the code I've worked on. From what I've seen, it's common for even moderately complex JS applications to carefully check incoming arguments at component boundaries.

It's true that ECMAScript specs the built-in objects to not throw in the case of obviously wrong arguments. This is really unfortunate and leads to really strange behavior (setFullYear() will set the instance's date field to NaN if passed zero arguments!).We can't do anything about ECMAScript built-ins because that is how they are spec'd, and that is how Firefox works today (IE, otoh, throws in these cases).

But in the case of all the other host-provided APIs, I think we should attempt to be as strict as possible without breaking compatibility seriously. Luckily the other major browsers are already strict in this way, so hopefully the amount of breakage is small.

At the very least, I hope that we can make all *new* APIs strict.

> If we do make a
> change, we should discuss standardizing the behavior in the WebIDL
> specification, if that has not already been done. 

This seems like a very good idea.

[Mike Hearn]

I presume this is related to the fact that in Chrome some_function.length always returns zero instead of the number of arguments it expects?

That's sort of unfortunate. I am building a DOM diffing tool and the fact that WebKit is so lax about such things is currently making it less useful than it otherwise could be.

Are the changes required complex? I have zero experience of being a WebKit hacker, and the fact that Chrome and Safari use different JavaScript engines is discouraging as well - presumably this bug would need to be fixed twice?

[Darin Adler]

(In reply to comment #3)
> I presume this is related to the fact that in Chrome some_function.length
> always returns zero instead of the number of arguments it expects?

If you are getting the wrong value for the length property on a function, you should file a separate bug about it. It's not the same issue.

I'm guessing this is a problem specific to the DOM bindings for the V8 JavaScript engine. I believe the length property works fine for DOM bindings with WebKit’s own JavaScript engine.

> That's sort of unfortunate. I am building a DOM diffing tool and the fact that
> WebKit is so lax about such things is currently making it less useful than it
> otherwise could be.

I think "so lax about such things" is a misleading way to put it.

Extra arguments are ignored to match the JavaScript language. The language and its built-in functions all work this way. For example, you can pass extra arguments to something like the substr() function on a string and they are ignored, and no exception is raised.

But it seems that the traditional behavior for the DOM functions in other browsers is not the same as the behavior of the JavaScript function that are part of the language specification it self. And I this bug is open because we think it’s worth considering changing to match the other browsers.

> Are the changes required complex? I have zero experience of being a WebKit
> hacker, and the fact that Chrome and Safari use different JavaScript engines is
> discouraging as well - presumably this bug would need to be fixed twice?

The fix should be relatively straightforward. It will just be adding code that checks numbers of parameters and throws exceptions, both to individual function bindings and to the automatically-generated function bindings created by the perl script that reads our DOM IDL files.

A more challenging part is the necessary step writing thorough test cases.

And yes, I think this will need to be fixed twice.

[Mike Hearn]

Sorry Darin, you're totally right. I tested this on Safari as well as Chrome - it's only Chrome that always returns zero, so I will go file a V8 bug.

[Aaron Boodman]

FWIW, we should try and find ways over time to share more of the bindings code between v8 and jsc. There are lots of bugs and complexity lurking in there.

[Adam Barth]

(In reply to comment #6)
> FWIW, we should try and find ways over time to share more of the bindings code
> between v8 and jsc. There are lots of bugs and complexity lurking in there.

*political land mine explodes*

FWIW, I agree.  :)

[Darin Adler]

*** Bug 39962 has been marked as a duplicate of this bug. ***

[Darin Adler]

*** Bug 40057 has been marked as a duplicate of this bug. ***

[Andreas Kling]

Created attachment 57671 [details]
Proposed patch

(Reposting patch from bug 40057)
This has since been standardized in the Web IDL specification and
WebKit is the only major engine that doesn't implement the behavior.
Spec: http://www.w3.org/TR/WebIDL/#es-operations
On a side note, fixing this brings us to a nice 100% on John Resig's Selectors API test ;-)

[Adam Barth]

Wow, that patch is kind of awesome, if huge.  Can you do the V8 bindings generator too?

I know AP and Sam have string opinions here.  Maybe we should find some time to chat on IRC to make sure we're all on board with this change?

[Alexey Proskuryakov]

My opinion is that the engine should help developer by detecting mistakes, as far as possible while preserving Web and other content compatibility. So, I like the idea, but I'm also concerned about compatibility, especially with Dashboard widgets. It's almost certain that we'll need to leave a quirk for those.

Others probably have stronger opinions on this matter.

[Darin Adler]

Personally, I was a fan of consistency between the DOM and the functions built in to the JavaScript language. But if no other browsers are doing it that way, and this is even specified in a future standard, it seems that was wrong.

Like other such sweeping changes, it’s highly likely there is some content unwittingly depends on our old behavior. All a programmer has to do is forget one argument to a function, and the program gets an exception whereas before it would keep running. If it’s code or a code path that was never tested with engines other than WebKit then this could easily be overlooked.

Thus this change is almost certain to break  “Safari-only” code paths or even “WebKit-only” ones on at least some websites, Dashboard widgets, Mac OS X applications, iPhone applications, and iTunes Store content. Other than that, I’m all for it!

[Adam Barth]

> Other than that, I’m all for it!

:)

It seems like we have a couple of things queued up that might be in the same camp (e.g., the HTML5 parser).  Maybe it makes some sense to coordinate these in some way to cause less pain?

[Andreas Kling]

Created attachment 57752 [details]
Proposed patch v2

Updated with support for V8 bindings as well.

[Ojan Vafai]

I really like the idea of enforcing that you pass the right number of arguments.

However, I also like that I can leave out the frequently unnecessary arguments. Can we make many of these optional via the IDL? That would maintain some of the benefits of not enforcing number of arguments and it would minimize the chance of introducing regressions in webkit-only codepaths.

For example, being able to leave out the third boolean parameter in addEventListener is great. We should maintain that.

Taking a quick look at the patch, the following all have arguments that should be optional: getComputedStyle, addEventListener, removeEventListener, setPosition, contiansNode, createDocumentType, collapse, extend, setProperty, setTimeout, setInterval, createDocument, createHTMLDocument, cloneNode, importNode, createTextNode, setCustomValidity, profile, profileEnd, evaluate, createNodeIterator, execCommand.

The tricky part is that the default value will need to be different for some of them, e.g. 
- "" for createHTMLDocument and createTextnode. 
- false for addEventListener and containsNode
- 0 for setTimeout, setInterval, setPosition, etc.
- null for getComputedStyle and evaluate

There are cases in this patch that I think are improvements and should not be optional, e.g., arc, createElement, alert, confirm, prompt, parseInt.

[Andreas Kling]

(In reply to comment #16)
> For example, being able to leave out the third boolean parameter in addEventListener is great. We should maintain that.

While I appreciate the implications of making previously working code fail, I don't agree that we should maintain this.

Imagine a web developer using a WebKit-based browser as his main development platform. When he switches to another browser for compatibility testing and his code throws exceptions in "random" places, WebKit starts to look a bit too permissive.

I like what AP said about the engine helping the developer detect mistakes. So if we do take the route of making these arguments optional, I think we should at least emit some kind of "developer-only" warning when leaving them out.

[Adam Barth]

> However, I also like that I can leave out the frequently unnecessary arguments. Can we make many of these optional via the IDL?

The path for doing that would be via standards bodies.  We should strive for interoperability between browsers rather than adding random WebKit quirks.

[Alexey Proskuryakov]

It is not always a mistake to omit the third argument in addEventListener() - for example, it's not useful with XMLHttpRequest objects. So, it makes sense to have it optional, especially if we can get other engines to support this behavior eventually.

Of course, I agree that this should go via standards bodies. Perhaps an experiment can be run in the meanwhile to determine what breaks due to the change to raise more exceptions.

[Ojan Vafai]

(In reply to comment #18)
> > However, I also like that I can leave out the frequently unnecessary arguments. Can we make many of these optional via the IDL?
> 
> The path for doing that would be via standards bodies.  We should strive for interoperability between browsers rather than adding random WebKit quirks.

If we were to a) make the default being to throw when too few arguments are passed in and b) make the relevant arguments optional, that would be an incremental step in the direction of interoperability with other browsers while still minimizing the risk of causing regressions.

I agree that we should push for these changes in standards bodies. Making previously required arguments optional should be extremely safe. We can move forward with this patch (including making arguments optional) and we could email the appropriate standards bodies. I don't care which order we do that in or we could do so in parallel.

It would be a real shame to just make all these arguments required though. FWIW, I have personally written sites that would break if we did. I can't imagine I'm the only one. And really, it's one of the nicer things about coding just for WebKit that you can use a more concise coding style by leaving out these arguments.

(In reply to comment #17)
> (In reply to comment #16)
> > For example, being able to leave out the third boolean parameter in addEventListener is great. We should maintain that.
> 
> While I appreciate the implications of making previously working code fail, I don't agree that we should maintain this.
> 
> Imagine a web developer using a WebKit-based browser as his main development platform. When he switches to another browser for compatibility testing and his code throws exceptions in "random" places, WebKit starts to look a bit too permissive.

Previously working code failing is only one of my concerns. I'm also worried that we're making the web platform worse. These specific cases are arguments that *should* have been optional from the start. In either case, the fix to your concern about WebKit being too permissive is to bring other browser vendors in agreement to make these arguments optional.

> I like what AP said about the engine helping the developer detect mistakes. So if we do take the route of making these arguments optional, I think we should at least emit some kind of "developer-only" warning when leaving them out.

In general, I do think we should throw when you give too few arguments. I also think that many of the arguments in question should be optional in all browsers.

[Adam Barth]

I think incrementally approaching the standards is a good idea.  Maybe the first iteration of this patch should just log to UMA in the V8 bindings so we can see how big the compat issues are?

[Sam Weinig]

(In reply to comment #21)
> I think incrementally approaching the standards is a good idea.  Maybe the first iteration of this patch should just log to UMA in the V8 bindings so we can see how big the compat issues are?

This won't show all the failures that will come up in things like dashboard widgets or mobile WebKit sites, that were designed to work for only for WebKit.

For the record, I don't think the benefit of making this change is worth the cost.

[Adam Barth]

> For the record, I don't think the benefit of making this change is worth the cost.

You've come a long way from offering to write the patch yourself.  :)

> This won't show all the failures that will come up in things like dashboard widgets or mobile WebKit sites, that were designed to work for only for WebKit.

That's true.  Engine-specific code is sadness.

Maybe that's a point worth bringing up to the WebIDL standards committee?  IMHO, it's a lot easier to change to not throwing exceptions than it is to change to throwing exceptions.

[Sam Weinig]

(In reply to comment #23)
> > For the record, I don't think the benefit of making this change is worth the cost.
> 
> You've come a long way from offering to write the patch yourself.  :)

A lot changes in ~2 years :) .  That said, I think adding the plumbing to at least warn in the console is not something I oppose (in fact I support it).

> 
> > This won't show all the failures that will come up in things like dashboard widgets or mobile WebKit sites, that were designed to work for only for WebKit.
> 
> That's true.  Engine-specific code is sadness.
> 
> Maybe that's a point worth bringing up to the WebIDL standards committee?  IMHO, it's a lot easier to change to not throwing exceptions than it is to change to throwing exceptions.

I am considering making this change.

[Adam Barth]

> That said, I think adding the plumbing to at least warn in the console is not something I oppose (in fact I support it).

Yeah, logging a warning seems like a win unless it produces too much console spam.

> > Maybe that's a point worth bringing up to the WebIDL standards committee?  IMHO, it's a lot easier to change to not throwing exceptions than it is to change to throwing exceptions.
> 
> I am considering making this change.

My perspective is that we should aim for convergence with other browsers.  If we decide there's too much WebKit-specific code out there that would break with this change, then we should try to convince other browsers to be more lenient so we can eventually interoperate.

[Ojan Vafai]

(In reply to comment #25)
> > That said, I think adding the plumbing to at least warn in the console is not something I oppose (in fact I support it).
> 
> Yeah, logging a warning seems like a win unless it produces too much console spam.
> 
> > > Maybe that's a point worth bringing up to the WebIDL standards committee?  IMHO, it's a lot easier to change to not throwing exceptions than it is to change to throwing exceptions.
> > 
> > I am considering making this change.
> 
> My perspective is that we should aim for convergence with other browsers.  If we decide there's too much WebKit-specific code out there that would break with this change, then we should try to convince other browsers to be more lenient so we can eventually interoperate.

I think that if we made all the cases where arguments should be optional as I suggested above, we'd find that there is very little webkit-specific code that relies on too few arguments generally not throwing an exception. That's just my intuition of course. Whatever we do with this bug, we should try to convince other browsers to make the obvious cases optional (e.g. addEventListener).

I still think we should make the cases I listed optional and try this patch and see what breaks. It incrementally approaches the behavior of other browsers and is the only way we'll get the data we need to evaluate whether throwing on too few arguments is remotely possible.

[Andreas Kling]

(In reply to comment #26)
> I still think we should make the cases I listed optional and try this patch and see what breaks. It incrementally approaches the behavior of other browsers and is the only way we'll get the data we need to evaluate whether throwing on too few arguments is remotely possible.

I like this approach. Updated patch coming in a bit.

[Darin Adler]

(In reply to comment #27)
> (In reply to comment #26)
> > I still think we should make the cases I listed optional and try this patch and see what breaks. It incrementally approaches the behavior of other browsers and is the only way we'll get the data we need to evaluate whether throwing on too few arguments is remotely possible.
> 
> I like this approach. Updated patch coming in a bit.

The part I’m unclear on here is how we will “try” this. Is someone planning some testing?

[Andreas Kling]

(In reply to comment #28)
> The part I’m unclear on here is how we will “try” this. Is someone planning some testing?

Actually, I'm gonna wait for this question to be answered before continuing work on this mountain of a patch.

[Maciej Stachowiak]

(In reply to comment #29)
> (In reply to comment #28)
> > The part I’m unclear on here is how we will “try” this. Is someone planning some testing?
> 
> Actually, I'm gonna wait for this question to be answered before continuing work on this mountain of a patch.

How about doing a version of this with an ENABLE flag? Then vendors can try enabling it in experimental builds whenever it is a good time to test this type of change.

I also suggest that, for the arguments we are leaving optional, that we advocate to the standards bodies to formally make them optional in the relevant specs.

[Anne van Kesteren]

This appears to work as expected.