DDE: Request WWW_OpenURL send to Mozilla web page address without last character of address.

RESOLVED FIXED

Status

RESOLVED FIXED
14 years ago
14 years ago

People

(Reporter: p.juracic, Assigned: neil)

Tracking

({fixed-aviary1.0, fixed1.7.5})

Trunk
x86
Windows 2000
fixed-aviary1.0, fixed1.7.5

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

14 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040803
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.7.2) Gecko/20040803

When you want open web page for example 'www.google.com' by DDE Request call,
Mozilla open page 'www.google.co'.
Missing last character in web page address.

Try thi small VisualBasic macro for Excel:
Sub DDEcallMozilla()
    Dim tmp
    Dim channelNumber
    
        channelNumber = Application.DDEInitiate("Mozilla", "WWW_OpenURL")
        tmp = Application.DDERequest(channelNumber, "www.google.com")
        Application.DDETerminate channelNumber
        
End Sub

Sorry for M$ macro, If you want I can send you VI code in NationaInstrument
LabVIEW :-))

Reproducible: Always
Steps to Reproduce:
1. Close all Mozilla browser [and Download Manager]
2. Open first Mozilla browser 
3. Now try DDE Reguest to Server(application) 'Mozilla' with Topic 'WWW_OpenURL'
and with Item 'www.google.com'. Or run there Excel Macro


Actual Results:  
Mozilla open new browser window and start opening page 'www.google.co'
Missing last character in web page address.

Expected Results:  
Expect opening web address 'www.google.com'
Using that code, I did reproduce the behavior.  However, the message format used
when I do start->run "http://www.google.com" (which works properly) would be:
"%1",,-1,0,,,,

Using the following code, google loads properly:

Sub DDEcallMozilla()
    Dim tmp
    Dim channelNumber
    
        channelNumber = Application.DDEInitiate("Mozilla", "WWW_OpenURL")
        tmp = Application.DDERequest(channelNumber, "" & Chr$(34) &
"www.google.com" & Chr$(34) & ",,-1,0,,,,")
        Application.DDETerminate channelNumber
        
End Sub

I don't know if this is a Mozilla bug, or if you aren't sending the right message.
(Assignee)

Comment 2

14 years ago
I understand www.google.com fails, and "www.google.com",,-1,0,,,, succeeds, but
how about "www.google.com" or www.google.com,,-1,0,,,, ? This well tell us if
it's the lack of the quotation marks or the commas that's confusing Mozilla.

Also, if you have a tree, try compiling nsNativeAppSupportWin.cpp with
-DMOZ_DEBUG_DDE to see where the URL is getting mangled.
"www.google.com" succeeded.
www.google.com,,-1,0,,,, also succeeded.
(In reply to comment #2)
> Also, if you have a tree, try compiling nsNativeAppSupportWin.cpp with
> -DMOZ_DEBUG_DDE to see where the URL is getting mangled.

I don't know how to do that.
(Assignee)

Comment 5

14 years ago
Alternatively deleting lines 143 and 146 should work.
Created attachment 160064 [details]
DDE Dump

This was the result of using the original code (www.google.com).
(Assignee)

Comment 7

14 years ago
Created attachment 160075 [details] [diff] [review]
Proposed patch
Assignee: general → neil.parkwaycc.co.uk
Status: UNCONFIRMED → ASSIGNED
(Assignee)

Comment 8

14 years ago
Comment on attachment 160075 [details] [diff] [review]
Proposed patch

DdeQueryString returns the string length, which is one less than the buffer
length it requires, to allow for the terminating null character.
Attachment #160075 - Flags: superreview?(rbs)
Attachment #160075 - Flags: review?(emaijala)

Comment 9

14 years ago
Comment on attachment 160075 [details] [diff] [review]
Proposed patch

Stealing more reviews ;)
Attachment #160075 - Flags: review?(emaijala) → review+

Comment 10

14 years ago
Comment on attachment 160075 [details] [diff] [review]
Proposed patch

I'm no string expert, but I believe the parameter to SetLength shouldn't
include the space for terminating null. Would temp.Length()+1 in the
DdeQueryString be more appropriate?
Attachment #160075 - Flags: review+ → review-
(Assignee)

Comment 11

14 years ago
Created attachment 160085 [details] [diff] [review]
Better patch
Attachment #160075 - Attachment is obsolete: true
(Assignee)

Comment 12

14 years ago
Comment on attachment 160085 [details] [diff] [review]
Better patch

ere, thanks for catching that. Also, here's -u7 for you.
Attachment #160085 - Flags: superreview?(rbs)
Attachment #160085 - Flags: review?(emaijala)

Updated

14 years ago
Attachment #160085 - Flags: review?(emaijala) → review+

Comment 13

14 years ago
Comment on attachment 160085 [details] [diff] [review]
Better patch

sr=rbs, include temp.SetLength( argLen+1 ); as well...

I see some conflicting comments in the tree (see below). No point being greedy
on 1 char when crossing a M$ boundary.

void
nsTSubstring_CharT::SetLength( size_type length )
  {
    SetCapacity(length);
    mLength = length;
  }
----
void
nsTSubstring_CharT::SetCapacity( size_type capacity )
  {
    // capacity does not include room for the terminating null char
[...]
}
Attachment #160085 - Flags: superreview?(rbs) → superreview+

Updated

14 years ago
Attachment #160075 - Flags: superreview?(rbs)
(Assignee)

Comment 14

14 years ago
Darin, please can settle this argument once and for all?

Comment 15

14 years ago
SetLength(N) ensures a buffer size that consists of N storage units plus one
extra storage unit for the nul-terminator.

It so happens that SetCapacity(M) is equivalent to SetLength(M).  In the string
code, the capacity of a string does not include the nul-terminator.  This may
seem counter-intuitive to those familiar with other string libraries.  Please
note that SetCapacity is not guaranteed to actually do anything.

The current patch is fine.  There is no need to increase the size of the string
buffer.
(Assignee)

Comment 16

14 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 17

14 years ago
The patch looks easy and DDE requests are important to some people. Could it be
considered for branches ?
Flags: blocking1.7.x?
Flags: blocking-aviary1.0?

Comment 18

14 years ago
bug 266896 is the same for Firefox. Please check it out and resolve as duplicate
of this one it is so.

Comment 19

14 years ago
Comment on attachment 160085 [details] [diff] [review]
Better patch

If this patch is working for Firefox on the branch then we want it. Get it in
for tomorrow morning's builds.
Attachment #160085 - Flags: approval-aviary+
Created attachment 164820 [details] [diff] [review]
Fix for firefox as well.
*** Bug 266896 has been marked as a duplicate of this bug. ***
Fixed on the aviary branch, a=asa.
Keywords: fixed-aviary1.0

Updated

14 years ago
Flags: blocking-aviary1.0?
Product: Browser → Seamonkey

Comment 23

14 years ago
Fixed on aviary, I think it should go on 1.7 branch too (same behaviour for
Mozilla 1.7 and Firefox)
(Assignee)

Comment 24

14 years ago
Comment on attachment 160085 [details] [diff] [review]
Better patch

As per comment 23 requesting 1.7/aviary sync.
Attachment #160085 - Flags: approval1.7.5?
Comment on attachment 160085 [details] [diff] [review]
Better patch

a=mkaply for 1.7.5
Attachment #160085 - Flags: approval1.7.5? → approval1.7.5+
(Assignee)

Updated

14 years ago
Keywords: fixed1.7.5

Updated

14 years ago
Flags: blocking1.7.5?
You need to log in before you can comment on or make changes to this bug.