448087 - AppleScript curl/pTit: non-ASCII character in title comes through incorrectly

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Jesse Ruderman]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1a2pre) Gecko/2008072522 Minefield/3.1a2pre

Steps to reproduce:
1. Make sure you have only one Firefox process running.
2. In Firefox, open bug 427448.
3. In Adium 1.2.7, open a message window.
4. Click the "Insert link to active page" button in the Adium message window (which for some reason uses the Safari icon).

Result in Firefox trunk:
"Bug 427448 – Applescript curl & ptit query worked in FF2 fails in FF3"

Result in Firefox 2.0.0.16, which is as expected:
"Bug 427448 – Applescript curl & ptit query worked in FF2 fails in FF3"

Comment 1

[Josh Aas]

Luis, can you take a look at this?

Comment 2

[Luis de la Rosa]

I took a look this weekend and reproduced this.  However, I found that if I added printfs, the string printed out correctly in Console.app.  So internally the string is correct.

The code with printfs looks like:
	const char* cTitle = NS_ConvertUTF16toUTF8(title).get();
	printf("cTitle:%s\n", cTitle);
	strncpy(outName, cTitle, maxLen);
	printf("outName1:%s\n", outName);
	outName[maxLen - 1] = '\0';
	printf("outName2:%s\n", outName);

The other code that calls GetCleanedWindowName and converts it to AppleScript did not change, so that should not have caused this breakage.  I think that the previous code which retrieved the window name may have done something special, specifically in the call CopyPascalToCString.

Can someone who knows more about CopyPascalToCString or the Mozilla string formats look into this as well?

Comment 3

[Luis de la Rosa]

After some investigation into the Firefox 2.x source, I figured out that Apple Events are expecting the strings that come across to be encoded in MacRoman.  I looked at the raw bytes of the chars for both the 2.x and 3.0.x branches and found how the ndash char that Bugzilla uses in the title was encoded in various encodings.  In Firefox 2, it was coming across as d0 which is how ndash is represented MacRoman.

So I've made a patch that converts the characters from UTF-8 encoding to MacRoman.

Comment 4

[Luis de la Rosa]

Attachment: Fixes the problem with getting the title via AppleScript and handles non-ASCII

This is the same as the patch https://bugzilla.mozilla.org/attachment.cgi?id=329689&action=edit but it changes the encoding from UTF-8 to MacRoman.  Apple Events expects this encoding, so this will ensure that Firefox 3 has the same behavior as Firefox 2 when retrieving the title via AppleScript.

Comment 5

[Jesse Ruderman]

Be sure to convert tabs to spaces before checking in.

Comment 6

[Josh Aas]

Comment on attachment 332282 [details] [diff] [review]
Fixes the problem with getting the title via AppleScript and handles non-ASCII

Please fix the tabbing in the patch (two-space indentation) as Jesse mentioned.

Also, if "converted" is false then "windowTitleAsCFString" will leak.

Comment 7

[Josh Aas]

We should fix this for 1.9.1 since we already took 427448.

Luis - can you update your patch?

Comment 8

[Josh Aas]

Attachment: fix v1.1

Comment 9

[Steven Michaud [:smichaud] (Retired)]

Comment on attachment 335544 [details] [diff] [review]
fix v1.1

Looks fine to me, but I have one minor quibble:

Shouldn't you initialize outName[0] to '\0', in case the conversion
fails?

Comment 10

[Josh Aas]

Attachment: fix v1.2

Comment 11

[Josh Aas]

Attachment: fix v1.3

Comment 12

[Josh Aas]

I can't test this because I can't get Adium to pick up links from Firefox. I'm using Adium 1.3.

Comment 13

[Markus Stange [:mstange]]

I can confirm that the patch fixes the bug. My Adium 1.3 picks up links from Firefox just fine.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+      strncpy(outName, windowTitleMacRoman, PR_MIN(maxLen, 255));

What's this PR_MIN for?

Maybe I'm being a bit picky, but how about moving nsLocalFile::CFStringRefToUTF8 (in nsLocalFileOSX.h/.cpp) somewhere more generally useful (some string utils file?) and letting it take any Mac encoding parameter. Then we wouldn't have this 255 limit, which gives me a bad flashback of Turbo Pascal in the 1980s.

Comment 15

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

(In reply to comment #14)

> Maybe I'm being a bit picky, but how about moving
> nsLocalFile::CFStringRefToUTF8 (in nsLocalFileOSX.h/.cpp) somewhere more
> generally useful (some string utils file?) and letting it take any Mac encoding
> parameter.

See also bug 334670 (there was another reference to this sort of desire somewhere else in Cocoa widgets a couple years ago, but I can't find it now).

Comment 16

[Josh Aas]

> +      strncpy(outName, windowTitleMacRoman, PR_MIN(maxLen, 255));
> 
> What's this PR_MIN for?

We have a string in CFString form which could be any length. We don't want to attempt to fill our return buffer with any more characters than are specified by the max length in-parameter or the size limit for the CStr255.

> Maybe I'm being a bit picky, but how about moving
> nsLocalFile::CFStringRefToUTF8 (in nsLocalFileOSX.h/.cpp) somewhere more
> generally useful (some string utils file?) and letting it take any Mac encoding
> parameter. Then we wouldn't have this 255 limit, which gives me a bad flashback
> of Turbo Pascal in the 1980s.

We need to kill off all of this old AppleScript code soon anyway, and anything else using CStr255, I don't think we should invest in string architecture on account of this problem and generally CFString isn't tricky to use.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #16)
> > +      strncpy(outName, windowTitleMacRoman, PR_MIN(maxLen, 255));
> > 
> > What's this PR_MIN for?
> 
> We have a string in CFString form which could be any length. We don't want to
> attempt to fill our return buffer with any more characters than are specified
> by the max length in-parameter or the size limit for the CStr255.

OK, so if maxLen==1000 then it seems to me we fill windowTitleMacRoman with 255 (non-null) characters, then we do a strncpy of 255 (non-null) characters into outName, then we set outName[999] to 0, thus returning a string containing 255 real characters and 744 characters of uninitialized garbage. Right?

Seems to me the simplest thing to do would be to set maxLen = PR_MIN(maxLen, 255) as early as possible.

> > Maybe I'm being a bit picky, but how about moving
> > nsLocalFile::CFStringRefToUTF8 (in nsLocalFileOSX.h/.cpp) somewhere more
> > generally useful (some string utils file?) and letting it take any Mac encoding
> > parameter. Then we wouldn't have this 255 limit, which gives me a bad flashback
> > of Turbo Pascal in the 1980s.
> 
> We need to kill off all of this old AppleScript code soon anyway, and anything
> else using CStr255, I don't think we should invest in string architecture on
> account of this problem and generally CFString isn't tricky to use.

Copying an arbitrary-length CFString to an nsA(C)String obviously is tricky, since you haven't done it here. I don't think the small amount of effort to make the nsLocalFile method more widely available, with an extra encoding parameter, will be wasted even if this code goes away.

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess the first point comes down to whether CStr255's buffer is always null-terminated. I can't find any documentation on that. I'd prefer to not have to care.

Comment 19

[Robert O'Callahan (:roc) (email my personal email if necessary)]

OK, so various fragments around the Web suggest it is always null-terminated. But then why would the PR_MIN be needed, since strncpy will stop as soon as it hits that null byte in windowTitleMacRoman?

Comment 20

[Josh Aas]

Attachment: fix v1.4

I agree that PR_MIN is unnecessary, this fixes that.

This also simplifies the string conversion by just using a different CFString creation method that can take UTF16 as an in-encoding parameter.

Again, I don't think we need a utility method. The only reason this is complicated is because the Carbon AE/Window stuff expects pascal-like (255 char limit) macroman encoded strings (which we're doing away with). I don't see how nsLocalFile::CFStringRefToUTF8 or a version of it that takes an encoding helps us here. We'd also have to figure out a convenient place for such a thing to live to be useful across modules, a pain.

Comment 21

[Josh Aas]

Attachment: fix v1.5

Ugh, we can probably even just do this. Either I am overlooking something or we should have just done this from the git-go.

Comment 22

[Josh Aas]

landed on trunk

Comment 23

[Marcia Knous [:marcia]]

Attachment: Screenshot of Adium selection dialog

I tested this with Adium 1.3 using Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b1pre) Gecko/20080905020415 Minefield/3.1b1pre and all my Firefox links come across as "(Untitled Page) and a link to the bug URL (no text). Safari seemed to handle the links properly and showed "Bug 427448 – Applescript curl & ptit query worked in FF2 fails in FF3" and the link to the bug.

Comment 24

[Samuel Sidler (old account; do not CC)]

Marcia, is that a regression from Firefox 2 or can we call this bug verified?

Comment 25

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

Marcia's right; "«class pTit» of window 1" always returns a blank/empty result in builds with this patch (I tested 3.1b2pre, 20081028020258), even for "ASCII-only" window titles.

Prior to this patch, getting the window title worked but had issues with non-ASCII characters (thus this bug), and of course getting the window title worked properly in Firefox 2 (thus bug 427448).

Comment 26

[Luis de la Rosa]

I verified that the current patch doesn't work, echoing what was found in comment #23 and #25.  But I have another fix that corrects the problem that I will post soon.

Comment 27

[Luis de la Rosa]

Attachment: Updated patch that correctly returns the window title, even with non-ASCII

The last patch had a bug - windowTitleCFString ended up empty due to the CFStringCreateWithCharacters call.  I changed it to use a previous technique which worked in the original patch.  I think this patch combines the best of the original and the latest.

I tested this with mozilla-central and verified that it works properly.

Comment 28

[Josh Aas]

Comment on attachment 363127 [details] [diff] [review]
Updated patch that correctly returns the window title, even with non-ASCII

Please add "::" before "CFStringCreateWithCString".

Comment 29

[Luis de la Rosa]

Attachment: Updated patch that correctly returns the window title, even with non-ASCII v2.

This adds "::" before "CFStringCreateWithCString" per josh's comment.

Comment 30

[Josh Aas]

pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/5145eee380f4

Comment 31

[Josh Aas]

pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0591df37c782

Comment 32

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090401 Minefield/3.6a1pre ID:20090401031047

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 ID:20090305133223