Closed Bug 1228667 Opened 8 years ago Closed 8 years ago

Implement FlyWeb service discovery

Categories

(Core :: Networking, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: djvj, Assigned: djvj)

References

Details

Attachments

(3 files, 25 obsolete files)

56.56 KB, patch
Details | Diff | Splinter Review
16.53 KB, patch
Details | Diff | Splinter Review
7.98 KB, patch
Details | Diff | Splinter Review
Build a first-stab implementation of FlyWeb service discovery in Gecko.

The initial implementation of this will use mDNS-based service discovery, but should expand to other discovery protocols over time.  As such, the interfaces here should be general enough to accommodate extension to separate transport protocols in the future.

The discovery model for FlyWeb has the following basic structure:

1. A collection of "known services" which mutates asynchronously as new services are discovered, and known services are lost.

2. Every known service is associated with a name.

3. Known services start out in an 'unconnected' state.  They API can be instructed to attempt to connect to one of them, which attempts to establish a connected session between the browser and the service.

4. At the end of a successful connection, the browser assigns a generated, internal name to the service.  Most likely a UUID.
Attached patch flyweb-service-discovery.patch (obsolete) — Splinter Review
Added some IDL and basic stub implementation code.  I am new to XPCOM, IDLs, and gecko/necko in general, so learning as I go.
Attached patch flyweb-service-discovery.patch (obsolete) — Splinter Review
Attachment #8693081 - Attachment is obsolete: true
Attached patch flyweb-service-discovery.patch (obsolete) — Splinter Review
More baby steps.  This should add a service for instantiating nsFlyWebTransportManager instances.  The overspecifiation in nsFlyWebTransportManager is just a testing ground to figure out how xpcom IDL semantics work on smaller APIs before moving onto doing nsFlyWebService and all the things that flow from that, which are the bigger tasks ahead.

Anyway, this seems to build (and therefore, it must be correct).
Attachment #8693126 - Attachment is obsolete: true
Attached patch flyweb-service-discovery.patch (obsolete) — Splinter Review
More fill-in.  Pared down the interfaces a bit - the API now is just discovery with the connection methods removed.  Implemented ServiceList, ServiceManager, and ServiceInfo.
Attachment #8698236 - Attachment is obsolete: true
Attached patch flyweb-service-discovery.patch (obsolete) — Splinter Review
Fixed a couple bugs with refcount handling.  Brief testing using Browser Console - the basics seem to work.  I can construct an nsIFlyWebTransportManager service instance and get |internetTransport| from it.  I can construct an nsIFlyWebServiceManager service instance and get an empty list of nsIFlyWebServiceList from it.
Attachment #8698604 - Attachment is obsolete: true
Attached patch flyweb-service-discovery.patch (obsolete) — Splinter Review
Fixed up service discovery interfaces.  This should be good enough to move work onto actually implementing the "internet" (mDNS/DNS-SD) case.
Attachment #8699124 - Attachment is obsolete: true
Attached patch internet-discovery.patch (obsolete) — Splinter Review
Initial start to internet discovery implementation.
Attached patch internet-discovery.patch (obsolete) — Splinter Review
Updated discovery patch.  This hooks into nsDNSServiceDiscovery, and looks for services.

Currently the policy is to do internet service discovery on a set interval.  Currently hardcoded to every 10 seconds (3 second scan duration, 10 second overall interval), just for debug testing.  Also hardcoded to discover '_http._tcp' services, since those exist on the network and I can see them already.

Seems to work well enough on desktop osx.
Attachment #8699289 - Attachment is obsolete: true
Attached patch internet-discovery.patch (obsolete) — Splinter Review
Updated internet discovery code.  FlyWebInternetServiceDiscoveryHandler now discovers and resolves '_http._tcp' services (only tested on OSX).

For this to work, you also need to include two bugfix patches currently open: 

https://bugzilla.mozilla.org/show_bug.cgi?id=1229726
https://bugzilla.mozilla.org/show_bug.cgi?id=1233813

These fix issues in mDNS code that gets in the way of discovery.
Attachment #8699646 - Attachment is obsolete: true
Attached patch internet-discovery.patch (obsolete) — Splinter Review
Updated internet discovery.  Wired up to 'listServices()' call on nsIDNSServiceManager so that internet-based services are listed.  Still doing hardcoded '_http._tcp' services though.
Attachment #8700180 - Attachment is obsolete: true
Attached patch firefox-discovery-ui.patch (obsolete) — Splinter Review
Discovery UI patch for firefox desktop - mostly written by mconley as a christmas gift (yay mconley!).  It adds a toolbar icon to list services.

It doesn't do anything with the listed services as of yet because there's an underlying memory error in my discovery implementation.  For some reason, when the UI obtains a nsIFlyWebServiceList, an later that list gets collected, it deletes the underlying nsIFlyWebServiceInfo objects, even though they should be held live by nsIFlyWebServiceManager service object.  Spent most of the day trying to track down why that was happening, but didn't get anywhere concrete.
Attached patch internet-discovery.patch (obsolete) — Splinter Review
Updated internet discovery patch.  Mostly some cleanups.  It's still buggy with the refcount issue.
Attachment #8701226 - Attachment is obsolete: true
Attached patch firefox-discovery-ui.patch (obsolete) — Splinter Review
Not updated discovery UI patch.  Just re-sending it to get the patch order correct.
Attachment #8701641 - Attachment is obsolete: true
Attached patch 1-flyweb-service-registry.patch (obsolete) — Splinter Review
New patch name, same patch (basic service discovery registry and xpcom API).
Attachment #8699288 - Attachment is obsolete: true
Attached patch 2-internet-discovery.patch (obsolete) — Splinter Review
Internet discovery code, now with refcount issues fixed.
Attachment #8701872 - Attachment is obsolete: true
Attached patch 3-firefox-discovery-ui.patch (obsolete) — Splinter Review
Same patch, new name.
Attachment #8701873 - Attachment is obsolete: true
Attached patch 01-webidl-flyweb-discovery.patch (obsolete) — Splinter Review
Updated implementation based off of 'larch' tree, using webidl to implement flyweb discovery.

This is not building yet.  Just learning my way around webidl.  I'm getting a strange error message with this patch.  Not sure why it wants me to implement "GetWrapperPreserveColor", but it seems more like something that should be autogenerated that is not getting autogenned because I forgot something:

 2:38.78 In file included from /Users/kannanvijayan/checkouts/larch/BUILD-DEBUG64/dom/bindings/UnifiedBindings6.cpp:50:
 2:38.78 ./FlyWebDiscoveryManagerBinding.cpp:142:21: error: no member named 'GetWrapperPreserveColor' in 'mozilla::dom::FlyWebDiscoveredService'
 2:38.79   if (self && self->GetWrapperPreserveColor()) {
 2:38.79               ~~~~  ^
 2:38.79 ./FlyWebDiscoveryManagerBinding.cpp:334:68: error: no member named 'GetParentObject' in 'mozilla::dom::FlyWebDiscoveredService'
 2:38.79   JS::Rooted<JSObject*> parent(aCx, WrapNativeParent(aCx, aObject->GetParentObject()));
 2:38.79                                                           ~~~~~~~  ^
 2:38.81 ./FlyWebDiscoveryManagerBinding.cpp:1035:68: error: no member named 'GetParentObject' in 'mozilla::dom::FlyWebDiscoveredServiceList'
 2:38.81   JS::Rooted<JSObject*> parent(aCx, WrapNativeParent(aCx, aObject->GetParentObject()));
 2:38.81                                                           ~~~~~~~  ^
 2:38.82 ./FlyWebDiscoveryManagerBinding.cpp:1237:21: error: no member named 'GetWrapperPreserveColor' in 'mozilla::dom::FlyWebDiscoveryManager'
 2:38.82   if (self && self->GetWrapperPreserveColor()) {
 2:38.82               ~~~~  ^
 2:38.82 ./FlyWebDiscoveryManagerBinding.cpp:1426:68: error: no member named 'GetParentObject' in 'mozilla::dom::FlyWebDiscoveryManager'
 2:38.82   JS::Rooted<JSObject*> parent(aCx, WrapNativeParent(aCx, aObject->GetParentObject()));
(In reply to Kannan Vijayan [:djvj] from comment #17)
> This is not building yet.  Just learning my way around webidl.  I'm getting
> a strange error message with this patch.  Not sure why it wants me to
> implement "GetWrapperPreserveColor", but it seems more like something that
> should be autogenerated that is not getting autogenned because I forgot
> something:
> 

Classes that implement a webidl must also inherit from nsWrapperCache and override a few methods.
dom/base/PerformanceEntry.* is a good example
Attached patch 01-webidl-flyweb-discovery.patch (obsolete) — Splinter Review
Thanks for that.  It helped out.

Updated patch that builds.  Just the interfaces and some basic mock implementation code so far.
Attachment #8706673 - Attachment is obsolete: true
(In reply to Kannan Vijayan [:djvj] from comment #19)
> Updated patch that builds.  Just the interfaces and some basic mock
> implementation code so far.

Also note that webidl definitions usually live under dom/webidl, and implementations somewhere in dom/. They usually call into the required networking things via XPCOM. Not sure if this is a hard rule though. Better ask one of the DOM peers.
Attached patch 01-webidl-flyweb-discovery.patch (obsolete) — Splinter Review
More filling in of WebIDL code.
Attachment #8707097 - Attachment is obsolete: true
Attached patch 01-webidl-flyweb-discovery.patch (obsolete) — Splinter Review
Updated WebIDL patch after figuring out more WebIDL stuff.  I can instantiate a FlyWebDiscoveryManager from chrome JS now, and have it hand me back an empty list of discovered services.

FlyWebDiscoveryManager, FlyWebDiscoveredServiceList, and FlyWebDiscoveredService objects all seem to work.  Now need to plumb them to actual discovery logic.
Attachment #8707181 - Attachment is obsolete: true
Attached patch 1-service-discovery-webidl.patch (obsolete) — Splinter Review
Updated base patch for service discovery infrastructure in webidl.  Builds and works.
Attachment #8707613 - Attachment is obsolete: true
Attached patch 2-mdns-discovery.patch (obsolete) — Splinter Review
New patch to wire in mDNS-based service discovery.  Not wired into UI yet but the objects can be instantiated and used from chrome JS, and listServices can be used to get a list of MDNS services.
Comment on attachment 8707880 [details] [diff] [review]
1-service-discovery-webidl.patch

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

Checking for initial feedback on discovery patches.  Jonas, can you take a quick look at this and let me know what you think?
Attachment #8707880 - Flags: feedback?(jonas)
Comment on attachment 8707883 [details] [diff] [review]
2-mdns-discovery.patch

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

Checking for initial feedback on discovery patches.  Jonas, can you take a quick look at this and let me know what you think?
Attachment #8707883 - Flags: feedback?(jonas)
Attached patch 3-flyweb-ui.patch (obsolete) — Splinter Review
Added flyweb ui patch.
Attachment #8708055 - Flags: feedback?(jonas)
Attachment #8708055 - Flags: feedback?(jdarcangelo)
(In reply to Valentin Gosu [:valentin] from comment #20)
> Classes that implement a webidl must also inherit from nsWrapperCache and
> override a few methods.
> dom/base/PerformanceEntry.* is a good example

Thanks!  That was very useful.
Attached patch 1-service-discovery-webidl.patch (obsolete) — Splinter Review
Update service discovery patch, all rolled into one.  Working off of larch tip.
Attachment #8702669 - Attachment is obsolete: true
Attachment #8702670 - Attachment is obsolete: true
Attachment #8702671 - Attachment is obsolete: true
Attachment #8707880 - Attachment is obsolete: true
Attachment #8707883 - Attachment is obsolete: true
Attachment #8708055 - Attachment is obsolete: true
Attachment #8707880 - Flags: feedback?(jonas)
Attachment #8707883 - Flags: feedback?(jonas)
Attachment #8708055 - Flags: feedback?(jonas)
Attachment #8708055 - Flags: feedback?(jdarcangelo)
Attached patch 2-add-connection-logic.patch (obsolete) — Splinter Review
Patch that adds 'connectToService' method to the internal FlyWebDiscoveryManager API.  Also changes the flyweb UI button to open pages to the connected services when menu items are clicked.

API is not promise-based or anything yet, but we now have a full UI for discovering and opening webpages to '_http._tcp' pages.
Attachment #8709196 - Attachment is obsolete: true
Attachment #8709197 - Attachment is obsolete: true
Last patch in this series enables discovery of both '_http._tcp' and '_flyweb._tcp' services.
Attachment #8709623 - Flags: review?(jonas)
Attachment #8709624 - Flags: review?(jonas)
Attachment #8709625 - Flags: review?(jonas)
Closing this bug as it has landed in larch.

http://hg.mozilla.org/projects/larch/rev/e426071f5f3c
http://hg.mozilla.org/projects/larch/rev/40183f878ffb
http://hg.mozilla.org/projects/larch/rev/bfa03eb798e9
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: