427448 - Applescript curl & ptit query worked in FF2 fails in FF3

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P2)

Description

[Jonathan Austin]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9b5) Gecko/2008032619 Firefox/3.0b5

AdiumX has a button in the chat window that will paste a link to the currently active browser page. This is accomplished via an applescript which makes a call to FF. In Firefox 2 this applescript call work without any problems but now fails in FF3.

Reproducible: Always

Steps to Reproduce:
1. Open AdiumX, start a conversation with a buddy.
2. Open a link in firefox 3
3. Click the "Insert link to active page" button in AdiumX
Actual Results:  
Applescript returns gibberish to AdiumX

Expected Results:  
Expected to have page title and link in the chat window.

I spoke with the adium developers, they pointed me to the relevant calling applescript:

if ((application processes whose (name is equal to "firefox-bin")) count) is greater than 0 then
     tell application "Firefox"
         if (count of every window) is greater than 0 then
               set the end of labelledcandidates to "Firefox: " & («class pTit» of window 1)
               set the end of candidateURLs to «class curl» of window 1
               set the end of candidates to «class pTit» of window 1
          end if
      end tell
end if

Comment 1

[Luis de la Rosa]

For testing on a development build, you can use the following AppleScript:

tell application "MinefieldDebug"
	set Link to get «class curl» of window 1 as text
	set theTitle to «class pTit» of window 1 as text
	{Link, theTitle}
end tell

It should return these values for the MinefieldDebug start page:
{"http://www.mozilla.org/projects/minefield/", "Minefield Start Page"}

Comment 2

[Luis de la Rosa]

Attachment: Proposed patch that will fix the problems with getting the title via AppleScript

Comment 3

[Luis de la Rosa]

Attachment: Better patch to fix the problems with getting the title via AppleScript

The code is cleaner thanks to better string conversion code from Gavin after he reviewed the first patch.

Comment 4

[Colin Barrett [:cbarrett]]

Comment on attachment 323430 [details] [diff] [review]
Better patch to fix the problems with getting the title via AppleScript

I wonder if getting the widget (and thus the NSWindow*) for the XUL window might be a better idea?

Also, isn't ToNewCString deprecated? I remember some sort of admonition against them, but I'm a bit fuzzy.

It is also mysterious to me why these methods are dealing with old Carbon constructs, but I'm chalking that up to Apple Events sucking.

Comment 5

[Colin Barrett [:cbarrett]]

I think this bug should block 3.0.1. It breaks a number of third party applications (Adium, WebnoteHappy, any sort of Applescript based bookmark utility, etc).

Comment 6

[Colin Barrett [:cbarrett]]

Also, it's a regression from Firefox 2.

Comment 7

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

This is true on trunk too and will probably be fixed there first then ported over to 1.9.

Comment 8

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

And yet none of you could assign the bug to the patch author? ;)

Comment 10

[John C]

How would one go about implementing this patch?  Is it for Adium or FF3?  Or is it simply a proposed solution to be implemented in a future FF release?

Comment 11

[Josh Aas]

Comment on attachment 323430 [details] [diff] [review]
Better patch to fix the problems with getting the title via AppleScript

Won't this leak cTitle?

Comment 12

[Luis de la Rosa]

Attachment: Even better patch to fix the problems with getting the title via AppleScript

This fixes the memory leak of cTitle in the previous patch.

Comment 13

[Josh Aas]

Comment on attachment 326364 [details] [diff] [review]
Even better patch to fix the problems with getting the title via AppleScript

Since this is Mac-only code there is no need to use the nsMemory version of free(), which is generally just there to make writing cross-platform code easier. Just do "free(cTitle)".

Comment 14

[Josh Aas]

My bad, keep using the nsMemory version of free(). I guess that might be useful for allocator work in the future. The right thing to do seems sort of unclear to me after some input from others but nsMemory is safer.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

+  nsXPIDLString title;
+  baseWindow->GetTitle(getter_Copies(title));
+  char *cTitle = ToNewCString(NS_ConvertUTF16toUTF8(title));
+  strncpy(outName, cTitle, maxLen);
+  nsMemory::Free(cTitle);

Seems suboptimal. Why not skip the ToNewCString and just do

nsXPIDLString title;
baseWindow->GetTitle(getter_Copies(title));
strncpy(outName, NS_ConvertUTF16toUTF8(title).get(), maxLen);

Or even better, use strcpy instead of strncpy.

There are more complicated things you can do if you want to avoid the extra
copy (the above copies twice, first a utf16->utf8 copy, then a copy to the
outName buffer).

Let me know if you really need that though.

Comment 16

[Josh Aas]

Comment on attachment 326364 [details] [diff] [review]
Even better patch to fix the problems with getting the title via AppleScript

Please do what Jonas suggested in comment #15 and then this patch should be ready to go.

Comment 17

[Luis de la Rosa]

Attachment: Better, simpler patch to fix the problems with getting the title via AppleScript

Incorporated Jonas's suggestion.

Comment 18

[Josh Aas]

Comment on attachment 327168 [details] [diff] [review]
Better, simpler patch to fix the problems with getting the title via AppleScript

How sure are you that GetTitle won't return nsnull?

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Also, do you really need to use strncpy rather than strcpy?

Comment 20

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #19)
> Also, do you really need to use strncpy rather than strcpy?
> 
Isn't strncpy better to avoid buffer overflows?

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Doh! Right you are, i recalled strcpy taking a length too but that's not the case. Stupid though, strncpy wastes time padding out the end which is totally unnecessary in most cases.

Comment 22

[Luis de la Rosa]

Josh: I didn't find any times when GetTitle returned null in my testing.  If a page doesn't load, it will return "Page Load Error" for the title.  If you load a blank page when you open a browser window, you'll get "Minefield" for the title, which in production will be "Mozilla Firefox."

Comment 23

[Josh Aas]

This code applies to all Mozilla-based products, so in production it won't necessarily say things related Firefox/Minefield. I think if you have no proof that it can't return null then you should null check it.

Comment 24

[Luis de la Rosa]

Attachment: Patch for AppleScript bug with more error checking

This checks that title and cTitle are not nil/NULL.  It also makes sure that outName is zero-terminated as apparently strncpy doesn't do that in the case of when cTitle is the same length as maxLen.

Comment 25

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

Comment on attachment 327846 [details] [diff] [review]
Patch for AppleScript bug with more error checking

+  ThrowErrIfNil(cTitle, paramErr);

cTitle can't be null here, so remove this.

Otherwise good.

Comment 26

[Luis de la Rosa]

Attachment: Patch for AppleScript bug

This is like the last patch but removes the null check for cTitle since roc said that was unnecessary.

Comment 27

[Shawn Wilsher :sdwilsh]

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/2ac85f53e36b

Comment 28

[Josh Aas]

Luis - can you create a patch for Firefox 3.0.x and post it here?

Comment 29

[Josh Aas]

Comment on attachment 329689 [details] [diff] [review]
Patch for AppleScript bug

I think this patch happens to apply cleanly to head cvs and hg, so lets just request approval.

Comment 30

[Jesse Ruderman]

This might not be ready for branch because of bug 448087, "AppleScript curl/pTit: non-ASCII character in title comes through incorrectly".

Comment 31

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

Comment on attachment 329689 [details] [diff] [review]
Patch for AppleScript bug

Please re-request approval after ensuring bug 448087 is fixed and ready for the branch.

Comment 32

[Josh Aas]

Attachment: 1.9.0 branch fix, v1.0

Comment 33

[Mike Connor [:mconnor]]

Comment on attachment 336768 [details] [diff] [review]
1.9.0 branch fix, v1.0

Josh, can you comment as to risk and whether this addresses bug 448087 as well?  We should also have a test plan for QA here.

Comment 34

[Josh Aas]

Comment on attachment 336768 [details] [diff] [review]
1.9.0 branch fix, v1.0

This code is rarely used - only when something uses AppleScript to query for a window title. Adium users are probably the #1 consumers of this capability. The bug is pretty obvious, the fix isolated and safe. I don't think we need any form of testing past patch review here and having someone in QA run through the repro steps in this bug once. Someday we could write an AppleScript test suite but it is just not worth it right now and we're rewriting all of this in Cocoa soon.

Comment 35

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

mconnor: See comment 34.

Comment 36

[Mike Connor [:mconnor]]

Comment on attachment 336768 [details] [diff] [review]
1.9.0 branch fix, v1.0

Ok, let's take it for 3.0.5 then.

Comment 37

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

Comment on attachment 336768 [details] [diff] [review]
1.9.0 branch fix, v1.0

I'm minusing this for now, based on comment 25 of bug 448087. This isn't the right fix and isn't even better if it includes the fix for bug 448087.

Comment 38

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

Note: Please renominate when there is a working fix that's landed and baked on trunk and has also been verified.

An AppleScript testcase in this bug will also be required based on previous and current regressions. Bonus points for an automated one.

(Should I reopen this bug since it's really not fixed?)

Comment 39

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

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081107 Minefield/3.1b2pre

Sure. Tested again with the script in comment 1 and the title is still empty for the latest debug build of Minefield. Only the URL is filled in.

Comment 41

[Elliotte Rusty Harold]

FYI, this still appears non-functional in Firefox 3.0.5. To the extent this prevents an upgrade from Firefox 2, this is getting critical as Firefox 2 will no longer be supported.

Comment 42

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

Luis, can you please give any update from your side? Are you wanna have a look again to fix this issue?

Comment 43

[Luis de la Rosa]

Henrik: The last patch I created applied cleanly - https://bugzilla.mozilla.org/attachment.cgi?id=329689  which I tested and worked to fix bug 448087  as well.  After that my wife had a new baby and I got really busy.  Meanwhile, Josh created another patch - https://bugzilla.mozilla.org/attachment.cgi?id=336768 You can take a look at the trail of what led to that patch I think on https://bugzilla.mozilla.org/show_bug.cgi?id=448087

I don't want to step on Josh's toes, so perhaps he can provide an update?

Comment 44

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

Josh, can you please give a status update? Are you planning to work on that for 1.9.1?

Comment 45

[Josh Aas]

Don't worry about stepping on my toes. I don't have plans to work on any of this at the moment, I just jumped in to supply a patch when Luis was busy. Someone needs to sort through these bugs, figure out what landed when and where, decide on a correct approach, and make sure that lands on trunk and 1.9.1. I'd be happy to do reviews.

Comment 46

[bluloo]

Still broken in 3.0.6

Update?

Thanks

Comment 47

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

This bug won't be fixed on 1.9.0 since it hasn't been fixed on any development branch (like, mozilla-central or 1.9.1). When this bug is RESOLVED FIXED, you can look for it to get fixed on 1.9.0 (for a Firefox 3.0.x release), but since no one is actively working on it, I don't expect that to be soon.

Comment 48

[Luis de la Rosa]

I'm sure this was fixed previously in a patch in bug 448087.  For some reason, subsequent updates broke the patch.  I'll take another look as this is behavior that we want to see in Firefox on Mac OS X.

Comment 49

[Luis de la Rosa]

I submitted a new patch which I verified by manual testing to be working to bug 448087 with mozilla-central.  It is already RESOLVED FIXED, but I hope that Josh or someone can re-open that bug, review it, and re-mark it as RESOLVED FIXED.

Comment 50

[jasper_b]

What's the status on this bug? It's giving me problems in Firefox 3.0.8 and how it interacts with DEVONthink.

Comment 51

[Chris Lawson (gone)]

Please read comment 47.

Comment 52

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

Works fantastic with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3) Gecko/20090305 Firefox/3.1b3 ID:20090305133223 and a recent trunk build. Thanks Luis for pointing to the fix. So we only need the follow-up on bug 448087 to get this bug fixed on 1.9.0 branch?

Comment 53

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

(In reply to comment #52)
> So we only need the follow-up on bug 448087 to get this bug fixed on 1.9.0 branch?

And tests, per comment 38.

Comment 54

[Luis de la Rosa]

Attachment: 1.9.0 branch fix which matches the mozilla-central/mozilla-1.9.1 branch fix

This matches the patch that fixed https://bugzilla.mozilla.org/show_bug.cgi?id=448087 but I have reapplied it to the mozilla-1.9.0 branch in CVS.

To test this out, open up Script Editor in Mac OS X and enter the following as a script and then run it:

tell application "MinefieldDebug"
	set Link to get «class curl» of window 1 as text
	set theTitle to «class pTit» of window 1 as text
	{Link, theTitle}
end tell

Before this patch, mozilla-1.9.0 branch source would return something like:
{"http://www.mozilla.org/projects/minefield/", ""}
Notice the blank title.

After the patch, it would return:
{"http://www.mozilla.org/projects/minefield/", "Minefield Start Page"}
Notice the title is filled in correctly.

Comment 55

[Elliotte Rusty Harold]

Is this supposed to be fixed in Firefox 3.5 beta 4 yet? Because it's not, as I just tested.  If 3.5 isn't yet synced to the trunk, is that likely to happen before release?

Comment 56

[Luis de la Rosa]

This _is_ fixed in Firefox 3.5 beta 4.  I just downloaded and tested.

The script you need to use for the beta and final releases is:
tell application "Firefox"
	set Link to get «class curl» of window 1 as text
	set theTitle to «class pTit» of window 1 as text
	{Link, theTitle}
end tell

Comment 57

[Elliotte Rusty Harold]

I'll test again, but that is what I tried, as well as a script of my own. Perhaps the script picked up a different version of Firefox than what was running? In any case, I'll let you know.

Comment 58

[Elliotte Rusty Harold]

OK. It works on the computer I have with me. I'll have to check on the other one where this failed and see if I can figure out why.

Comment 59

[Elliotte Rusty Harold]

Hmm, weird. It definitely and reporducibly fails on my home MacBook with 10.5.6 and 3.5b4. It succeeds on my work MacBook Pro with 10.5.6. Error:

"Can’t make «class curl» of window 1 of application \"Firefox 3.5\" into type Unicode text."

I will investigate further. Perhaps a plugin conflict?

Comment 60

[Elliotte Rusty Harold]

Nope, disabling all extensions didn't fix it. Maybe let's try renaming the app back to the default?

Comment 61

[Elliotte Rusty Harold]

Bingo. That's it. Renaming the app from Firefox to Firefox 3.5 somehow breaks AppleScript. Note that the script editor notices the revised name and updates the script accordingly. Not sure if this is a bug or not. I can;t help but think that it is, but I'm not sure who it belongs to.

Comment 62

[Elliotte Rusty Harold]

I may have spoken too soon. Now it's failing again, and I haven't even relaunched Firefox.

Comment 63

[Elliotte Rusty Harold]

OK. I think this script breaks it somehow:

tell application "Firefox"
	activate
	set Link to get «class curl» of window 1 as text
	set theTitle to «class pTit» of window 1 as text
	{Link, theTitle}
end tell


After you run this (speciifcally the activate call) further calls to the regular script

tell application "Firefox"
	set Link to get «class curl» of window 1 as text
	set theTitle to «class pTit» of window 1 as text
	{Link, theTitle}
end tell

(no activate) also break.

Comment 64

[Luis de la Rosa]

The applications that broke due to this bug (Adium, WebnoteHappy, others) do not need to call "activate" to get the information they need (title and URL) from Firefox.

I filed a new bug for the "activate" problem you found: https://bugzilla.mozilla.org/show_bug.cgi?id=490762 "activate breaks AppleScript support".

I think we should proceed with this fix and investigate the "activate" problem separately.

Comment 65

[Daniel A. Shockley]

For people who are having problems with the new app, try it with parentheses in a useful place (may or may not help):
tell application "Firefox"
	--activate -- this line commented out, since should be unnecessary
	set Link to (get «class curl» of window 1) as text  
	  -- not sure why the above line uses "get" and next doesn't, but left in
	set theTitle to («class pTit» of window 1) as text
	{Link, theTitle}
end tell

It worked, with and without parentheses, for me.

Comment 66

[Daniel Veditz [:dveditz]]

Comment on attachment 372406 [details] [diff] [review]
1.9.0 branch fix which matches the mozilla-central/mozilla-1.9.1 branch fix

Approved for 1.9.0.12, a=dveditz for release-drivers

Comment 67

[Josh Aas]

committed to 1.9.0

Checking in xpfe/bootstrap/appleevents/nsWindowUtils.cpp;
/cvsroot/mozilla/xpfe/bootstrap/appleevents/nsWindowUtils.cpp,v  <--  nsWindowUtils.cpp
new revision: 1.24; previous revision: 1.23
done
Checking in xpfe/bootstrap/appleevents/nsWindowUtils.h;
/cvsroot/mozilla/xpfe/bootstrap/appleevents/nsWindowUtils.h,v  <--  nsWindowUtils.h
new revision: 1.8; previous revision: 1.7
done