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

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Peter Juracic]

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'

Comment 1

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

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.

Comment 2

[[e-mail]]

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.

Comment 3

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

"www.google.com" succeeded.
www.google.com,,-1,0,,,, also succeeded.

Comment 4

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(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.

Comment 5

[[e-mail]]

Alternatively deleting lines 143 and 146 should work.

Comment 6

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

Attachment: DDE Dump

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

Comment 7

[[e-mail]]

Attachment: Proposed patch

Comment 8

[[e-mail]]

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.

Comment 9

[Benjamin Smedberg]

Comment on attachment 160075 [details] [diff] [review]
Proposed patch

Stealing more reviews ;)

Comment 10

[Ere Maijala (slow)]

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?

Comment 11

[[e-mail]]

Attachment: Better patch

Comment 12

[[e-mail]]

Comment on attachment 160085 [details] [diff] [review]
Better patch

ere, thanks for catching that. Also, here's -u7 for you.

Comment 13

[rbs]

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
[...]
}

Comment 14

[[e-mail]]

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

Comment 15

[Darin Fisher]

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.

Comment 16

[[e-mail]]

Fix checked in.

Comment 17

[Bernard Alleysson]

The patch looks easy and DDE requests are important to some people. Could it be
considered for branches ?

Comment 18

[Bernard Alleysson]

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

Comment 19

[Asa Dotzler [:asa]]

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.

Comment 20

[Johnny Stenback (:jst)]

Attachment: Fix for firefox as well.

Comment 21

[Johnny Stenback (:jst)]

*** Bug 266896 has been marked as a duplicate of this bug. ***

Comment 22

[Johnny Stenback (:jst)]

Fixed on the aviary branch, a=asa.

Comment 23

[Bernard Alleysson]

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

Comment 24

[[e-mail]]

Comment on attachment 160085 [details] [diff] [review]
Better patch

As per comment 23 requesting 1.7/aviary sync.

Comment 25

[Mike Kaply [:mkaply]]

Comment on attachment 160085 [details] [diff] [review]
Better patch

a=mkaply for 1.7.5