793547 - Defect - Clicking links in Chatzilla/Skype (and others?) strips off the hash at the end of the URL

Status: VERIFIED FIXED

(Tracking Graveyard :: Metro Operations, defect, P1)

Description

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If there's a link posted in some channel that includes a hash at the end, the hash is not passed along to Firefox.  (So "https://bugzilla.mozilla.org/show_bug.cgi?id=793052#c2" will become "https://bugzilla.mozilla.org/show_bug.cgi?id=793052" when it is opened.)

Interestingly, if I right-click the link and choose to copy the link location, the copied link DOES include the hash.

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm using Chatzilla on XULRunner, if that makes a difference compared to the extension version.

Comment 2

[Tony Mechelynck [:tonymec]]

Mozilla/5.0 (X11; Linux x86_64; rv:18.0) Gecko/18.0 Firefox/18.0 SeaMonkey/2.15a1 ID:20120921110141 c-c:f181aef1ee3f m-c:3c68fdd4f77a

ChatZilla 0.9.89

I do not see that bug in cZ running as an extension in SeaMonkey; but in my case the link is passed internally to the browser of the same app (i.e. the SeaMonkey browser, system default or not), while I suppose that "cZ in XR" would have to pass the link to "whatever is the default browser on this OS".

Wes: which Firefox version, on which OS? You may find it by pasting the line(s) labeled "User Agent" and/or "Build Identifier" at the bottom of the page which opens when you type "about:" (without the quotes, and followed by Enter) in the URL bar. It should be similar but not identical to what I pasted at the top of this comment.

Comment 3

[Tony Mechelynck [:tonymec]]

P.S. Oh, and which version of ChatZilla? *That* is found at the top of the /client tab in cZ, or in the second column on the ChatZilla line of the "Help → Troubleshooting Information" page.

Comment 4

[Tony Mechelynck [:tonymec]]

P.P.S. Ah, I don't know if XR has "Troubleshooting Information". Well, even cZ in XR has a client tab — just type /client to go to it.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Build identifier: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:18.0) Gecko/18.0 Firefox/18.0
ChatZilla 0.9.88-rdmsoft [XULRunner 1.9.0.17/2009122204]

This isn't a relatively recent issue for me, it's been happening at least since Nightly was in the 14.0a1 cycle, maybe earlier. It's been bugging me for a while, just never enough to actually get me to file the bug.

Might be a Windows 8 issue? I no longer have another OS to test this on, as I just deleted my Win7 partition yesterday.

Comment 6

[Tony Mechelynck [:tonymec]]

What happens if you click a link with a fragment in an email (assuming that you read mail in a mailer, not by webmail in your browser)? Does the fragment also get sripped when your OS passes it to Firefox? Here is such a link, it should be in the bugmail Bugzilla sent you for this comment:
http://tools.ietf.org/html/rfc3986#section-3.5

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Doesn't work from Skype to Firefox, either.

Huh.

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

But it does work from the Win8 Metro "Mail" app to the Metro version of Firefox...

Comment 9

[Tony Mechelynck [:tonymec]]

(In reply to Wes Kocher (:KWierso) from comment #7)
> Doesn't work from Skype to Firefox, either.
> 
> Huh.

Hm. I suspect a problem with how Windows passes URLs to the default browser. However Microsoft is notoriously unhelpful toward customers who didn't pay through the nose to buy an official (non-OEM) copy of Windows plus professional support, so unless you are one of these paying customers, I don't know how you should proceed: When I was still on Windows, once I had a problem and sent them a support request by filling in a form on their site. The answer was: "Your version of Windows is an OEM version, meant to be sold only together with a computer. It is not supported by Microsoft but only by your computer vendor." (So what was I to do? The shop where I had bought the computer had gone bankrupt by then.)

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Tony Mechelynck [:tonymec] from comment #9)
> Hm. I suspect a problem with how Windows passes URLs to the default browser.
> However Microsoft is notoriously unhelpful toward customers who didn't pay
> through the nose to buy an official (non-OEM) copy of Windows plus
> professional support, so unless you are one of these paying customers, I
> don't know how you should proceed: When I was still on Windows, once I had a
> problem and sent them a support request by filling in a form on their site.
> The answer was: "Your version of Windows is an OEM version, meant to be sold
> only together with a computer. It is not supported by Microsoft but only by
> your computer vendor." (So what was I to do? The shop where I had bought the
> computer had gone bankrupt by then.)

Well, I got my copy through my TechNet subscription, so I guess I am a direct paying customer. I'm gonna see if anyone else can reproduce this and track down if it is a Win8 regression before escalating this to MS, though.

Comment 11

[James Ross]

(In reply to Wes Kocher (:KWierso) from comment #5)
> This isn't a relatively recent issue for me, it's been happening at least
> since Nightly was in the 14.0a1 cycle, maybe earlier. It's been bugging me
> for a while, just never enough to actually get me to file the bug.

Were you on Windows 8 the whole time it has been happening? This is unlikely to be a ChatZilla issue and obviously your XULRunner hasn't changed for some time (2009), which leave the OS and Firefox. My money's on Firefox. ;)

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to James Ross from comment #11)
> Were you on Windows 8 the whole time it has been happening? This is unlikely
> to be a ChatZilla issue and obviously your XULRunner hasn't changed for some
> time (2009), which leave the OS and Firefox. My money's on Firefox. ;)

I only remember actively noticing it while using the various test builds and the RTM build of Windows 8.

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

(In reply to Wes Kocher (:KWierso) from comment #10)
> I'm gonna see if anyone else can reproduce this and
> track down if it is a Win8 regression before escalating this to MS, though.

Mossop says that it works correctly for him on Windows 7 using Chatzilla on XULRunner, so I guess there's something different in Windows 8 or Firefox.

Regardless, this happens from Skype > Firefox, so it's definitely not a Chatzilla bug.

Comment 14

[Brian R. Bondy [:bbondy]]

fwiw I confirmed this bug on win8 on skype but not on win7 with skype.

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

If desktop IE is the default browser, the hash is preserved from Skype to IE.

Comment 16

[Brian R. Bondy [:bbondy]]

More findings:
If you type Start Menu Button + R, and then in the run dialog run:
http://www.bugzilla.mozilla.org/#home 
It'll strip off the anchor name.

If you run cmd.exe, cd into your installation directory and run:
firefox.exe -url http://www.bugzilla.mozilla.org/#home
It'll keep the anchor name.

Comment 17

[Robert Strong (they/them - no direct email)]

We had a similar situation with the windows shell munging the command line. We used a standalone program to analyze all of the different permutations so we could do the right thing. I would suggest verifying that it is the shell that is stripping it and not something in our code by recording the command line sent to us early in the startup path just in case.

Comment 18

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Actually, this only seems to happen for me on Windows 8 with the Firefox versions that include the Metro stuff. I was using Elm Nightlies when I reported this bug, and now that Elm has merged back into m-c, I see it with normal Nightlies.

Comment 19

[Brian R. Bondy [:bbondy]]

Probably has to do with CEH which app invocations go through now only when MOZ_METRO is defined. Unassigned myself though due to prioritization.

Comment 20

[Matt Brubeck (:mbrubeck)]

I can reproduce this by clicking any link in Pidgin that includes a fragment identifier.

In a probably related issue, clicking HTTPS links in Pidgin is broken; it seems to pass Firefox a file: URI pointing to the Temporary Internet Files directory.

Comment 21

[Matt Brubeck (:mbrubeck)]

When I click on "http://limpet.net/#test" I get this output from CommandExecuteHandler (built with with #define SHOW_CONSOLE):

SetSelection param count: 1
SetSelection param: 'http://limpet.net/'
SetSelection target: http://limpet.net/
Initialize(open)
IExecuteCommandApplicationHostEnvironment::GetValue()
Couldn't get IExecuteCommandHost service from explorer. (80004001)
Execute()
Desktop Launch: verb:open exe:c:\Users\mbrub_000\src\elm\obj-metro\dist\bin\firefox.exe params:-url
http://limpet.net/

and when I click any https URI in Pidgin, I get:

SetSelection param count: 1
SetSelection param: 'C:\Users\mbrub_000\AppData\Local\Microsoft\Windows\Temporary Internet Files\Content.IE5\DJCOP9UF\CA3S0V90'
SetSelection target: C:\Users\mbrub_000\AppData\Local\Microsoft\Windows\Temporary Internet Files\Content.IE5\DJCOP9UF\CA3S0V90
Initialize(open)
IExecuteCommandApplicationHostEnvironment::GetValue()
Couldn't get IExecuteCommandHost service from explorer. (80004001)
Execute()
Desktop Launch: verb:open exe:c:\Users\mbrub_000\src\elm\obj-metro\dist\bin\firefox.exe params:-url
C:\Users\mbrub_000\AppData\Local\Microsoft\Windows\Temporary Internet Files\Content.IE5\DJCOP9UF\CA3S0V90

Comment 22

[Jim Mathies [:jimm]]

> When I click on "http://limpet.net/#test" I get this output from
> CommandExecuteHandler (built with with #define SHOW_CONSOLE):
> 
> SetSelection param count: 1
> SetSelection param: 'http://limpet.net/'

I have this fixed. 


> and when I click any https URI in Pidgin, I get:
> 
> SetSelection param count: 1
> SetSelection param:
> 'C:\Users\mbrub_000\AppData\Local\Microsoft\Windows\Temporary Internet
> Files\Content.IE5\DJCOP9UF\CA3S0V90'

I'd like to create a try build and have you test this. not sure if it's fixed, as I don't know the exact path these take through the ceh.

Comment 23

[Jim Mathies [:jimm]]

> > and when I click any https URI in Pidgin, I get:
> > 
> > SetSelection param count: 1
> > SetSelection param:
> > 'C:\Users\mbrub_000\AppData\Local\Microsoft\Windows\Temporary Internet
> > Files\Content.IE5\DJCOP9UF\CA3S0V90'
> 
> I'd like to create a try build and have you test this. not sure if it's
> fixed, as I don't know the exact path these take through the ceh.

https://tbpl.mozilla.org/?tree=Try&rev=a39dc68a6fa0

When you get a chance, can you take this build for a spin? The build has the ceh console hard coded on, with output that should help diagnose if anything goes wrong.

Comment 24

[Jim Mathies [:jimm]]

Attachment: wip

Comment 25

[Jim Mathies [:jimm]]

Attachment: patch

Comment 26

[bhavana bajaj [:bajaj]]

Considering metro is not riding the trains yet, this will not impact our Aurora Fx21 users.Hence no need to track for release.

Comment 27

[Asa Dotzler [:asa]]

This is a pretty serious defect in the fixed bug 831887. We should nail this down for sure.

Comment 28

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Comment on attachment 714558 [details] [diff] [review]
patch

Review of attachment 714558 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good with a couple nits.  Hopefully we can find a way to fix/workaround the issues with "file:///" URLs that contain spaces, though I'm fine doing that as a followup (as long as we comment).  Is there a good way to test this functionality?

::: browser/metro/shell/commandexecutehandler/CommandExecuteHandler.cpp
@@ +438,5 @@
>    }
>    return false;
>  }
>  
> +FORMATETC* GetPlainTextFormat() {

Instead of functions that return statics, I'd like to see 

namespace {
  const FORMATETC kPlainTextFormat = {CF_TEXT, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL};
  const FORMATETC kPlainTextWFormat = {CF_UNICODETEXT, 0, DVASPECT_CONTENT, -1, TYMED_HGLOBAL};
};

Then, anywhere we would have used GetPlainTextFormat or GetPlainTextWFormat, we use &kPlainTextFormat or &kPlainTextWFormat instead.

If it turns out that we want to use these values outside this file, we can move them into MetroUtils.cpp or a similar file.

@@ +463,5 @@
> +  STGMEDIUM store;
> +
> +  // unicode text
> +  if (SUCCEEDED(aDataObj->GetData(GetPlainTextWFormat(), &store))) {
> +    cstrText = static_cast<LPCWSTR>(GlobalLock(store.hGlobal));

It might be helpful to add a comment explaining that, since cstrText is a CString, the memory that store.hGlobal points to gets copied and can now be discarded.  This is just in case someone edits or copy+pastes this code in the future; we don't want them to accidentally use the memory pointed to by store.hGlobal after calling GlobalUnlock.

@@ +502,5 @@
> +    URL_COMPONENTS components = {0};
> +    components.lpszScheme = scheme;
> +    components.dwSchemeLength = sizeof(scheme)/sizeof(scheme[0]);
> +    components.dwStructSize = sizeof(components);
> +    if (!InternetCrackUrlW(cstrText, 0, 0, &components)) {

According to the MSDN documentation for InternetCrackUrl, it should not be used on "file://" URLs that contain spaces.  We should at the very least comment the fact that dwUrlPathLength will be wrong for "file://" URLs that contain spaces, if we can't find a workaround.

Comment 29

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes (:tabraldes) from comment #28)
> @@ +502,5 @@
> > +    URL_COMPONENTS components = {0};
> > +    components.lpszScheme = scheme;
> > +    components.dwSchemeLength = sizeof(scheme)/sizeof(scheme[0]);
> > +    components.dwStructSize = sizeof(components);
> > +    if (!InternetCrackUrlW(cstrText, 0, 0, &components)) {
> 
> According to the MSDN documentation for InternetCrackUrl, it should not be
> used on "file://" URLs that contain spaces.  We should at the very least
> comment the fact that dwUrlPathLength will be wrong for "file://" URLs that
> contain spaces, if we can't find a workaround.

I'll add a comment. I don't think we'll have issues with the parsing here since we are only using this to detect the scheme.

Comment 30

[Jim Mathies [:jimm]]

(In reply to Tim Abraldes (:tabraldes) from comment #28)
> Comment on attachment 714558 [details] [diff] [review]
> patch
> 
> Review of attachment 714558 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good with a couple nits.  Hopefully we can find a way to
> fix/workaround the issues with "file:///" URLs that contain spaces, though
> I'm fine doing that as a followup (as long as we comment).  Is there a good
> way to test this functionality?


Pretty much anything you launch on win8 goes through here. start -> run will trigger the code for various uri types.

Comment 31

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c64083c5696d

Comment 32

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Jim Mathies [:jimm] from comment #30)
> (In reply to Tim Abraldes (:tabraldes) from comment #28)
> > Comment on attachment 714558 [details] [diff] [review]
> > patch
> > 
> > Review of attachment 714558 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Looks good with a couple nits.  Hopefully we can find a way to
> > fix/workaround the issues with "file:///" URLs that contain spaces, though
> > I'm fine doing that as a followup (as long as we comment).  Is there a good
> > way to test this functionality?
> 
> 
> Pretty much anything you launch on win8 goes through here. start -> run will
> trigger the code for various uri types.

Right, I was unclear: What I meant was, "would it be difficult to write some _automated_ tests for this?" :)

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c64083c5696d

Comment 34

[Asa Dotzler [:asa]]

Verified with nightly built from from http://hg.mozilla.org/mozilla-central/rev/702d2814efbf  the hashes are passed in when clicking links with hashes in other apps .

Comment 35

[Samvedana (:Samvedana)]

User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0
Build ID: 20130823030204
Built from http://hg.mozilla.org/mozilla-central/rev/fb2318875cd4

WFM
Tested on windows 8 using latest nightly for iteration-12. The hashes are passed in when clicking links with hashes in other apps .