721540 - Incorrectly resolving new-style path= arguments for Bonjour hosts

Status: RESOLVED FIXED

(Camino Graveyard :: OS Integration, defect)

Description

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

On 10.5 (at least), a the local host's user's website (~username) has an address that's advertised as "path=~username/".

On 10.3, this was advertised as "path=/~username/".

Our Bonjour code can't handle (or mishandles) the new-style format, so that any Camino version resolves a new-style path argument to the root site, rather than the user's site.  We somehow need to detect the "path=no-leading-slash" case and rewrite it to "path=/stuff" like everyone else.

STR:
0) On 10.5, customize index.html in your user's Sites folder and turn on Web sharing.
1) On any version of Camino running on any OS version, try to open the site from the Bonjour collection.

ER: http://foo-bar.local./~username/
AR: http://foo-bar.local./

Safari and OmniWeb properly resolve the new-style path arguments to the correct sites.

(You can repeat the STR with a step-0 host running 10.3.9, where the path argument apparently contained the leading "/", and you'll have success in step 1 instead of failure.)

This is an easy fix, if someone can explain what all the C manipulation in the if (pathData) block is doing :P

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: possible fix

> This is an easy fix, if someone can explain what all the C manipulation in
> the if (pathData) block is doing :P

Well, it turned out to also be an easy fix by just ignoring all the bizarre C manipulation and working only with the NSString at the end. :P  So this is _a_ fix; dunno if it's the right one.

(What is all that bizarre C stuff doing?  Why not just use NSString's initWithData:encoding: instead?)

Comment 2

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

(In reply to comment #1)
> (What is all that bizarre C stuff doing?  Why not just use NSString's
> initWithData:encoding: instead?)

[5:56pm] sauron: ilya: if you have a sec, can you explain what the C-goop inside if (pathData) is doing? http://mxr.mozilla.org/camino/source/camino/src/rendezvous/NetworkServices.mm#316 (and possibly why we didn't just use NSString initWithData:encoding: instead?)
[5:58pm] ilya: looking
[6:02pm] ilya: sauron: yeah, it's just copying the NSData into a C string and then copying the data back out of that into an NSString (and specifying the utf8 encoding for parsing)
[6:02pm] ilya: afaict initWithData:encoding: would do the same thing, and be much cleaner

So I can clean up that if block, too, if that's OK.

Comment 3

[Stuart Morgan]

Comment on attachment 592036 [details] [diff] [review]
possible fix

Might be clearer as path = [@"/" stringByAppendingString:path], but sr=smorgan either way.

It looks from the original bug like that C code was probably an artifact of the way the patch evolved, but before changing it do make sure that if the NSData is zero-length we get @"" and not nil from initWithData:encoding:, since otherwise the behavior will change (in which case we would want to still change it, but add a nil check and then assign @"")

Comment 4

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Fix with sr comment addressed and C-goop removed

> Might be clearer as path = [@"/" stringByAppendingString:path], but

Oh, yes, lots!  Somehow I ruled that out incorrectly :o:

Here's the version that also replaces 6 lines of inscrutable C-goop with 3 lines of understandable Obj-C.

To test, I force-fed it a 0-length pathData ([NSData dataWithBytes:[@"" UTF8String] length:strlen([@"" UTF8String])]), and initWithData in that case gives us an NSString of @"", so all is well.

One last question: do we have a convention about where to release?  Is it better to release right after you're done with the thing?  I put this one at the end of the if because that's where the C-goop's free was.

Also, I can land this in two parts if you'd prefer to keep the blame separate; otherwise, I'll just use a checkin comment that clearly references both changes.

Comment 5

[Stuart Morgan]

Comment on attachment 592857 [details]
Fix with sr comment addressed and C-goop removed

>+            NSString* pathString = [[NSString alloc] initWithData:pathData
>+                                                         encoding:NSUTF8StringEncoding];

Pink's rule has always been to autorelease at the time of creation, the minor overhead of autoreleasing being than the possibility of a leak later. So this should be = [[[NSString alloc] init...] autorelease]; and then no release call later.

sr=smorgan with that change.

Comment 6

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

http://hg.mozilla.org/camino/rev/b5821c0d34a3 with the autorelease.