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

VERIFIED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Cocoa
VERIFIED FIXED
10 years ago
9 years ago

People

(Reporter: Jesse Ruderman, Assigned: Josh Aas)

Tracking

({verified1.9.1})

Trunk
mozilla1.9.2a1
x86
Mac OS X
verified1.9.1
Points:
---
Bug Flags:
blocking1.9.1 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Comment 1

10 years ago
Luis, can you take a look at this?
Assignee: joshmoz → bugzilla

Comment 2

10 years ago
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

10 years ago
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.
Status: NEW → ASSIGNED

Comment 4

10 years ago
Created attachment 332282 [details] [diff] [review]
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.
Attachment #332282 - Flags: review?(joshmoz)
(Reporter)

Comment 5

10 years ago
Be sure to convert tabs to spaces before checking in.
(Assignee)

Comment 6

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

Comment 7

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

Luis - can you update your patch?
Flags: blocking1.9.1+
(Assignee)

Comment 8

10 years ago
Created attachment 335544 [details] [diff] [review]
fix v1.1
Attachment #332282 - Attachment is obsolete: true
Attachment #335544 - Flags: review?(mstange)
Attachment #332282 - Flags: review?(joshmoz)
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?
Attachment #335544 - Flags: review?(mstange) → review+
(Assignee)

Comment 10

10 years ago
Created attachment 335570 [details] [diff] [review]
fix v1.2
Attachment #335544 - Attachment is obsolete: true
Attachment #335570 - Flags: superreview?(roc)
(Assignee)

Updated

10 years ago
Attachment #335570 - Flags: superreview?(roc)
(Assignee)

Comment 11

10 years ago
Created attachment 335575 [details] [diff] [review]
fix v1.3
Attachment #335570 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
I can't test this because I can't get Adium to pick up links from Firefox. I'm using Adium 1.3.
I can confirm that the patch fixes the bug. My Adium 1.3 picks up links from Firefox just fine.
(Assignee)

Updated

10 years ago
Attachment #335575 - Flags: superreview?(roc)
+      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.
(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).
(Assignee)

Comment 16

10 years ago
> +      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.
(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.
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.
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?
(Assignee)

Comment 20

10 years ago
Created attachment 336675 [details] [diff] [review]
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.
Attachment #335575 - Attachment is obsolete: true
Attachment #336675 - Flags: superreview?(roc)
Attachment #335575 - Flags: superreview?(roc)
(Assignee)

Comment 21

10 years ago
Created attachment 336677 [details] [diff] [review]
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.
Attachment #336675 - Attachment is obsolete: true
Attachment #336677 - Flags: superreview?(roc)
Attachment #336675 - Flags: superreview?(roc)
Attachment #336677 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

10 years ago
Assignee: bugzilla → joshmoz
(Assignee)

Comment 22

10 years ago
landed on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Created attachment 337109 [details]
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.
Marcia, is that a regression from Firefox 2 or can we call this bug verified?
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).
Keywords: fixed1.9.1

Comment 26

10 years ago
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

10 years ago
Created attachment 363127 [details] [diff] [review]
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.
Attachment #363127 - Flags: review?(joshmoz)
(Assignee)

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 28

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

Please add "::" before "CFStringCreateWithCString".
Attachment #363127 - Flags: superreview?(roc)
Attachment #363127 - Flags: review?(joshmoz)
Attachment #363127 - Flags: review+
Keywords: fixed1.9.1

Comment 29

10 years ago
Created attachment 363130 [details] [diff] [review]
Updated patch that correctly returns the window title, even with non-ASCII v2.

This adds "::" before "CFStringCreateWithCString" per josh's comment.
Attachment #363127 - Attachment is obsolete: true
Attachment #363130 - Flags: superreview?
Attachment #363127 - Flags: superreview?(roc)

Updated

10 years ago
Attachment #363130 - Flags: superreview? → superreview?(roc)
Attachment #363130 - Flags: superreview?(roc) → superreview+
(Assignee)

Comment 30

10 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/5145eee380f4
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 31

10 years ago
pushed to mozilla-1.9.1

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0591df37c782
Keywords: fixed1.9.1
Target Milestone: --- → mozilla1.9.2a1
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
Status: RESOLVED → VERIFIED
Keywords: fixed1.9.1 → verified1.9.1
You need to log in before you can comment on or make changes to this bug.