Bug 38180 – Remove unneeded custom code for WebSocket.send

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore JavaScript

[Adam Barth]

Remove unneeded custom code for WebSocket.send

[Adam Barth]

Created attachment 54393 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 54393 [details]
Patch

Seems like we should write a layout test to explain the badness?

[Adam Barth]

It's not really part of our testing plan, but we could.

[Alexey Proskuryakov]

-    if (exec->hadException())
-        return throwError(exec, SyntaxError, "bad message data.");

How is this going to be autogenerated?

> Seems like we should write a layout test to explain the badness?

I'm confused - this bug sounds like it shouldn't change behavior, how can one make a layout test for that?

[Adam Barth]

The old code looks wrong.  (In reply to comment #4)
> -    if (exec->hadException())
> -        return throwError(exec, SyntaxError, "bad message data.");
> 
> How is this going to be autogenerated?

Is that code correct?  Why do we do that here but not everywhere else we convert arguments to strings?

> > Seems like we should write a layout test to explain the badness?
> 
> I'm confused - this bug sounds like it shouldn't change behavior, how can one
> make a layout test for that?

- if (args.size() < 1)

Why do we check args.size() here?  That's very rare in our bindings and usually reserved for when we need it for web compat.  Our general philosophy is that args that aren't given are treated as undefined.

[Alexey Proskuryakov]

> Is that code correct?  Why do we do that here but not everywhere else we
> convert arguments to strings?

We do that sometimes. In particular, we do that elsewhere in WebSocket bindings code (e.g. constructWebSocket()).

> Why do we check args.size() here?  That's very rare in our bindings

Searching for "if (args.size()" reveals many cases where we check for the number of arguments.

I don't have good answers to those questions. It's not unthinkable that most of WebKit code is wrong in this respect. Given that IE and Firefox diverge a lot, and WebIDL is just a working draft, it's hard to even define"right" and "wrong".

Providing better error reporting and catching exceptions from argument conversion early both seem to be generally good things to do, although there's of course the question of how these affect performance.

[Adam Barth]

(In reply to comment #6)
> > Is that code correct?  Why do we do that here but not everywhere else we
> > convert arguments to strings?
> 
> We do that sometimes. In particular, we do that elsewhere in WebSocket bindings
> code (e.g. constructWebSocket()).
[...]
> Providing better error reporting and catching exceptions from argument
> conversion early both seem to be generally good things to do, although there's
> of course the question of how these affect performance.

If that's the right things to do, then we should do it everywhere.  I don't know why we have special-case code here.

> > Why do we check args.size() here?  That's very rare in our bindings
> 
> Searching for "if (args.size()" reveals many cases where we check for the
> number of arguments.

Our project-wide policy is not to check args.size() unless we need to.  This violates the spec, but my understanding is that we do this to make writing JS wrapper functions easier for library developers.  In any case, we should be making these decisions on a project-wide basis (e.g., in the code generator) instead of having randomly different behavior in each location based on who happened to write the custom bindings code.

> I don't have good answers to those questions. It's not unthinkable that most of
> WebKit code is wrong in this respect. Given that IE and Firefox diverge a lot,
> and WebIDL is just a working draft, it's hard to even define"right" and
> "wrong".

Indeed.  However, consistency is a virtue.  As far as I can tell, this method is randomly custom for no reason.  It seems much better to have it work the same way as every other method.

[Alexey Proskuryakov]

Comment on attachment 54393 [details]
Patch

> It seems much better to have it work the same way as every other method.

I don't have a big problem with this change in behavior per se. But I have two problems with this patch:
1. Since it's a behavior change, there should be tests.
2. The "every other method" explanation doesn't really hold water. As I mentioned, there are hundreds of cases where we do the same as the code you are changing does. Those may be a minority, but not a negligible minority.

[Sam Weinig]

(In reply to comment #8)
> (From update of attachment 54393 [details])
> > It seems much better to have it work the same way as every other method.
> 
> I don't have a big problem with this change in behavior per se. But I have two
> problems with this patch:
> 1. Since it's a behavior change, there should be tests.
> 2. The "every other method" explanation doesn't really hold water. As I
> mentioned, there are hundreds of cases where we do the same as the code you are
> changing does. Those may be a minority, but not a negligible minority.

I have to agree with Alexey on this one and would add that we should aim to separate cleanup from functionality change.  And to reiterate, any functionality change here should be tested.

[Eric Seidel (no email)]

Any time we convert a method from custom to autogen a few "bugs" get fixed automatically.

One of them is that

otherObject.function = Class.proto.function
otherObject.function()

starts to throw an error because the autogen bindings check to make sure "this" is of the right type, and so far I've not seen a single custom binding do so.

If we're going to bother to test such a thing, the test should simply walk the whole tree and test all classes at once.

[Adam Barth]

> starts to throw an error because the autogen bindings check to make sure "this"
> is of the right type, and so far I've not seen a single custom binding do so.

I think that's checked before the custom implementation is called.

[Adam Barth]

> I have to agree with Alexey on this one and would add that we should aim to
> separate cleanup from functionality change.

That's a noble aim, but more or less impossible.  The custom bindings are riddled with trivial mistakes.  It seems a fools errand to document that we don't have these specific mistakes.  There's are infinite sea of trivial mistakes we could have.

> And to reiterate, any functionality change here should be tested.

In that case, we should accept any new custom bindings code without testing all the trivial behaviors it has since every time we write custom bindings we introduce tons of subtle functionality changes.

[Adam Barth]

> In that case, we should accept any new custom bindings code 

*shouldn't

[Adam Barth]

Here's the a diff from the future.  Will explain in a minute.

--- /tmp/layout-test-results/websocket/tests/send-throw-expected.txt	2010-04-27 16:47:23.000000000 -0700
+++ /tmp/layout-test-results/websocket/tests/send-throw-actual.txt	2010-04-27 16:47:23.000000000 -0700
@@ -3,7 +3,7 @@
 On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
 
 Connected.
-Caught exception: SyntaxError: bad message data.
+Caught exception: Pickles
 Closed.
 PASS successfullyParsed is true

[Adam Barth]

Created attachment 54473 [details]
Patch

[Adam Barth]

The diff in Comment #14 is the change in behavior from this patch.  I think the old behavior is really bogus because it magically catches and mutates the exception.

[Adam Barth]

Comment on attachment 54473 [details]
Patch

There's another difference in behavior, which is whether the side-effects happen when toString throws, which I've filed https://bugs.webkit.org/show_bug.cgi?id=38230 about.

[Alexey Proskuryakov]

> https://bugs.webkit.org/show_bug.cgi?id=38230 about

For WebSocket specifically, that would be a regression from this patch, right?

As mentioned in comment 6, if we're regression this behavior, it makes no sense to do that only for send() - constructWebSocket also has this check.

> I think the old behavior is really bogus because it magically catches and 
> mutates the exception.

What do WebIDL or other applicable specs say?

[Sam Weinig]

(In reply to comment #12)
> > I have to agree with Alexey on this one and would add that we should aim to
> > separate cleanup from functionality change.
> 
> That's a noble aim, but more or less impossible.  The custom bindings are
> riddled with trivial mistakes.  It seems a fools errand to document that we
> don't have these specific mistakes.  There's are infinite sea of trivial
> mistakes we could have.

For my benefit, can you list some of them?

> 
> > And to reiterate, any functionality change here should be tested.
> 
> In that case, we should accept any new custom bindings code without testing all
> the trivial behaviors it has since every time we write custom bindings we
> introduce tons of subtle functionality changes.

We expect new code to be well tested, but we need to set a higher bar on changes to existing code since it is a change in existing behavior.  For this change in particular, it seems the correct way to go would be to remove the number of arguments check first (with a test as we would require if that was the only thing you wanted to do) and then auto-generate it.

[Adam Barth]

(In reply to comment #18)
> > https://bugs.webkit.org/show_bug.cgi?id=38230 about
> 
> For WebSocket specifically, that would be a regression from this patch, right?

Yes it would.  However, the case is quite rare and we have this bug almost everywhere already.  I'd be happy to fix it in the code generator so that the fix can be applied to every non-custom binding.

> As mentioned in comment 6, if we're regression this behavior, it makes no sense
> to do that only for send() - constructWebSocket also has this check.

I don't understand this comment.  Is there a way to generate constructWebSocket automatically?  I haven't looked at it yet.

> > I think the old behavior is really bogus because it magically catches and 
> > mutates the exception.
> 
> What do WebIDL or other applicable specs say?

I'm not sure, but if WebIDL says we should mutate the exception, then we have many, many other parts of our bindings that are also out of spec.

My point is that autogenerating this code makes it work the same as the vast, vast majority of our bindings.  If there are bugs in the autogenerated code, then those bugs exist everywhere and should be fixed.  As far as I can tell, there's no reason why this specific API should have custom code that's randomly different than the rest of our code.  That's just entropy for the sake of entropy.  Autogenerating this code is a step forward even if we randomly respin a bunch of arbitrary subtleties in the API's behavior because we're making those subtleties match how the rest of the bindings work.

(In reply to comment #19)
> (In reply to comment #12)
> > > I have to agree with Alexey on this one and would add that we should aim to
> > > separate cleanup from functionality change.
> > 
> > That's a noble aim, but more or less impossible.  The custom bindings are
> > riddled with trivial mistakes.  It seems a fools errand to document that we
> > don't have these specific mistakes.  There's are infinite sea of trivial
> > mistakes we could have.
> 
> For my benefit, can you list some of them?

Here's a list off the top of my head:

1) The order in which arguments are coerced to various types can be observed use toString and valueOf.
2) What happens when each of the argument separately (or in combination) throw exceptions: Are the other arguments coerced?  Do any side effects happen?  Does the exception get swallowed?  Which exception do you get thrown?  What if the underlying operation generates an exception?
3) Relatedly, if the method operates on objects, when are the identities of those object frozen?  For example, if you mutate the DOM or the Frame hierarchy during coercion, do you get the objects from before the mutation or after the mutation?

Those are just the ones I can think of related into how and when you coerce arguments.  As we remove more custom code, I'm sure we'll find lost more exciting bugs.  For example, in NodeIterator, we saw that the custom code was computing DOM wrappers using the wrong global object.

The bindings are extremely high-touch regions of code because the web platform can poke at them very directly.  Essentially, the only way to ensure uniform behavior is to autogenerate them.  The custom bindings we have are a necessary evil in some places, but by and large just filled with trivial (and not so trivial) bugs.

> > > And to reiterate, any functionality change here should be tested.
> > 
> > In that case, we should accept any new custom bindings code without testing all
> > the trivial behaviors it has since every time we write custom bindings we
> > introduce tons of subtle functionality changes.
> 
> We expect new code to be well tested, but we need to set a higher bar on
> changes to existing code since it is a change in existing behavior.  For this
> change in particular, it seems the correct way to go would be to remove the
> number of arguments check first (with a test as we would require if that was
> the only thing you wanted to do) and then auto-generate it.

Well, the patch I posted keeps the argument check using the "RequiresAllArguments" IDL attribute.  We can remove that attribute later, if we like.

[Darin Adler]

Comment on attachment 54473 [details]
Patch

> +        // FIXME: RequiresAllArguments is likely bogus here.

I think we should find a better way to keep track of these. I am not sure these “is likely bogus” comments are useful.

> +        [RequiresAllArguments] boolean send(in DOMString data)
> +            raises(DOMException);

I prefer the "all in one line" style for these.

[Adam Barth]

(In reply to comment #21)
> (From update of attachment 54473 [details])
> > +        // FIXME: RequiresAllArguments is likely bogus here.
> 
> I think we should find a better way to keep track of these. I am not sure these “is likely bogus” comments are useful.

Yeah, I removed these comments in the other patches in this vein.

> > +        [RequiresAllArguments] boolean send(in DOMString data)
> > +            raises(DOMException);
> 
> I prefer the "all in one line" style for these.

Will do.

[Adam Barth]

Committed r61390: <http://trac.webkit.org/changeset/61390>