Implement FlyWeb service discovery

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: djvj, Assigned: djvj)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 25 obsolete attachments)

56.56 KB, patch
Details | Diff | Splinter Review
16.53 KB, patch
Details | Diff | Splinter Review
7.98 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

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

Comment 1

2 years ago
Created attachment 8693081 [details] [diff] [review]
flyweb-service-discovery.patch

Added some IDL and basic stub implementation code.  I am new to XPCOM, IDLs, and gecko/necko in general, so learning as I go.
(Assignee)

Comment 2

2 years ago
Created attachment 8693126 [details] [diff] [review]
flyweb-service-discovery.patch
Attachment #8693081 - Attachment is obsolete: true
(Assignee)

Comment 3

2 years ago
Created attachment 8698236 [details] [diff] [review]
flyweb-service-discovery.patch

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

Comment 4

2 years ago
Created attachment 8698604 [details] [diff] [review]
flyweb-service-discovery.patch

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

Comment 5

2 years ago
Created attachment 8699124 [details] [diff] [review]
flyweb-service-discovery.patch

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

Comment 6

2 years ago
Created attachment 8699288 [details] [diff] [review]
flyweb-service-discovery.patch

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

Comment 7

2 years ago
Created attachment 8699289 [details] [diff] [review]
internet-discovery.patch

Initial start to internet discovery implementation.
(Assignee)

Comment 8

2 years ago
Created attachment 8699646 [details] [diff] [review]
internet-discovery.patch

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

Comment 9

2 years ago
Created attachment 8700180 [details] [diff] [review]
internet-discovery.patch

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

Comment 10

2 years ago
Created attachment 8701226 [details] [diff] [review]
internet-discovery.patch

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

Comment 11

2 years ago
Created attachment 8701641 [details] [diff] [review]
firefox-discovery-ui.patch

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

Comment 12

2 years ago
Created attachment 8701872 [details] [diff] [review]
internet-discovery.patch

Updated internet discovery patch.  Mostly some cleanups.  It's still buggy with the refcount issue.
Attachment #8701226 - Attachment is obsolete: true
(Assignee)

Comment 13

2 years ago
Created attachment 8701873 [details] [diff] [review]
firefox-discovery-ui.patch

Not updated discovery UI patch.  Just re-sending it to get the patch order correct.
Attachment #8701641 - Attachment is obsolete: true
(Assignee)

Comment 14

2 years ago
Created attachment 8702669 [details] [diff] [review]
1-flyweb-service-registry.patch

New patch name, same patch (basic service discovery registry and xpcom API).
Attachment #8699288 - Attachment is obsolete: true
(Assignee)

Comment 15

2 years ago
Created attachment 8702670 [details] [diff] [review]
2-internet-discovery.patch

Internet discovery code, now with refcount issues fixed.
Attachment #8701872 - Attachment is obsolete: true
(Assignee)

Comment 16

2 years ago
Created attachment 8702671 [details] [diff] [review]
3-firefox-discovery-ui.patch

Same patch, new name.
Attachment #8701873 - Attachment is obsolete: true
(Assignee)

Comment 17

2 years ago
Created attachment 8706673 [details] [diff] [review]
01-webidl-flyweb-discovery.patch

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

Comment 19

2 years ago
Created attachment 8707097 [details] [diff] [review]
01-webidl-flyweb-discovery.patch

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

Comment 21

2 years ago
Created attachment 8707181 [details] [diff] [review]
01-webidl-flyweb-discovery.patch

More filling in of WebIDL code.
Attachment #8707097 - Attachment is obsolete: true
(Assignee)

Comment 22

2 years ago
Created attachment 8707613 [details] [diff] [review]
01-webidl-flyweb-discovery.patch

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

Comment 23

2 years ago
Created attachment 8707880 [details] [diff] [review]
1-service-discovery-webidl.patch

Updated base patch for service discovery infrastructure in webidl.  Builds and works.
Attachment #8707613 - Attachment is obsolete: true
(Assignee)

Comment 24

2 years ago
Created attachment 8707883 [details] [diff] [review]
2-mdns-discovery.patch

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

Comment 25

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

Comment 26

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

Comment 27

2 years ago
Created attachment 8708055 [details] [diff] [review]
3-flyweb-ui.patch

Added flyweb ui patch.
Attachment #8708055 - Flags: feedback?(jonas)
Attachment #8708055 - Flags: feedback?(jdarcangelo)
(Assignee)

Comment 28

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

Comment 29

2 years ago
Created attachment 8709196 [details] [diff] [review]
1-service-discovery-webidl.patch

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

Comment 30

2 years ago
Created attachment 8709197 [details] [diff] [review]
2-add-connection-logic.patch

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

Comment 31

2 years ago
Created attachment 8709623 [details] [diff] [review]
1-service-discovery-webidl.patch
Attachment #8709196 - Attachment is obsolete: true
(Assignee)

Comment 32

2 years ago
Created attachment 8709624 [details] [diff] [review]
2-add-connection-logic.patch
Attachment #8709197 - Attachment is obsolete: true
(Assignee)

Comment 33

2 years ago
Created attachment 8709625 [details] [diff] [review]
3-add-fw-discovery.patch

Last patch in this series enables discovery of both '_http._tcp' and '_flyweb._tcp' services.
(Assignee)

Updated

2 years ago
Attachment #8709623 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Attachment #8709624 - Flags: review?(jonas)
(Assignee)

Updated

2 years ago
Attachment #8709625 - Flags: review?(jonas)
(Assignee)

Comment 34

2 years ago
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
Last Resolved: 2 years ago
Resolution: --- → FIXED
Attachment #8709623 - Flags: review?(jonas)
Attachment #8709624 - Flags: review?(jonas)
Attachment #8709625 - Flags: review?(jonas)
You need to log in before you can comment on or make changes to this bug.