784893 - Make nsITCPSocketEvent look more like a normal object

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

From bug 733573 comment 192:

If the reason is that we can't implement DOMEvents in JS, then we should make sure that nsITCPSocketEvent looks as much as possible as nsIDOMEvent. That would mean renaming "source" to "target", and making sure that the "type" property returns strings like "open", "error", "data", "drain", "close". I.e. the "on"-prefix should not be included in .type.

Comment 1

[Olli Pettay [:smaug][[e-mail]]]

nsITCPSocketEvent should be implemented using event generator.
For now change 
readonly attribute jsval data;
to
readonly attribute nsIVariant data;

Add [noscript] initTCPSocketEvent method and dictionary for constructor
and add the idl to http://mxr.mozilla.org/mozilla-central/source/js/xpconnect/src/event_impl_gen.conf.in?force=1
The event interface should be in its own .idl file.

That way you get an event which can be created using event ctors:
var e = new TCPSocketEvent("foobarname", { socket: yoursocket, data: yourdata});

No need to write any C++. The event generator takes care of cycle collecting member variables
when needed.

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

See also 
https://bugzilla.mozilla.org/show_bug.cgi?id=765163#c1

Comment 3

[Andrew Sutherland [:asuth] (he/him)]

I'm experiencing a regression on the contents of the data array when actually run in content, so I think we should add a mochitest for this fix that does actual network access to be really sure that the event is good.  We might be able to just have mochitest talk to our built-in HTTP server as the test?

Comment 4

[Donovan Preston [:fzzzy]]

Sounds like we would have to implement a simple http client on top of tcpsocket if we want to talk to the built in httpd? This shouldn't be too hard if it just makes a lot of assumptions about http and sticks to doing http 1.0 with Connection: close.

Comment 5

[Andrew Sutherland [:asuth] (he/him)]

All we need to do is make sure that ondata returns something that looks valid.  I was thinking just "GET / HTTP/1.0\n\n" and making sure that the result has ASCII-looking values.  If the values are undefined or remain initialized to \u0000, that is no good.  Implementing an http client would be total overkill.

Comment 6

[Donovan Preston [:fzzzy]]

Ok, right. That's pretty much what I meant anyway; enough of an http client to make the server produce a response :-)

Comment 7

[Andrew Sutherland [:asuth] (he/him)]

A potentially related issue to switching to a proper event object is using dispatchEvent or something that will properly catch and report exceptions thrown by the event handler in such a way so that window.onerror gets the error and so that there is a full stack trace.  The current errors that end up looking like the following example are fairly sucky:

[Exception... "'TypeError: index is undefined' when calling method: [nsIStreamListener::onDataAvailable]" nsresult: "0x8057001c (NS_ERROR_XPC_JS_THREW_JS_OBJECT)" location: "<unknown>" data: no] [XPConnect JavaScript]

I think the XPConnect lossage case will still likely need to exist for the xpcshell case/tests, but having proper errors in content would be a real nice thing.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I think the only thing that we have to do right now is to fix the issues mentioned in comment 0. Anything else will be easier to do once we have proper support for implementing EventTargets in JS. And those changes will be backwards compatible enough that we can do them without worrying about breaking content.

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Just implement nsITCPSocketEvent using codegenerator.

Oh, and I hope http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/nsIDOMTCPSocket.idl?mark=194-198#191 is wrong. Event types don't start with on-

Comment 10

[Donovan Preston [:fzzzy]]

(In reply to Olli Pettay [:smaug] from comment #9)
> Just implement nsITCPSocketEvent using codegenerator.
> 
> Oh, and I hope
> http://mxr.mozilla.org/mozilla-central/source/dom/network/interfaces/
> nsIDOMTCPSocket.idl?mark=194-198#191 is wrong. Event types don't start with
> on-

The types currently start with on- but this bug is about fixing that, as Jonas mentioned in the first comment on this bug.

Using the codegenerator to get the right behavior is definitely the way this bug will be fixed.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Olli: Can you take this bug. Donovan won't have time to learn the code generator stuff in time for this to make B2Gv1 so otherwise we'll have to just do the simple changes mentioned in comment 0.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

Ok, I'll add support for TCPConnectEvent, but someone needs to test this stuff.
I don't know how to run any b2g stuff, so I'll test this first without #ifdefs and add then
hopefully the right #ifdef.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Actually, since .target can't point to the right object, and 'this' handling would be wrong,
I'll just wait for someone to make TCPSocket a real eventtarget.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Ok, can we then simply do the renaming from comment 0. That way we are much less likely to break content once we fix this stuff up in the future. That was the whole point of this bug in the first place.

Donovan: would you be able to do this?

Comment 15

[Donovan Preston [:fzzzy]]

Sure.

Comment 16

[Donovan Preston [:fzzzy]]

Attachment: Rename the event's .socket attribute to .target; remove "on" prefix from event names

- Switched .socket attribute name to .target
- Changed the .type strings to "open", "data" etc instead of "onopen", "ondata"

Tests pass. The tests showed me how to fix the IPC code too, so Horray! Tests!

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 672379 [details] [diff] [review]
Rename the event's .socket attribute to .target; remove "on" prefix from event names

Review of attachment 672379 [details] [diff] [review]:
-----------------------------------------------------------------

Yay! Thanks!

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

And thanks for jumping on this so quickly!

Comment 19

[Donovan Preston [:fzzzy]]

Do I need to push this to try or add checkin-needed to the whiteboard? I'm still a little new to bugzilla..

Comment 20

[Dave Hylands [:dhylands]]

If I understand things properly, then:

If you have L3 then you need to push to mozilla-inbound, and if it's sucessful there, push to mozilla-aurora.

If you don't have L3, then you should add [checkin-needed] and [needs-checkin-aurora] to the whiteboard.

Comment 21

[Donovan Preston [:fzzzy]]

Attachment: Rename the event's .socket attribute to .target; remove "on" prefix from event names

This is the exact same patch with the hg metadata lines at the beginning.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Do we have any mochitests or other tests that are running automatically on try that cover this code? If so, definitely push to try.

Comment 23

[Donovan Preston [:fzzzy]]

Yes, there are mochitests and xpcshell tests.

I tried to push to try but my ssh key is still not set up properly or something.

Comment 24

[Ryan VanderMeulen [:RyanVM]]

Having a commit message on the patch you want landed is greatly appreciated. Please do so in the future. Also, I was unaware that Jonas is able to approve patches for landing on aurora? Jonas, I don't want to land this on aurora without being sure that the release drivers are OK with it. Can you clarify?

https://hg.mozilla.org/integration/mozilla-inbound/rev/67f8d99d80fb

Finally, should this have a test?

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I was honestly a bit surprised myself, but I assumed I was given the bugzilla bits for the same reason that I was given bits to mark basecamp-blocking+ (which currently has the same effect).

I'll check with drivers and set back the approval flag if they confirm.

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/67f8d99d80fb

Comment 27

[Ryan VanderMeulen [:RyanVM]]

(In reply to Jonas Sicking (:sicking) from comment #25)
> I'll check with drivers and set back the approval flag if they confirm.

Any updates?

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Thanks for reminding me. I had lost track of which bug this question had been asked.

I just checked with Lukas and he said that this was ok for patches that we want for B2G and that are safe for desktop/fennec. Which is the case here (I wouldn't have approved it otherwise).

Thanks for checking on this to make sure that all the ducks are in a row!

Comment 29

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 672497 [details] [diff] [review]
Rename the event's .socket attribute to .target; remove "on" prefix from event names

Please make sure to mark this as status-firefox18:fixed when landing this.

Donovan: We really should be having tests for this. Would you mind writing some up? Ideally we would have mochitests which test the full implementation of the API, for example by connecting to the mochitest http server and simulating a simple HTTP request.

Comment 30

[Donovan Preston [:fzzzy]]

There's already a full test suite of xpcshell tests which test the lower level stuff, and mochitests which test the permissions stuff.

Comment 31

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

But apparently nothing that tests the API itself?

Good to know though that most of this stuff is tested. Definitely makes it a lower priority to write any remaining tests.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2f0e4a2982fb

Going to call this in-testsuite+ based on comment 30. Jonas, feel free to change it back if you feel otherwise.