Closed
Bug 448087
Opened 16 years ago
Closed 16 years ago
AppleScript curl/pTit: non-ASCII character in title comes through incorrectly
Categories
(Core :: Widget: Cocoa, defect)
Tracking
()
VERIFIED
FIXED
mozilla1.9.2a1
People
(Reporter: jruderman, Assigned: jaas)
References
Details
(Keywords: verified1.9.1)
Attachments
(3 files, 6 obsolete files)
2.02 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
17.06 KB,
image/png
|
Details | |
1.24 KB,
patch
|
roc
:
superreview+
|
Details | Diff | Splinter Review |
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"
Luis, can you take a look at this?
Assignee: joshmoz → bugzilla
Comment 2•16 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•16 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•16 years ago
|
||
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•16 years ago
|
||
Be sure to convert tabs to spaces before checking in.
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.
We should fix this for 1.9.1 since we already took 427448. Luis - can you update your patch?
Flags: blocking1.9.1+
Attachment #332282 -
Attachment is obsolete: true
Attachment #335544 -
Flags: review?(mstange)
Attachment #332282 -
Flags: review?(joshmoz)
Comment 9•16 years ago
|
||
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•16 years ago
|
||
Attachment #335544 -
Attachment is obsolete: true
Attachment #335570 -
Flags: superreview?(roc)
Attachment #335570 -
Flags: superreview?(roc)
Assignee | ||
Comment 11•16 years ago
|
||
Attachment #335570 -
Attachment is obsolete: true
Assignee | ||
Comment 12•16 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.
Comment 13•16 years ago
|
||
I can confirm that the patch fixes the bug. My Adium 1.3 picks up links from Firefox just fine.
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•16 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•16 years ago
|
||
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•16 years ago
|
||
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 | ||
Comment 22•16 years ago
|
||
landed on trunk
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Resolution: --- → FIXED
Comment 23•16 years ago
|
||
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•16 years ago
|
||
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).
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 26•16 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•16 years ago
|
||
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 | ||
Comment 28•16 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+
Updated•16 years ago
|
Keywords: fixed1.9.1
Comment 29•16 years ago
|
||
This adds "::" before "CFStringCreateWithCString" per josh's comment.
Attachment #363127 -
Attachment is obsolete: true
Attachment #363130 -
Flags: superreview?
Attachment #363127 -
Flags: superreview?(roc)
Updated•16 years ago
|
Attachment #363130 -
Flags: superreview? → superreview?(roc)
Attachment #363130 -
Flags: superreview?(roc) → superreview+
Assignee | ||
Comment 30•16 years ago
|
||
pushed to mozilla-central http://hg.mozilla.org/mozilla-central/rev/5145eee380f4
Status: REOPENED → RESOLVED
Closed: 16 years ago → 16 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 31•16 years ago
|
||
pushed to mozilla-1.9.1 http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0591df37c782
Keywords: fixed1.9.1
Updated•16 years ago
|
Target Milestone: --- → mozilla1.9.2a1
Comment 32•16 years ago
|
||
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.
Description
•