1272678 - [FlyWeb] Crash on linux in FlyWebMDNSService::DiscoveredInfo::DiscoveredInfo when trying to get attributes

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Kannan Vijayan [:djvj]]

Linux build crashes in FlyWebMDNSService::DiscoveredInfo::DiscoveredInfo when trying to get the attributes of the discovered service.

Comment 1

[Kannan Vijayan [:djvj]]

Attachment: fix-attributes-on-linux.patch

There were a few issues identified that are all fixed in this patch.

1: The attributes weren't getting forwarded from the fallback implementation of MulticastDNS.jsm, to the abstract DNSServiceDiscovery API implementation in nsDNSServiceDiscovery.js.  This has been fixed.

2: A couple places where UniquePtr.release() is used in an argument list, where a prior argument derefenced (via arrow) the uniqueptr.  C doesn't guarantee order-of-evaluation of function argument expressions, and this was leading to a crash.

3: TXT record parsing converted an empty TXT string into an object {"":""}, instead of an empty object.  Fixed that.

Comment 2

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

Comment on attachment 8752214 [details] [diff] [review]
fix-attributes-on-linux.patch

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

r=me with the below fixed

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
@@ +64,5 @@
> +      if (aServiceInfo.attributes.QueryInterface) {
> +        let attrEnum = aServiceInfo.attributes.enumerator;
> +        while (attrEnum.hasMoreElements()) {
> +          let prop = attrEnum.getNext();
> +          attributes.setPropertyAsAString(prop.name, prop.value.toString());

I don't think you need to call .toString() here. That'll be done automatically by the setPropertyAsAString implementation (or, more specifically, by XPConnect)

@@ +73,5 @@
> +        }
> +      }
> +    } catch (err) {
> +        log("Caught unset attributes error: " + err + " - " + err.stack);
> +        // Ignore unset attributes in object.

We should probably more gracefully handle aServiceInfo.attributes being null. Per the API documentation that's not an unusual case at all, and the publishServer code will commonly generate serviceInfo objects with a null attributes (at least until we turn on https by default)

Comment 3

[Kannan Vijayan [:djvj]]

Attachment: fix-attributes-on-linux.patch

Updated patch.  The previous patch fixed attributes handling on the publish-side, but did not handle attributes-passing issues on the discovery side.

Comment 4

[Justin D'Arcangelo [:justindarc]]

Comment on attachment 8752334 [details] [diff] [review]
fix-attributes-on-linux.patch

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

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
@@ +192,5 @@
> +{
> +  let result = Cc['@mozilla.org/hash-property-bag;1']
> +            .createInstance(Ci.nsIWritablePropertyBag2);
> +  let propBag = obj.QueryInterface && obj.QueryInterface(Ci.nsIPropertyBag2);
> +  if (propBag) {

If `obj` is already a nsIPropertyBag2, can't we just return it here or do we need it to be writable elsewhere?

Comment 5

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

Comment on attachment 8752334 [details] [diff] [review]
fix-attributes-on-linux.patch

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

Looks good, but Justin should review the mDNS changes.

::: netwerk/dns/mdns/libmdns/fallback/DNSResourceRecord.jsm
@@ +146,5 @@
> +    let parts = label.split('.');
> +    parts.forEach((part) => {
> +      let pair  = part.split('=');
> +      let name  = pair.shift();
> +      let value = pair.join('=');

Somewhat smaller:

let [name] = part.split('=', 1);
let value = part.substr(name.length + 1);

::: netwerk/dns/mdns/libmdns/fallback/MulticastDNS.jsm
@@ +508,5 @@
>          host: host,
>          address: address,
>          port: port,
>          domainName: domainName,
> +        attributes: services[name].attributes || {}

A ',' at the end would simplify future changes. But not sure what the common style is...

@@ +746,5 @@
> +function _propertyBagToObject(propBag) {
> +  let result = {};
> +  if (propBag.QueryInterface) {
> +    propBag = propBag.QueryInterface(Ci.nsIPropertyBag2);
> +    if (propBag) {

QueryInterface doesn't actually return a new object in JS. So simply

propBag.QueryInterface(...);

is fine. Also means that the if-statement is a no-op.

@@ +750,5 @@
> +    if (propBag) {
> +      let propEnum = propBag.enumerator;
> +      while (propEnum.hasMoreElements()) {
> +        let prop = propEnum.getNext().QueryInterface(Ci.nsIProperty);
> +        result[prop.name] = prop.value.toString();

I think calling .getAsAString() is safer than calling toString() here.

I guess toString() works because there's magic in XPConnect which knows how to stringify nsIVariants, but I'd rather not rely on that I think.

::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
@@ +60,5 @@
>  
> +    let attributes;
> +    try {
> +      attributes = _toPropertyBag2(aServiceInfo.attributes);
> +    } catch (err) {

Could we make _toPropertyBag2 catch exceptions and return an empty propertybag instead?

@@ +64,5 @@
> +    } catch (err) {
> +        // Ignore unset attributes in object.
> +        log("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");
> +        log("Caught unset attributes error: " + err + " - " + err.stack);
> +        log("!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!!");

This seems a little excessive?

@@ +192,5 @@
> +{
> +  let result = Cc['@mozilla.org/hash-property-bag;1']
> +            .createInstance(Ci.nsIWritablePropertyBag2);
> +  let propBag = obj.QueryInterface && obj.QueryInterface(Ci.nsIPropertyBag2);
> +  if (propBag) {

Could we simply call _propertyBagToObject(obj) and just use the else-branch?

Comment 6

[Kannan Vijayan [:djvj]]

(In reply to Jonas Sicking (:sicking) from comment #5)
> Somewhat smaller:
> 
> let [name] = part.split('=', 1);
> let value = part.substr(name.length + 1);

Done.

> A ',' at the end would simplify future changes. But not sure what the common
> style is...

I'm willing to go either way.  I don't have a preference other than consistency.  The rest of the file doesn't use terminal ','s on objects, so I left it out.

> 

> QueryInterface doesn't actually return a new object in JS. So simply
> 
> propBag.QueryInterface(...);
> 
> is fine. Also means that the if-statement is a no-op.

Did not know that.  Ok fixing.


> I think calling .getAsAString() is safer than calling toString() here.
> 
> I guess toString() works because there's magic in XPConnect which knows how
> to stringify nsIVariants, but I'd rather not rely on that I think.

Done.

> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
> @@ +60,5 @@
> >  
> > +    let attributes;
> > +    try {
> > +      attributes = _toPropertyBag2(aServiceInfo.attributes);
> > +    } catch (err) {
> 
> Could we make _toPropertyBag2 catch exceptions and return an empty
> propertybag instead?

The problem here isn't _toPropertyBag2 catching exceptions.  It's that |aServiceInfo.attributes| itself can throw, if the attributes field is not set on the nsIDNSServiceInfo instance.  SO we need the catch fallback to initialize an empty propbag even if _toPropertyBag2 caught errors.

> This seems a little excessive?

Heh, sorry.  Put that in to make it easier in the logs when debugging.  It's out now.

> Could we simply call _propertyBagToObject(obj) and just use the else-branch?

_propertyBagToObject is in the MulticastDNS.jsm file.  It feels a bit wasteful creating an intermediate object for no real reason.

Comment 7

[Kannan Vijayan [:djvj]]

(In reply to Justin D'Arcangelo [:justindarc] from comment #4)
> Comment on attachment 8752334 [details] [diff] [review]
> fix-attributes-on-linux.patch
> 
> Review of attachment 8752334 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/dns/mdns/libmdns/nsDNSServiceDiscovery.js
> @@ +192,5 @@
> > +{
> > +  let result = Cc['@mozilla.org/hash-property-bag;1']
> > +            .createInstance(Ci.nsIWritablePropertyBag2);
> > +  let propBag = obj.QueryInterface && obj.QueryInterface(Ci.nsIPropertyBag2);
> > +  if (propBag) {
> 
> If `obj` is already a nsIPropertyBag2, can't we just return it here or do we
> need it to be writable elsewhere?

Done.

Comment 8

[Kannan Vijayan [:djvj]]

Comment on attachment 8752334 [details] [diff] [review]
fix-attributes-on-linux.patch

Forwarding r+ from justin since he reviewed the mdns stuff and cleared his r? for the second patch already.

Comment 9

[Kannan Vijayan [:djvj]]

https://hg.mozilla.org/projects/larch/rev/03634caa1ea1

Comment 10

[Jason Duell]

Kanna: should this land in m-c too?

Comment 11

[Kannan Vijayan [:djvj]]

It should have gotten uplifted from the review patches.  The patches we sent for review for landing on trunk were made from taking a big diff between larch and m-i, and then splitting those up into the relevant sections.

These changes are already in.