Bug 84650 - FormData.append() should throw TypeError for not enough arguments
: FormData.append() should throw TypeError for not enough arguments
Status: NEW
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Kentaro Hara
:
Depends on:
Blocks: 84074
  Show dependency treegraph
 
Reported: 2012-04-23 16:45 PDT by Kentaro Hara
Modified: 2013-02-18 20:04 PST (History)
10 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Kentaro Hara 2012-04-23 16:45:05 PDT
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 Kentaro Hara 2012-04-23 16:50:13 PDT
Created attachment 138459 [details]
Patch
Comment 2 Kentaro Hara 2012-04-24 09:51:29 PDT
jianli: review?
Comment 3 Jian Li 2012-04-24 09:57:05 PDT
Comment on attachment 138459 [details]
Patch

The test does not need anything related to setting http server, so it should put under fast/.
Comment 4 Kentaro Hara 2012-04-24 10:01:10 PDT
Created attachment 138586 [details]
Patch
Comment 5 Kentaro Hara 2012-04-24 10:01:34 PDT
(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.
Comment 6 Jian Li 2012-04-24 10:06:06 PDT
Comment on attachment 138586 [details]
Patch

Please make sure the test passes in both JSC and V8.
Comment 7 Kentaro Hara 2012-04-24 10:14:55 PDT
Created attachment 138590 [details]
patch for landing
Comment 8 Alexey Proskuryakov 2012-04-24 10:35:59 PDT
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?
Comment 9 Kentaro Hara 2012-04-24 10:40:12 PDT
(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?
Comment 10 Kentaro Hara 2012-04-24 11:18:44 PDT
Created attachment 138604 [details]
test case
Comment 11 Kentaro Hara 2012-04-24 11:20:33 PDT
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 Hajime Morrita 2012-04-24 14:05:09 PDT
+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 Jian Li 2012-04-24 17:30:58 PDT
(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.
Comment 14 Kentaro Hara 2012-04-24 17:55:06 PDT
I got the result of IE.

IE 9:
FormData is not defined.
Comment 15 Kentaro Hara 2012-04-24 17:57:36 PDT
(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 Kentaro Hara 2012-04-27 13:33:55 PDT
Sam: Do you have any concern to fix this bug?
Comment 17 Kentaro Hara 2012-04-30 11:03:54 PDT
Created attachment 139480 [details]
rebased patch
Comment 18 Kentaro Hara 2012-04-30 11:06:18 PDT
Created attachment 139482 [details]
rebased patch
Comment 19 Kentaro Hara 2012-05-22 03:52:25 PDT
Created attachment 143259 [details]
rebased patch
Comment 20 Adam Barth 2012-05-22 10:01:16 PDT
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.)
Comment 21 Kentaro Hara 2012-05-22 17:34:32 PDT
Created attachment 143412 [details]
rebased patch for landing
Comment 22 Kentaro Hara 2012-05-22 17:35:12 PDT
Sam: Any comment for landing the patch?
Comment 23 Kentaro Hara 2012-11-26 00:27:56 PST
*** Bug 103221 has been marked as a duplicate of this bug. ***
Comment 24 Kentaro Hara 2012-11-26 00:29:10 PST
Sam: Sorry for revoking the old discussion... any objection to landing this patch?
Comment 25 Sam Weinig 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 Chris Dumez 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 Sam Weinig 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 Adam Barth 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 Sam Weinig 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 Adam Barth 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 Kentaro Hara 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.