Applescript curl & ptit query worked in FF2 fails in FF3

RESOLVED FIXED in mozilla1.9.2a1

Status

()

Core
Widget: Cocoa
P2
major
RESOLVED FIXED
9 years ago
7 years ago

People

(Reporter: Jonathan Austin, Assigned: Luis de la Rosa)

Tracking

({fixed1.9.0.12, fixed1.9.1, regression})

Trunk
mozilla1.9.2a1
All
Mac OS X
fixed1.9.0.12, fixed1.9.1, regression
Points:
---
Bug Flags:
blocking1.9.1 -
wanted1.9.1 +
blocking1.9.0.1 -
wanted1.9.0.x +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch includes fix for 448087])

Attachments

(2 attachments, 6 obsolete attachments)

(Reporter)

Description

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

Comment 1

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

Comment 2

9 years ago
Created attachment 323086 [details] [diff] [review]
Proposed patch that will fix the problems with getting the title via AppleScript
Attachment #323086 - Flags: review?(joshmoz)
(Assignee)

Comment 3

9 years ago
Created attachment 323430 [details] [diff] [review]
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.
Attachment #323086 - Attachment is obsolete: true
Attachment #323430 - Flags: review?(joshmoz)
Attachment #323086 - Flags: review?(joshmoz)
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.
Version: unspecified → 3.0 Branch
Status: UNCONFIRMED → NEW
Ever confirmed: true
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).
Flags: blocking1.9.0.1?
Also, it's a regression from Firefox 2.
This is true on trunk too and will probably be fixed there first then ported over to 1.9.
Severity: normal → major
Flags: wanted1.9.0.x?
Keywords: regression
Version: 3.0 Branch → Trunk
And yet none of you could assign the bug to the patch author? ;)
Assignee: nobody → bugzilla

Updated

9 years ago
Flags: blocking-firefox3.1?

Updated

9 years ago
Component: OS Integration → Widget: Cocoa
Flags: blocking-firefox3.1?
Product: Firefox → Core

Updated

9 years ago
Duplicate of this bug: 438624
QA Contact: os.integration → cocoa

Comment 10

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

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

Won't this leak cTitle?
(Assignee)

Comment 12

9 years ago
Created attachment 326364 [details] [diff] [review]
Even better patch to fix the problems with getting the title via AppleScript

This fixes the memory leak of cTitle in the previous patch.
Attachment #323430 - Attachment is obsolete: true
Attachment #326364 - Flags: review?(joshmoz)
Attachment #323430 - Flags: review?(joshmoz)

Comment 13

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

9 years ago
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.
+  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.
Flags: wanted1.9.0.x?
Flags: wanted1.9.0.x+
Flags: blocking1.9.0.1?
Flags: blocking1.9.0.1-

Comment 16

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

Comment 17

9 years ago
Created attachment 327168 [details] [diff] [review]
Better, simpler patch to fix the problems with getting the title via AppleScript

Incorporated Jonas's suggestion.
Attachment #326364 - Attachment is obsolete: true
Attachment #327168 - Flags: review?(joshmoz)
Attachment #326364 - Flags: review?(joshmoz)

Comment 18

9 years ago
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?
Also, do you really need to use strncpy rather than strcpy?
(In reply to comment #19)
> Also, do you really need to use strncpy rather than strcpy?
> 
Isn't strncpy better to avoid buffer overflows?
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.
(Assignee)

Comment 22

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

9 years ago
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.

Updated

9 years ago
Flags: wanted1.9.1+
Priority: -- → P2
(Assignee)

Comment 24

9 years ago
Created attachment 327846 [details] [diff] [review]
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.
Attachment #327168 - Attachment is obsolete: true
Attachment #327846 - Flags: review?(joshmoz)
Attachment #327168 - Flags: review?(joshmoz)

Updated

9 years ago
Attachment #327846 - Flags: superreview?(roc)
Attachment #327846 - Flags: review?(joshmoz)
Attachment #327846 - Flags: review+
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.
Attachment #327846 - Flags: superreview?(roc) → superreview+

Updated

9 years ago
Whiteboard: [checkin needed]
(Assignee)

Comment 26

9 years ago
Created attachment 329689 [details] [diff] [review]
Patch for AppleScript bug

This is like the last patch but removes the null check for cTitle since roc said that was unnecessary.
Attachment #327846 - Attachment is obsolete: true
Keywords: checkin-needed
Whiteboard: [checkin needed]
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/2ac85f53e36b
Status: NEW → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1

Comment 28

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

Comment 29

9 years ago
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.
Attachment #329689 - Flags: approval1.9.0.2?
Whiteboard: [needs baking]

Updated

9 years ago
Depends on: 448087

Comment 30

9 years ago
This might not be ready for branch because of bug 448087, "AppleScript curl/pTit: non-ASCII character in title comes through incorrectly".
Whiteboard: [needs baking] → [needs baking, regression bug 448087?]
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.
Attachment #329689 - Flags: approval1.9.0.2?
Hardware: Macintosh → All

Comment 32

9 years ago
Created attachment 336768 [details] [diff] [review]
1.9.0 branch fix, v1.0
Attachment #336768 - Flags: superreview?(roc)
Attachment #336768 - Flags: superreview?(roc) → superreview+

Updated

9 years ago
Attachment #336768 - Flags: approval1.9.0.3?
Whiteboard: [needs baking, regression bug 448087?] → [patch includes fix for 448087]

Updated

9 years ago
Attachment #336768 - Flags: approval1.9.0.4?
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

9 years ago
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.
Attachment #336768 - Flags: approval1.9.0.4?
Attachment #336768 - Flags: approval1.9.0.5?
Attachment #336768 - Flags: approval1.9.0.4?
Attachment #336768 - Flags: approval1.9.0.4-
mconnor: See comment 34.
Comment on attachment 336768 [details] [diff] [review]
1.9.0 branch fix, v1.0

Ok, let's take it for 3.0.5 then.
Attachment #336768 - Flags: approval1.9.0.5? → approval1.9.0.5+
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.
Attachment #336768 - Flags: approval1.9.0.5+ → approval1.9.0.5-
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?)
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Duplicate of this bug: 464701
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.
Luis, can you please give any update from your side? Are you wanna have a look again to fix this issue?
Whiteboard: [patch includes fix for 448087] → [status update needed][patch includes fix for 448087]
Target Milestone: mozilla1.9.1a1 → ---
(Assignee)

Comment 43

9 years ago
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?
Josh, can you please give a status update? Are you planning to work on that for 1.9.1?
Status: REOPENED → ASSIGNED
Flags: blocking1.9.1?
Flags: blocking1.9.1? → blocking1.9.1-

Comment 45

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

8 years ago
Still broken in 3.0.6

Update?

Thanks
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.
(Assignee)

Comment 48

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

Comment 49

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

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

8 years ago
Please read comment 47.
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?
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
Keywords: fixed1.9.1
(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.
(Assignee)

Comment 54

8 years ago
Created attachment 372406 [details] [diff] [review]
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.
Attachment #329689 - Attachment is obsolete: true
Attachment #372406 - Flags: review?(joshmoz)

Updated

8 years ago
Attachment #372406 - Flags: review?(joshmoz) → review+
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?
(Assignee)

Comment 56

8 years ago
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
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.

Updated

8 years ago
Attachment #372406 - Flags: superreview?(roc)
Attachment #372406 - Flags: superreview?(roc) → superreview+
(Assignee)

Updated

8 years ago
Attachment #372406 - Flags: approval1.9.0.11?
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.
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?
Nope, disabling all extensions didn't fix it. Maybe let's try renaming the app back to the default?
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.
I may have spoken too soon. Now it's failing again, and I haven't even relaunched Firefox.
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.
(Assignee)

Comment 64

8 years ago
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.
Depends on: 490762
No longer depends on: 490762
Depends on: 490762
(Assignee)

Updated

8 years ago
No longer depends on: 490762

Comment 65

8 years ago
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.

Updated

8 years ago
Depends on: 490762
Attachment #372406 - Flags: approval1.9.0.11? → approval1.9.0.12?
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
Attachment #372406 - Flags: approval1.9.0.12? → approval1.9.0.12+
No longer depends on: 490762
Keywords: checkin-needed
Whiteboard: [status update needed][patch includes fix for 448087] → [patch includes fix for 448087][needs 1.9.0 landing = josh?]

Comment 67

8 years ago
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
Keywords: fixed1.9.0
Whiteboard: [patch includes fix for 448087][needs 1.9.0 landing = josh?] → [patch includes fix for 448087]
Keywords: fixed1.9.0 → fixed1.9.0.12

Updated

8 years ago
Keywords: checkin-needed

Updated

7 years ago
Duplicate of this bug: 561141
You need to log in before you can comment on or make changes to this bug.