Bug 84650 - FormData.append() should throw TypeError for not enough arguments
: FormData.append() should throw TypeError for not enough arguments
Status: NEW
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
:
:
: 84074
  Show dependency treegraph
 
Reported: 2012-04-23 16:45 PST by
Modified: 2013-02-18 20:04 PST (History)


Attachments
Patch (5.77 KB, patch)
2012-04-23 16:50 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
Patch (5.63 KB, patch)
2012-04-24 10:01 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
patch for landing (5.61 KB, patch)
2012-04-24 10:14 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
test case (311 bytes, text/html)
2012-04-24 11:18 PST, Kentaro Hara
no flags Details
rebased patch (5.61 KB, patch)
2012-04-30 11:03 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
rebased patch (5.61 KB, patch)
2012-04-30 11:06 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff
rebased patch (5.64 KB, patch)
2012-05-22 03:52 PST, Kentaro Hara
abarth: review+
Review Patch | Details | Formatted Diff | Diff
rebased patch for landing (6.08 KB, patch)
2012-05-22 17:34 PST, Kentaro Hara
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-04-23 16:45:05 PST
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
------- Comment #1 From 2012-04-23 16:50:13 PST -------
Created an attachment (id=138459) [details]
Patch
------- Comment #2 From 2012-04-24 09:51:29 PST -------
jianli: review?
------- Comment #3 From 2012-04-24 09:57:05 PST -------
(From update of attachment 138459 [details])
The test does not need anything related to setting http server, so it should put under fast/.
------- Comment #4 From 2012-04-24 10:01:10 PST -------
Created an attachment (id=138586) [details]
Patch
------- Comment #5 From 2012-04-24 10:01:34 PST -------
(In reply to comment #3)
> (From update of attachment 138459 [details] [details])
> The test does not need anything related to setting http server, so it should put under fast/.

Done. Thanks.
------- Comment #6 From 2012-04-24 10:06:06 PST -------
(From update of attachment 138586 [details])
Please make sure the test passes in both JSC and V8.
------- Comment #7 From 2012-04-24 10:14:55 PST -------
Created an attachment (id=138590) [details]
patch for landing
------- Comment #8 From 2012-04-24 10:35:59 PST -------
(From update of attachment 138590 [details])
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?
------- Comment #9 From 2012-04-24 10:40:12 PST -------
(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?
------- Comment #10 From 2012-04-24 11:18:44 PST -------
Created an attachment (id=138604) [details]
test case
------- Comment #11 From 2012-04-24 11:20:33 PST -------
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...
------- Comment #12 From 2012-04-24 14:05:09 PST -------
+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.
------- Comment #13 From 2012-04-24 17:30:58 PST -------
(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] [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.
------- Comment #14 From 2012-04-24 17:55:06 PST -------
I got the result of IE.

IE 9:
FormData is not defined.
------- Comment #15 From 2012-04-24 17:57:36 PST -------
(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
------- Comment #16 From 2012-04-27 13:33:55 PST -------
Sam: Do you have any concern to fix this bug?
------- Comment #17 From 2012-04-30 11:03:54 PST -------
Created an attachment (id=139480) [details]
rebased patch
------- Comment #18 From 2012-04-30 11:06:18 PST -------
Created an attachment (id=139482) [details]
rebased patch
------- Comment #19 From 2012-05-22 03:52:25 PST -------
Created an attachment (id=143259) [details]
rebased patch
------- Comment #20 From 2012-05-22 10:01:16 PST -------
(From update of attachment 143259 [details])
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.)
------- Comment #21 From 2012-05-22 17:34:32 PST -------
Created an attachment (id=143412) [details]
rebased patch for landing
------- Comment #22 From 2012-05-22 17:35:12 PST -------
Sam: Any comment for landing the patch?
------- Comment #23 From 2012-11-26 00:27:56 PST -------
*** Bug 103221 has been marked as a duplicate of this bug. ***
------- Comment #24 From 2012-11-26 00:29:10 PST -------
Sam: Sorry for revoking the old discussion... any objection to landing this patch?
------- Comment #25 From 2012-11-26 11:42:29 PST -------
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?
------- Comment #26 From 2012-11-26 11:46:27 PST -------
(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.
------- Comment #27 From 2012-11-26 11:49:27 PST -------
(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.
------- Comment #28 From 2012-11-26 12:14:48 PST -------
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.
------- Comment #29 From 2012-11-26 14:24:02 PST -------
(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.
------- Comment #30 From 2012-11-26 15:08:16 PST -------
> 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.
------- Comment #31 From 2012-11-26 15:39:37 PST -------
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.