The default bug view has changed. See this FAQ.

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

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
11 months ago
10 months ago

People

(Reporter: djvj, Assigned: djvj)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

11 months ago
Linux build crashes in FlyWebMDNSService::DiscoveredInfo::DiscoveredInfo when trying to get the attributes of the discovered service.
(Assignee)

Comment 1

11 months ago
Created attachment 8752214 [details] [diff] [review]
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.
Attachment #8752214 - Flags: review?(jonas)
Attachment #8752214 - Flags: review?(jdarcangelo)
Attachment #8752214 - Flags: review?(jdarcangelo) → review+
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)
Attachment #8752214 - Flags: review?(jonas) → review+
(Assignee)

Comment 3

10 months ago
Created attachment 8752334 [details] [diff] [review]
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.
Attachment #8752214 - Attachment is obsolete: true
(Assignee)

Updated

10 months ago
Attachment #8752334 - Flags: review?(jonas)
Attachment #8752334 - Flags: review?(jdarcangelo)
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?
Attachment #8752334 - Flags: review?(jdarcangelo) → review+
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?
Attachment #8752334 - Flags: review?(jonas)
Attachment #8752334 - Flags: review?(jdarcangelo)
Attachment #8752334 - Flags: review+
(Assignee)

Comment 6

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

Comment 7

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

Comment 8

10 months ago
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.
Attachment #8752334 - Flags: review?(jdarcangelo) → review+
(Assignee)

Comment 9

10 months ago
https://hg.mozilla.org/projects/larch/rev/03634caa1ea1
(Assignee)

Updated

10 months ago
Blocks: 1228662

Updated

10 months ago
Assignee: nobody → kvijayan
Status: NEW → ASSIGNED
Whiteboard: [necko-active]
Kanna: should this land in m-c too?
Flags: needinfo?(kvijayan)
(Assignee)

Comment 11

10 months ago
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.
Flags: needinfo?(kvijayan)
(Assignee)

Updated

10 months ago
Status: ASSIGNED → RESOLVED
Last Resolved: 10 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.