Bug 84650 – FormData.append() should throw TypeError for not enough arguments

• Component: WebCore JavaScript
• Status: RESOLVED CONFIGURATION CHANGED

[Kentaro Hara]

FormData.append() should throw TypeError if the number of arguments is less than 2.

The spec:
http://www.w3.org/TR/WebIDL/#dfn-overload-resolution-algorithm
http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-append()-method

[Kentaro Hara]

Created attachment 138459 [details]
Patch

[Kentaro Hara]

jianli: review?

[Jian Li]

Comment on attachment 138459 [details]
Patch

The test does not need anything related to setting http server, so it should put under fast/.

[Kentaro Hara]

Created attachment 138586 [details]
Patch

[Kentaro Hara]

(In reply to comment #3)
> (From update of attachment 138459 [details])
> The test does not need anything related to setting http server, so it should put under fast/.

Done. Thanks.

[Jian Li]

Comment on attachment 138586 [details]
Patch

Please make sure the test passes in both JSC and V8.

[Kentaro Hara]

Created attachment 138590 [details]
patch for landing

[Alexey Proskuryakov]

Comment on attachment 138590 [details]
patch for landing

Wait, is spec compliance the only reason to make this change?

Raising exceptions where we didn't use to do that is extremely dangerous for web compatibility, and shouldn't be treated lightly.  Why do we have this behavior? What do other browsers do? Is there a specific reason why it's unlikely for WebKit specific content to rely on this quirk?

[Kentaro Hara]

(In reply to comment #8)
> (From update of attachment 138590 [details])
> Wait, is spec compliance the only reason to make this change?

The spec compliance and the fact that almost all "not enough arguments" are throwing TypeError. At least, the current behavior that JSC does not throw any error but V8 throws SyntaxError is a bug.

> Raising exceptions where we didn't use to do that is extremely dangerous for web compatibility, and shouldn't be treated lightly.  Why do we have this behavior? What do other browsers do? Is there a specific reason why it's unlikely for WebKit specific content to rely on this quirk?

Let me confirm other browsers' behavior.

jianli: Was there any reason why you implemented SyntaxError in V8 and no error in JSC?

[Kentaro Hara]

Created attachment 138604 [details]
test case

[Kentaro Hara]

Opera 11.62:
FormData is not defined.

Firefox 11.0:
FormData.append(): [Exception... "Not enough arguments [nsIDOMFormData.append]" nsresult: "0x80570001 (NS_ERROR_XPC_NOT_ENOUGH_ARGS)" location: "JS frame :: http://www.corp.google.com/~haraken/null/typeerror.html :: :: line 12" data: no]

I do not have IE now...

[Hajime Morrita]

+1 for ap's comment as a basic standpoint.
I think it's OK to change the error type thrown.
But switching from non-throw to throw should be careful, especially for long standing APIs.
Note that we sometimes selected non-standard behaviors for a compatibility reason in the past.

I think this specific change is acceptable considering the Firefox behavior. 
It would be great if each differences is written down in the ChangeLog.
It's a part of our accountability to webdev folks.

[Jian Li]

(In reply to comment #12)
> +1 for ap's comment as a basic standpoint.
> I think it's OK to change the error type thrown.
> But switching from non-throw to throw should be careful, especially for long standing APIs.
> Note that we sometimes selected non-standard behaviors for a compatibility reason in the past.
> 
> I think this specific change is acceptable considering the Firefox behavior. 
> It would be great if each differences is written down in the ChangeLog.
> It's a part of our accountability to webdev folks.

(In reply to comment #9)
> (In reply to comment #8)
> > (From update of attachment 138590 [details] [details])
> > Wait, is spec compliance the only reason to make this change?
> 
> The spec compliance and the fact that almost all "not enough arguments" are throwing TypeError. At least, the current behavior that JSC does not throw any error but V8 throws SyntaxError is a bug.
> 
> > Raising exceptions where we didn't use to do that is extremely dangerous for web compatibility, and shouldn't be treated lightly.  Why do we have this behavior? What do other browsers do? Is there a specific reason why it's unlikely for WebKit specific content to rely on this quirk?
> 
> Let me confirm other browsers' behavior.
> 
> jianli: Was there any reason why you implemented SyntaxError in V8 and no error in JSC?

Per the comment from Sam (https://bugs.webkit.org/show_bug.cgi?id=36024), I changed the JSC implementation to throw no error. We can keep the V8 change if JSC wants to stick with the current behavior for consistency.

[Kentaro Hara]

I got the result of IE.

IE 9:
FormData is not defined.

[Kentaro Hara]

(In reply to comment #13)
> Per the comment from Sam (https://bugs.webkit.org/show_bug.cgi?id=36024), I changed the JSC implementation to throw no error. We can keep the V8 change if JSC wants to stick with the current behavior for consistency.

Sam: do you have an opinion to change no error to TypeError? (Leaving aside the compatibility problem, ) the first argument and the second argument of FormData.append() are not optional in the spec:

http://dvcs.w3.org/hg/xhr/raw-file/tip/Overview.html#the-append()-method

[Kentaro Hara]

Sam: Do you have any concern to fix this bug?

[Kentaro Hara]

Created attachment 139480 [details]
rebased patch

[Kentaro Hara]

Created attachment 139482 [details]
rebased patch

[Kentaro Hara]

Created attachment 143259 [details]
rebased patch

[Adam Barth]

Comment on attachment 143259 [details]
rebased patch

Since Sam wrote that comment in the other bug, we've changed our default behavior to throwing exception when web sites supply too few arguments.  Given that we're already throwing in this case in V8 and Firefox throws, this patch seems relatively safe from a web compatibly point of view.  You might want to give Sam a chance to comment before landing.

(I would also add the information about what other browsers do to the ChangeLog in case we look into the history of this behavior later.)

[Kentaro Hara]

Created attachment 143412 [details]
rebased patch for landing

[Kentaro Hara]

Sam: Any comment for landing the patch?

[Kentaro Hara]

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

[Kentaro Hara]

Sam: Sorry for revoking the old discussion... any objection to landing this patch?

[Sam Weinig]

I'm disappointed that the behavior of WebKit changed to throw on too many arguments. Can you confirm that both JSC and V8 do that for all of WebKit now?

[Chris Dumez]

(In reply to comment #25)
> I'm disappointed that the behavior of WebKit changed to throw on too many arguments. Can you confirm that both JSC and V8 do that for all of WebKit now?

I don't understand. The patch makes it throw on too few arguments (not too many). For this method, WebKit will not throw if too many arguments are provided.

[Sam Weinig]

(In reply to comment #26)
> (In reply to comment #25)
> > I'm disappointed that the behavior of WebKit changed to throw on too many arguments. Can you confirm that both JSC and V8 do that for all of WebKit now?
> 
> I don't understand. The patch makes it throw on too few arguments (not too many). For this method, WebKit will not throw if too many arguments are provided.

Sorry, I meant too few.

[Adam Barth]

Sam, I'm very confused by your message.

1) This patch does not appear to have landed, so WebKit's behavior has not yet changed.

2) Can you explain why you would be disappointed if we landed this patch?  Your comment above isn't very helpful because it just states your conclusion without letting us follow your reasoning.

[Sam Weinig]

(In reply to comment #28)
> Sam, I'm very confused by your message.
> 
> 1) This patch does not appear to have landed, so WebKit's behavior has not yet changed.
> 
> 2) Can you explain why you would be disappointed if we landed this patch?  Your comment above isn't very helpful because it just states your conclusion without letting us follow your reasoning.

I apologize for not being clear.  I was not disappointed that the patch was landed (as it hasn't) but rather that we had changed our behavior elsewhere to throw more often (something I don't think is a good idea as it is likely to break legacy content, for little gain).  I was (am) trying to determine exactly what our current policy for throwing on incorrect number of arguments being provided, as I don't remember the specifics.

[Adam Barth]

> I was (am) trying to determine exactly what our current policy for throwing on incorrect number of arguments being provided, as I don't remember the specifics.

Our current policy is that new APIs should throw when given too few arguments (as required by the specs) but that we shouldn't change existing APIs to throw if there is a risk of compatibility problems.

The way we've done this is to make the code generator default to throwing when too few arguments are supplied and to mark the parameters for APIs where the arguments are optional with the [Optional] attribute.

[Kentaro Hara]

Sam: Thanks. As far as I checked a few months ago, FormData.append() was one of a few methods that don't throw TypeError. So we'd like to make the change for consistency if there is no compatibility concern. Given that Firefox and Chrome throw TypeError and that IE and Opera don't support FormData, it would make sense to throw TypeError in JSC.

[Ahmad Saleem]

Updated results from all browsers:

*** Safari Technology Preview 152 ***

FormData.append(): TypeError: Not enough arguments

*** Firefox Nightly 106 ***

 FormData.append(): TypeError: FormData.append: 0 is not a valid argument count for any overload.

*** Chrome Canary 107 ***

FormData.append(): TypeError: Failed to execute 'append' on 'FormData': 2 arguments required, but only 0 present.

________

Just wanted to share updated status. Is anything needed here because all of them are giving same error but in different details? Thanks!

[Darin Adler]

That confirms that this is resolved. Likely many years ago.