bug#18310: 24.3.93; relative links don't work in eww and Windows 7

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[João Távora]

Hi maintainers,

On Windows 7:

    emacs -Q
    M-x eww RET
    http://www.lispworks.com/documentation/HyperSpec/Front/index.htm RET

Try to follow any of the relative links on the page, they point to
something strange like "www.lispworks.comz" (note the final "z") which
basically breaks all navigation.

The `shr-url' property at point shows

    http://www.lispworks.comz:/documentation/HyperSpec/Front/StartPts.htm

And everything indicates this is a consequence of a previous bug fix of
mine for bug#17217 [1], which does not manifest itself in my Linux
box. I'm pretty sure it also did not manifest itself on my old Windows
XP box.

In that fix, I used the function `expand-file-name' in `shr-expand-url'
to compute the expanded URL for "totally relative" case of hrefs like
"../something".

This new bug seems to be caused by `expand-file-name' insisting on
producing a valid windows pathname (with drive letter), even though it
was passed the second argument DEFAULT-DIRECTORY.

That is, on my Windows 7 system:

   (expand-file-name "../bla" "/something/else")

expands to

   "z:/something/bla"

Whereas I intented it to expand to "/something/bla".

My HOME variable is set to at "z:", but unsetting it does not help
either. I don't have time right now to look at the C-code for
`expand-file-name'.

João

[1]: http://lists.gnu.org/archive/html/bug-gnu-emacs/2014-04/msg00266.html


In GNU Emacs 24.3.93.1 (i686-pc-mingw32)
 of 2014-08-15 on LEG570
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking 'CFLAGS=-O0 -g3' CPPFLAGS=-DGLYPH_DEBUG=1'

Important settings:
  value of $LC_CTYPE: UTF-8
  value of $LANG: C.UTF-8
  locale-coding-system: cp1252

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Date: Thu, 21 Aug 2014 11:33:32 +0100
> 
> On Windows 7:
> 
>     emacs -Q
>     M-x eww RET
>     http://www.lispworks.com/documentation/HyperSpec/Front/index.htm RET
> 
> Try to follow any of the relative links on the page, they point to
> something strange like "www.lispworks.comz" (note the final "z") which
> basically breaks all navigation.
> 
> The `shr-url' property at point shows
> 
>     http://www.lispworks.comz:/documentation/HyperSpec/Front/StartPts.htm
> 
> And everything indicates this is a consequence of a previous bug fix of
> mine for bug#17217 [1], which does not manifest itself in my Linux
> box. I'm pretty sure it also did not manifest itself on my old Windows
> XP box.

I'm pretty sure it did happen on XP.

> In that fix, I used the function `expand-file-name' in `shr-expand-url'
> to compute the expanded URL for "totally relative" case of hrefs like
> "../something".
> 
> This new bug seems to be caused by `expand-file-name' insisting on
> producing a valid windows pathname (with drive letter), even though it
> was passed the second argument DEFAULT-DIRECTORY.

That's what expand-file-name is supposed to do: it should produce a
fully-qualified file name that unequivocally describes a file.  That
means the file name must specify the drive letter.

> That is, on my Windows 7 system:
> 
>    (expand-file-name "../bla" "/something/else")
> 
> expands to
> 
>    "z:/something/bla"
> 
> Whereas I intented it to expand to "/something/bla".

"/something/bla" is not a fully-qualified file name, not on Windows.
As long as the drive letter is not specified, the file name is
ambiguous.

> My HOME variable is set to at "z:", but unsetting it does not help
> either.

I don't think it's because of HOME, since there was no "~" in the file
name.  I'm guessing that the default-directory of the buffer where you
used that code was on the z: drive, so Emacs used that to complete the
missing drive letter.

> I don't have time right now to look at the C-code for
> `expand-file-name'.

There's nothing wrong with expand-file-name, please don't waste your
time looking there.  The problem is in the change you made.  You
cannot use expand-file-name on anything but a file name on a local
filesystem, even if the "thing" you have to deal with happens to look
like a file name and uses slashes as separators.  expand-file-name
assumes without testing that its arguments are syntactically valid
local file names, and will produce invalid results if they are not.

Please use url-expand-file-name instead, it does exactly what you
want, and does that portably.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

> I'm pretty sure it did happen on XP.

I wasn't aware of the bug until the switch to Windows 7. Maybe I
didn't use eww with relative links at all, or maybe other factors
influencing `expand-file-name' weren't acting.

> I don't think it's because of HOME, since there was no "~" in the file
> name.  I'm guessing that the default-directory of the buffer where you
> used that code was on the z: drive, so Emacs used that to complete the
> missing drive letter.

Something other than `default-directory' seems to be influencing it. I
did some tests:

   (let ((default-directory "/"))
     (expand-file-name "../" "/something/bla"))

   (let ((default-directory "\\"))
     (expand-file-name "../" "/something/bla"))
    
   (let ((default-directory nil))
     (expand-file-name "../" "/something/bla"))

   (let ((default-directory ""))
     (expand-file-name "../" "/something/bla"))

All produce "z:/something". However

   (let ((default-directory "c:"))
     (expand-file-name "../" "/something/bla"))

   (let ((default-directory "\\\\mymachine"))
     (expand-file-name "../" "/something/bla"))

Do produce "c:/something/" and "//mymachine/something/",
respectively. Finally

   (let ((default-directory "\\\\"))
     (expand-file-name "../" "/something/bla"))

Crashed the Emacs process on my machine. Can you reproduce? I don't have
debuggers handy on Windows, so can't even get a backtrace.

> There's nothing wrong with expand-file-name, please don't waste your
> time looking there.

Well, there's the fact that it crashed... but anyway I didn't say there
was.  I just didn't understand what it did. Now that you have explained,
I do, more than before at least.

But from its docstring it's not clear. I didn't understand that
"canonicalize" means different for different platforms. Maybe because
Emacs use of "/" on Windows is an exception I'm used to.

The docstring should explain its relationship with the
`default-directory' variable, which in the current version sounds like
it's shadowed completely by the DEFAULT-DIRECTORY parameter. Perhaps a
sentence could be added.

   If DEFAULT-DIRECTORY is nil or missing, the current buffer's value of
   `default-directory' is used. Even if DEFAULT-DIRECTORY is non-nil,
   `default-directory' may still be used to help canonicalize the
   resulting name for the current platform.

If it's being influenced by anything else, it should also explain it.

> Please use url-expand-file-name instead, it does exactly what you
> want, and does that portably.

That sounds perfectly right, but I don't have time to setup my Emacs
development environment, sorry, so maybe someone else can perform the
change. I think it's a bug to fix for 24.4.

João

PS: Had a look at `url-expand-file-name': isn't it doing to much for
`shr-expand-url''s purposes?  There seems to be an overlap between the
two. Anyway for 24.4 any fix will do I guess.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: 18310@debbugs.gnu.org
> Date: Thu, 21 Aug 2014 16:43:48 +0100
> 
> Something other than `default-directory' seems to be influencing it. I
> did some tests:

Like I said: Emacs uses the current drive to complete the missing
drive letter.  That is what you see.

> Finally
> 
>    (let ((default-directory "\\\\"))
>      (expand-file-name "../" "/something/bla"))
> 
> Crashed the Emacs process on my machine.

It's not a crash, it's a deliberate abort.  "\\\\" (i.e., 2
backslashes in a row without anything after that) is an invalid file
name on Windows.  Just don't do that.

> The docstring should explain its relationship with the
> `default-directory' variable, which in the current version sounds like
> it's shadowed completely by the DEFAULT-DIRECTORY parameter. Perhaps a
> sentence could be added.
> 
>    If DEFAULT-DIRECTORY is nil or missing, the current buffer's value of
>    `default-directory' is used. Even if DEFAULT-DIRECTORY is non-nil,
>    `default-directory' may still be used to help canonicalize the
>    resulting name for the current platform.

How does that obscure hint help?  It doesn't tell anything that mere
mortals could understand.

Once again, you are talking about semi-invalid use cases.  IMO,
complicating the doc string (which is not at all simple as it is) on
behalf of those use cases is not TRT.

> PS: Had a look at `url-expand-file-name': isn't it doing to much for
> `shr-expand-url''s purposes?

I have no idea, but as long as you need to resolve relative URLs, it
is your friend.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: joaotavora@gmail.com (João Távora)
>> Cc: 18310@debbugs.gnu.org
>> Date: Thu, 21 Aug 2014 16:43:48 +0100
>> 
>> Something other than `default-directory' seems to be influencing it. I
>> did some tests:
>
> Like I said: Emacs uses the current drive to complete the missing
> drive letter.  That is what you see.

OK. How is the "current drive" calculated when `default-directory' is
nil?

>> Finally
>> 
>>    (let ((default-directory "\\\\"))
>>      (expand-file-name "../" "/something/bla"))
>> 
>> Crashed the Emacs process on my machine.
>
> It's not a crash, it's a deliberate abort.  "\\\\" (i.e., 2
> backslashes in a row without anything after that) is an invalid file
> name on Windows.

Would signalling an error be very wrong, does the process really need to
be aborted? I mean unprotected code may easily lead to that invalid
case.

> How does that obscure hint help? It doesn't tell anything that mere
> mortals could understand. 

It mirrors the function's obscurity, so mere mortals can at least be
warned of death by deliberate abort.

Anyway it was meant for you to complete with enlightenment about the
function's interaction with things other than `default-directory',
something you didn't do. 

And needn't do, at least for me, since at least now I know to stay away
from `expand-file-name's quirks.

>  Once again, you are talking about semi-invalid use cases.  IMO,
> complicating the doc string (which is not at all simple as it is) on
> behalf of those use cases is not TRT.

BTW "Semi-invalid" uses, or even downright absurd use cases, are what
development/testing/experimentation is all about. IMO aborting the
process and not at least warning elisp users about invalid cases in the
documentation is not good practice in my opinion.

João

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: 18310@debbugs.gnu.org
> Date: Thu, 21 Aug 2014 17:54:11 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: joaotavora@gmail.com (João Távora)
> >> Cc: 18310@debbugs.gnu.org
> >> Date: Thu, 21 Aug 2014 16:43:48 +0100
> >> 
> >> Something other than `default-directory' seems to be influencing it. I
> >> did some tests:
> >
> > Like I said: Emacs uses the current drive to complete the missing
> > drive letter.  That is what you see.
> 
> OK. How is the "current drive" calculated when `default-directory' is
> nil?

There's a system call to get the "current drive", where Emacs runs.

> >> Finally
> >> 
> >>    (let ((default-directory "\\\\"))
> >>      (expand-file-name "../" "/something/bla"))
> >> 
> >> Crashed the Emacs process on my machine.
> >
> > It's not a crash, it's a deliberate abort.  "\\\\" (i.e., 2
> > backslashes in a row without anything after that) is an invalid file
> > name on Windows.
> 
> Would signalling an error be very wrong, does the process really need to
> be aborted?

Yes.  This is a deliberate trap for code either in Emacs itself or in
Lisp applications that uses such invalid default-directory values.

> I mean unprotected code may easily lead to that invalid case.

Not "easily", no.  Usually, default-directory comes from some file or
buffer.  But if some code does use that, we want to catch it
red-handed, because there's no way to know what other damage it can do
down the road.

> BTW "Semi-invalid" uses, or even downright absurd use cases, are what
> development/testing/experimentation is all about. IMO aborting the
> process and not at least warning elisp users about invalid cases in the
> documentation is not good practice in my opinion.

Emacs is not mission-critical software.  If it were, then I'd agree
with you (I happen to develop mission-critical software for a living).

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> >> Finally
>> >> 
>> >>    (let ((default-directory "\\\\"))
>> >>      (expand-file-name "../" "/something/bla"))
>> >> 
>> >> Crashed the Emacs process on my machine.
>> >
>> > It's not a crash, it's a deliberate abort.  "\\\\" (i.e., 2
>> > backslashes in a row without anything after that) is an invalid file
>> > name on Windows.
>> 
>> Would signalling an error be very wrong, does the process really need to
>> be aborted?
>
> Yes.  This is a deliberate trap for code either in Emacs itself or in
> Lisp applications that uses such invalid default-directory values.

But it really seems arbitrary, since

   (let ((default-directory "><>:\"/\|?*"))
     (expand-file-name "../" "/something/bla"))

contains very much an invalid `default-directory' value and does not
"deliberately abort". It returs "z:/something" as always (i.e
default-directory is fully ignored).

>> I mean unprotected code may easily lead to that invalid case.
>
> Not "easily", no.  Usually, default-directory comes from some file or
> buffer.

It can very well be lexically bound to something that eventually
evaluates to "////". For example to temporarily work on a directory in a
Lisp program. Emacs own lisp code seems littered with (let
((default-directory ...)), just grep for it.

> But if some code does use that, we want to catch it red-handed,
> because there's no way to know what other damage it can do down the
> road.

To be clear, I fully support your "early abort" cause. But one thing is
aborting the command the other thing is aborting the process. I think
you should do the latter if it's the Emacs' internals that caused the
(supposedly unrecoverable) error. But you should do the former if it was
the user's Lisp program that provided incorrect input.

I've looked at the code and expand-file-name is a woolly mammoth so
maybe that's hard to do, but it would be the right thing IMO.

> Emacs is not mission-critical software.  If it were, then I'd agree
> with you (I happen to develop mission-critical software for a living).

Me too. In Lisp. But that's besides the point. Just because Emacs exists
"in the hope that it will be useful" doesn't mean one shouldn't care
about user's critical mission.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Eli Zaretskii]

> From: joaotavora@gmail.com (João Távora)
> Cc: 18310@debbugs.gnu.org
> Date: Fri, 22 Aug 2014 11:26:03 +0100
> 
> > Yes.  This is a deliberate trap for code either in Emacs itself or in
> > Lisp applications that uses such invalid default-directory values.
> 
> But it really seems arbitrary, since
> 
>    (let ((default-directory "><>:\"/\|?*"))
>      (expand-file-name "../" "/something/bla"))
> 
> contains very much an invalid `default-directory' value and does not
> "deliberately abort". It returs "z:/something" as always (i.e
> default-directory is fully ignored).

It's not arbitrary: "\\\\" signals a beginning of a UNC, which should
have the host name and the share name following it, while the string
you used above is simply random garbage.

> >> I mean unprotected code may easily lead to that invalid case.
> >
> > Not "easily", no.  Usually, default-directory comes from some file or
> > buffer.
> 
> It can very well be lexically bound to something that eventually
> evaluates to "////".

Then we want to catch those uses.

> For example to temporarily work on a directory in a Lisp program.

"\\\\" is not a directory, it's a butchered UNC.

> To be clear, I fully support your "early abort" cause. But one thing is
> aborting the command the other thing is aborting the process. I think
> you should do the latter if it's the Emacs' internals that caused the
> (supposedly unrecoverable) error. But you should do the former if it was
> the user's Lisp program that provided incorrect input.

Signaling an error is not prominent enough to catch the attention, it
can be easily skipped, ignored, or even disabled.

> I've looked at the code and expand-file-name is a woolly mammoth so
> maybe that's hard to do, but it would be the right thing IMO.

expand-file-name already does what is humanly possible to handle many
weird cases.  This one is not, and should not, be one of them.

> Just because Emacs exists "in the hope that it will be useful"
> doesn't mean one shouldn't care about user's critical mission.

When Emacs aborts, it auto-saves, so it does care about users.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Stefan Monnier]

> To be clear, I fully support your "early abort" cause. But one thing is
> aborting the command the other thing is aborting the process. I think
> you should do the latter if it's the Emacs' internals that caused the
> (supposedly unrecoverable) error. But you should do the former if it was
> the user's Lisp program that provided incorrect input.

FWIW, I fully agree.  Luckily I I don't need to use an Emacs compiled
for w32, but if I were to have my Emacs die from under me just because
I used an incorrect default-directory, I wouldn't like it all.


        Stefan

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Eli Zaretskii]

> From: Stefan Monnier <monnier@iro.umontreal.ca>
> Cc: Eli Zaretskii <eliz@gnu.org>,  18310@debbugs.gnu.org
> Date: Fri, 22 Aug 2014 10:39:13 -0400
> 
> > To be clear, I fully support your "early abort" cause. But one thing is
> > aborting the command the other thing is aborting the process. I think
> > you should do the latter if it's the Emacs' internals that caused the
> > (supposedly unrecoverable) error. But you should do the former if it was
> > the user's Lisp program that provided incorrect input.
> 
> FWIW, I fully agree.

Then we'll have to disagree.

> Luckily I I don't need to use an Emacs compiled for w32

Then I submit that my opinion matters more than yours here.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> PS: Had a look at `url-expand-file-name': isn't it doing to much for
>> `shr-expand-url''s purposes?
>
> I have no idea, but as long as you need to resolve relative URLs, it
> is your friend.

Back on topic, the diff in PS seems to work. With the new calling
convention, of first concatenating the car and cadr of `base', so does
`expand-file-name', by the way.

Can anyone verify and install it?

João

diff --git a/lisp/net/shr.el b/lisp/net/shr.el
index 5844257..244d6d6 100644
--- a/lisp/net/shr.el
+++ b/lisp/net/shr.el
@@ -32,6 +32,8 @@
 
 (eval-when-compile (require 'cl))
 (eval-when-compile (require 'url))      ;For url-filename's setf handler.
+
+(require 'url-expand)                  ; For url-expand-file-name
 (require 'browse-url)
 
 (defgroup shr nil
@@ -610,7 +612,7 @@ size, and full-buffer size."
 	 (concat (nth 3 base) url))
 	(t
 	 ;; Totally relative.
-	 (concat (car base) (expand-file-name url (cadr base))))))
+         (url-expand-file-name url (concat (car base) (cadr base))))))
 
 (defun shr-ensure-newline ()
   (unless (zerop (current-column))

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Glenn Morris]

João Távora wrote:

> Can anyone verify and install it?

Sorry, don't have time to test it right now, but I assume you checked
the basic functionality, and it looks like you can install it yourself
(in emacs-24 branch)?

Would be good to get some automated tests for shr-expand-url.

I wonder if it even needs to exist (in trunk); since I doubt
shr needs some form of url expansion that url.el hasn't already
implemented.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[João Távora]

On Tue, Aug 26, 2014 at 7:11 PM, Glenn Morris <rgm@gnu.org> wrote:
> João Távora wrote:
>
>> Can anyone verify and install it?
> Sorry, don't have time to test it right now, but I assume you checked
> the basic functionality, and it looks like you can install it yourself
> (in emacs-24 branch)?

I use it daily, but mostly on
http://www.lispworks.com/documentation/HyperSpec/Front/index.htm
which is pretty link-intensive with absolute, relative and anchor links,
but I can't say I've tested exhaustively.

I could, but I'd have to setup my linux box again, which I don't have access
to right now. I could do  emacs development in some other environment, or
I could keep my sanity :-)
Anyway, about next week there should be some time to set it up.

> Would be good to get some automated tests for shr-expand-url.

I could, if I understood it correctly...

> I wonder if it even needs to exist (in trunk); since I doubt
> shr needs some form of url expansion that url.el hasn't already
> implemented.

Precisely, but for some reason Lars used `url-generic-parse-url'
in `shr-parse-base', but not quite. ..

-- 
João Távora

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Glenn Morris]

Version: 24.3.94

Fine; applied.

bug#18310: 24.3.93; relative links don't work in eww and Windows 7

[Lars Magne Ingebrigtsen]

João Távora <joaotavora@gmail.com> writes:

>> I wonder if it even needs to exist (in trunk); since I doubt
>> shr needs some form of url expansion that url.el hasn't already
>> implemented.
>
> Precisely, but for some reason Lars used `url-generic-parse-url'
> in `shr-parse-base', but not quite. ..

I dimly remember writing most of that function before I became aware of
the `url-generic-parse-url', which probably explains the
inconsistency...

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no