Make nsITCPSocketEvent look more like a normal object

RESOLVED FIXED in Firefox 18

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: sicking, Assigned: fzzzy)

Tracking

Trunk
mozilla19
x86
Mac OS X
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox18 fixed, firefox19 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
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.
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?
(Assignee)

Comment 4

6 years ago
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.
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.
(Assignee)

Comment 6

6 years ago
Ok, right. That's pretty much what I meant anyway; enough of an http client to make the server produce a response :-)
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.
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.
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-
(Assignee)

Comment 10

6 years ago
(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.
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.
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.
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.
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?
(Assignee)

Comment 15

6 years ago
Sure.
Assignee: nobody → dpreston
(Assignee)

Comment 16

6 years ago
Created attachment 672379 [details] [diff] [review]
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!
Attachment #672379 - Flags: review?(jonas)
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!
Attachment #672379 - Flags: review?(jonas) → review+
And thanks for jumping on this so quickly!
Attachment #672379 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 19

6 years ago
Do I need to push this to try or add checkin-needed to the whiteboard? I'm still a little new to bugzilla..
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.
(Assignee)

Comment 21

6 years ago
Created attachment 672497 [details] [diff] [review]
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.
Attachment #672379 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Whiteboard: checkin-needed needs-checkin-aurora
Keywords: checkin-needed
Whiteboard: checkin-needed needs-checkin-aurora → [checkin to inbound and aurora]
Do we have any mochitests or other tests that are running automatically on try that cover this code? If so, definitely push to try.
(Assignee)

Comment 23

6 years ago
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.
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?
Flags: in-testsuite?
Keywords: checkin-needed
Whiteboard: [checkin to inbound and aurora]
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.
https://hg.mozilla.org/mozilla-central/rev/67f8d99d80fb
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
(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?
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 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.
Attachment #672497 - Flags: approval-mozilla-aurora+
(Assignee)

Comment 30

6 years ago
There's already a full test suite of xpcshell tests which test the lower level stuff, and mochitests which test the permissions stuff.
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.
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.
status-firefox18: --- → fixed
status-firefox19: --- → fixed
Flags: in-testsuite? → in-testsuite+
Component: DOM: Mozilla Extensions → DOM
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.