377392 - "copy link location" fails to copy the letter H and the number 0 where present

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[joe vella]

User-Agent:       Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a2) Gecko/20070215 Minefield/3.0a2
Build Identifier: Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9a2) Gecko/20070215 Minefield/3.0a2

using "copy link location"  omits the letter h and the number 0 so that the pasted link looks like this: ttp:/ow_do_you_do.tml   This seems to happen after the application has been in use for some time.  Once it begins, it is repeatable every time.  Exiting firefox and reloading it will restore proper behaviour.

Reproducible: Always

Steps to Reproduce:
1. right click on a link
2. pick 'copy link location'
3. paste the result somewhere - such as the 'actual results' box below..
Actual Results:  
ttps://bugzilla.mozilla.org/sow_bug.cgi?id=299482

Expected Results:  
https://bugzilla.mozilla.org/show_bug.cgi?id=299482

Although I'm using minefield at the moment - I have the identical issue with Firefox 2.0.03

Comment 1

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

This works for me on Mac OS X, Windows, and Linux. Maybe it's OS/2 specific?

Comment 2

[Peter Weilbacher]

Yes, I think it is OS/2 specific. I have seen something like this in the past, too, although never on branch (FF 2.0.0.x) and on trunk we are still broken, so I cannot currently confirm the bug as such.  If the fault is still there, it's probably somewhere in widget...

(I'm also a bit confused by joe's user agent string, the last usable FF nightly that I see is 20070206.)

Comment 3

[Felix Miata]

Maybe this is related. In 1.8.1.2 OS/2 nightly SM when I copy a link location from the context menu of the history window and paste it into an FC/2 edit session, the actual URL has an 0Dh appended to it. If I paste into an empty e.exe session and save, the file ends with 0Dh, 0Ah (saving an empty session results in a 0 byte file). If I paste into an empty epm.exe session and save, the file ends with 0Dh, 0Dh, 0Ah (saving an empty session results in a 2 byte file ending in 0Dh 0Ah). So far I haven't noticed any characters omitted from the paste.

Comment 4

[Peter Weilbacher]

I think the line endings are more of a speciality of the editors than the text copy properties of Mozilla code. I just tried e.exe ae.exe and epm.exe and got different results in all of them (only AE doesn't append anything!).

I also used SeaMonkey trunk -- where this had happened for me quite often some while ago -- extensively over the last weeks and haven't seen the missing-chars problem any more. Neither with missing 'h', '0', or any other character...

Comment 5

[Peter Weilbacher]

Joe, have you seen this again? Perhaps you have tried the more recent nightly builds from <http://sourceforge.net/project/showfiles.php?group_id=183811&package_id=213528>? I haven't seen this at all any more, so I would tend to resolve this as WFM.

Comment 6

[joe vella]

I Hadn't seen it for a while, then got it again recently 0- I'll try the new build.  thanks.

Comment 7

[Peter Weilbacher]

Have seen it again myself in the meantime, but neither with 'h' nor with '0'. instead 'l' and 'i' were missing. And when trying a debug build to determine what the problem was, I couldn't reproduce it any more. In any case, it doesn't happen often --> severity=minor.

Comment 8

[Martin Sch]

It happens quite often here with FF Minefield/3.6a1pre on os/2.

Comment 9

[Peter Weilbacher]

If you can reliably reproduce this (even after restart of FF and system restart?!), I can create a debug DLL with some additional output. But then I need to know the _exact_ build you are running (please tell me the full BuildID from Help -> About).

Comment 10

[Peter Weilbacher]

Looking at the code in nsClipboard.cpp I wonder why we never added the OBJ_ANY flag to the DosAllocSharedMem() calls in that file while we did so in all the other files when we added high-memory support. Does the PM clipboard not work with himem?

Comment 11

[Dave Yeo]

Replying to Comment #9
I can reproduce this after restarting Firefox, if you can give me some directions to produce the debug DLL, I can look at it.

Comment 12

[Rich Walsh]

So far, I've found that the unicode text is delivered to the OS/2 clipboard code without the missing characters (yesterday I was losing 'p' & '9' - today, it's 'p' & '?').  We'll have to look upstream to see where they're disappearing.

As to hi-mem for the clipboard:  don't.  The data has to be in the lower 512 to be accessible to 16-bit code, most notably, VIO windows.  In any case, one 64k segment just isn't that big a deal.

Comment 13

[Peter Weilbacher]

I don't know how to explain this but since July it has not happened for me again. And I haven't seen any complaints on the newsgroup since then. This seems to come up again every few months, but then vanish again soon.

What should we do with this?

Comment 14

[Martin Sch]

In 3.5 it was impossible to use the "copy link location" menu because a lot of characters where missing *every* time.

Now with 3.6b1 all "p" are missing.

Comment 15

[Dave Yeo]

(in reply to comment #13)
I just tested on a trunk Firefox pulled a couple of days ago. d and p at least were missing.
Don't have the problem with trunk built Seamonkey though.

Comment 16

[Cliff Scott]

Just to add to the list, I am seeing "p" , but NOT "d" missing in 3.6b1.

Comment 17

[Martin Sch]

Is there any way how we could debug this?
Is there a tool showing the contents of the clipboard?

Any idea why it depends on the letter? It would be normal if letters randomly disappear, but it is a bit strange that only specific letters disappear.

Comment 18

[Dave Yeo]

I notice this bug has spread to Seamonkey, Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.3a1pre) Gecko/20091202 SeaMonkey/2.1a1pre, with periods not being copied to the clipboard from an URL.
Interestingly copying an email address works fine.

Comment 19

[Cliff Scott]

In my case the ONLY place I see it happening is when I use the context menu item "Copy Link Location". The other copy functions seem to work fine. Why it would choose that one is beyond me.

Comment 20

[Dave Yeo]

(In reply to comment #19)
I meant to add that this is from the right click context menu, "Copy Link Location", and "Copy Email Address" which I'd assume use similar code to access the clipboard.

Comment 21

[Mark Dodel]

Been using Mozilla/5.0 (OS/2; U; Warp 4.5; en-US; rv:1.9.2.3) Gecko/20100403 Firefox/3.6.3 for several days now. Seems to be pretty stable.   
However whenever I select "Copy Link location" to copy a URL, the 
resulting copied link is missing some characters in the URL.

"tt://ownoa.smeey.info/ostgresq-..3-os2-200006a.zi"

which should be

"http://download.smedley.info/postgresql-8.4.3-os2-20100406a.zip"

Steve Wendt pointed me to this bug.  What is weird is that this occurs on both my T42p and my T61 and it is the exact same characters that are missing, so it is not random.  This is the first I am seeing this problem.  Just to let people know it is still occurring.  I don't see any issue with the "Copy Email Address" function.

Comment 22

[Cliff Scott]

Same thing here on FF3.6.3. FF 3.5.9 is ok. Both on eCS 2.0RC7a, using the same profile and plugins. It seems to be worse than it was back in FF 2.x.x.x, i.e. losing more letters (H,h,D,d,p,x,S,V 1).

From 3.6.3

tt://www.wunergroun.com/raar/miecomosite.as?region=c3&size=2&I=9

From 3.5.9

http://www.wunderground.com/radar/mixedcomposite.asp?region=c3&size=2x&ID=SHV19

Comment 23

[Andy Willis (abwillis)]

(In reply to comment #10)
> Looking at the code in nsClipboard.cpp I wonder why we never added the OBJ_ANY
> flag to the DosAllocSharedMem() calls in that file while we did so in all the
> other files when we added high-memory support. Does the PM clipboard not work
> with himem?

When first looking into highmem support I had put OBJ_ANY into the clipboard function but found as Rich already pointed out that it didn't play well with VIO.  I could paste into most other applications and but not a command prompt.  I found that I could paste into some other application and then I could paste to a command prompt but not directly into a command prompt.  Deciding that was not optimal I removed it :-)

Comment 25

[Cliff Scott]

It's been a problem since FF 3.5.xx with the exception of 3.5.9 for me. 3.6.x loses the "t" and now 4.0b7pre loses "a", "p" and maybe others. I know this has been going on for a long time with no resolution. This bug is marked as "minor", and in some way it is, but I find it frustrating in V4.0b because a "mouse over" a link doesn't show the URL of the link in the bottom bar of the window any more. Being able to copy the link location would be at least somewhat helpful in this situation.

Is there any hope of a resolution?

Comment 26

[Felix Miata]

If the same problem happens or not if copying instead out of history maybe there's a clue to be found there.

Comment 27

[Felix Miata]

I switched to 2.1b1 from Peter's 1.1.18 a week ago. I frequently loose numeral "0", never anything else that I've noticed. Is anyone besides me who sees this using something other than WorkPlace Sans for Gecko 1.9.x system fonts? I'm using Trebuchet MS @10pt with 120 DPI on 1400x1050 set in via CONFIG.SYS as follows:

SET SDDFONTSIZE=medium
REM SET SDDFONTDPI=120
SET SDDICONS=small

Comment 28

[Peter Weilbacher]

I am pretty sure that the characters getting lost has nothing to do with displaying them (fonts, SDD settings, etc.). The one time I debugged it, they already arrived lost in the OS/2 code in nsClipboard.cpp, IIRC.

No idea why I never followed up on comment 11. I think one should start by adding lots of printf()s in nsClipboard::SetClipboardData. Just dumping the bytes of pMozData to the screen could be a first step, like (untested)
   printf("%d bytes in clipboard data\n", NumOfBytes);
   for (int i = 0; i < NumOfBytes; i++) {
     printf("%d: %c %x\n", i+1, (char)pMozData[i], pMozData[i]);
   }

If that is already missing chars, add similar stuff in nsTransferable::GetTransferData or even further up.

Comment 29

[Rich Walsh]

(In reply to comment #28)
> The one time I debugged it[...]

I just finished a debugging session for this problem then read these new comments.  Problem is, I can't get my debug version to mess up.  OTOH, my non-debug build consistently drops 'h' and 'e'.  We're working on it...

Comment 30

[Rich Walsh]

Attachment: fixes "Copy Link Location" bug

This should fix the problem.

Comment 31

[Dave Yeo]

Should not all the control characters be filtered out? Seems none should ever be in a link.

Comment 32

[Martin Sch]

Stripping also the \0 fixed the problem?

Comment 33

[Rich Walsh]

Attachment: fixes "Copy Link Location" bug - v2

This update makes the nature of the fix more obvious (i.e. the string wasn't null-terminated).

Comment 34

[Martin Sch]

This bug was os/2 only? Why?

Comment 35

[Walter Meinl]

(In reply to comment #33)
> Created attachment 490415 [details] [diff] [review]
> fixes "Copy Link Location" bug - v2
> 
> This update makes the nature of the fix more obvious (i.e. the string wasn't
> null-terminated).

The solution provided by your patch was also suggested in bug77522#c20. Unfortunately the lines making us trouble were checked in in bug77522

Comment 36

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 490415 [details] [diff] [review]
fixes "Copy Link Location" bug - v2

r=me.  Thanks for catching this!

I think we should take this for 2.0.  The risk is minimal, and randomly copying the wrong text for links (and also randomly getting incorrect link locations in a11y code, which _would_ be hit by Firefox) seems bad.

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 490415 [details] [diff] [review]
fixes "Copy Link Location" bug - v2

a2.0=dbaron

Comment 38

[Martin Sch]

Can someone explain why
static char strippedChars[] = {'\t','\r','\n'};
caused the bug and 
static const char strippedChars[] = "\t\r\n";
does not?

And why is it os/2 specific?

Comment 39

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Because the former is missing the null-terminator, and therefore the string includes whatever random data follows that memory, up to the next null byte.

Comment 40

[Rich Walsh]

(In reply to comment #38)

> And why is it os/2 specific?

It isn't.  It just happened to manifest itself on OS/2 where the gcc compiler chose to put other random data (including characters found in URLs) next to these 3 bytes.  On another platform, gcc may have put null characters after them (for whatever reason), thus hiding the bug.  Simply changing the data used by the source file could have caused the problem to appear on other platforms - or to disappear on OS/2.

Comment 41

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/30d1de20ece2

Comment 42

[Martin Sch]

Crazy. Thanks!

Comment 43

[Walter Meinl]

Comment on attachment 490415 [details] [diff] [review]
fixes "Copy Link Location" bug - v2

This bug was introduced long ago by a faulty patch. Although it was most obvious on OS/2 it may have been spurious on other platforms. It should be considered to take it for all branches that are still developed. For the risk assessment see bz's comment https://bugzilla.mozilla.org/show_bug.cgi?id=377392#c36

Comment 44

[Martin Sch]

Talking about risk: can this bug be used to cause a buffer overrun in firefox by constructing a malicious link?

Comment 45

[Boris Zbarsky [:bzbarsky]]

No.

Comment 46

[Boris Zbarsky [:bzbarsky]]

More precisely, this bug could cause Firefox to read memory it doesn't own in the unlikely event that there are no null bytes between the strippedChars address and the next memory not owned by Firefox.  But this would be dependent on the binary layout only, and would happen on any link, independent of the link text.

Comment 47

[Daniel Veditz [:dveditz]]

Comment on attachment 490415 [details] [diff] [review]
fixes "Copy Link Location" bug - v2

Approved for 1.9.2.15 and 1.9.1.18, a=dveditz for release-drivers

Comment 48

[Walter Meinl]

Attachment: cvs'ified version of the patch for 1.9.0.next

Comment 49

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

Landed attachment 514618 [details] [diff] [review] on CVS trunk for 1.9.0.next:

Checking in layout/base/nsPresShell.cpp;
/cvsroot/mozilla/layout/base/nsPresShell.cpp,v  <--  nsPresShell.cpp
new revision: 3.1129; previous revision: 3.1128
done

Comment 50

[(no longer active)]

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/151e611fc58b
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/d9a42eb3a7a5

Comment 51

[Daniel Veditz [:dveditz]]

The "3.6.15" we're releasing today does not fix this bug, the release containing this bug fix has been renamed to "3.6.16" and the bugzilla flags will be updated to reflect that soon. Today's release is a re-release of 3.6.14 plus a fix for a bug that prevented many Java applets from starting up.