288154 - If the path has 0x7c in its name, firefox cannot open the file (Should encode 0x7c to %7C)

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P1)

Description

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

See this report with UTF-8.

If the path has 0x7c in its name(e.g., on Win-Ja, "c:\翻訳\test.html"),
firefox cannot open the file at launching from Explorer.
This is not reproduced on Suite.
And it is not reproduced by entering "http://翻訳.jp" from Run of Start Menu.

"翻訳" is 0x96 0x7C 0x96 0xF3 in Shift_JIS.

Comment 1

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

It is not reproduced when firefox already exists.
But excluding if "Open links from other applications in" is "New Window".

Comment 2

[Jo Hermans]

related to bug 221445 ? (0x7C = pipe)

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

This bug is caused by following code.

browser.js#1363, browser.js#1371, browser.js#1396
These lines use |aURIString.split("|");|
This should be processed in UTF16 or UTF8.

Comment 4

[Mike Connor [:mconnor]]

not a regression in Firefox, this has never worked, at least since 2002 sometime.

*** This bug has been marked as a duplicate of 221445 ***

Comment 5

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

No, it is not the bug of "|".
This problem is the multi-byte charachter that has 0x7c.
not single charachter problem.

Jungshik:
Do you think it?

Comment 6

[Hidehiro Kozawa]

original reporter in Japanese bugzilla says not reproduced at Phoenix 0.1 �` 0.4
and reproduced after Phoenix 0.5.

Comment 7

[Mike Connor [:mconnor]]

the problem is in the same code, and rather than fixing the bad implementation,
we need to implement multiple homepages in a sane way.

Comment 8

[Mike Connor [:mconnor]]

Phoenix 0.5 shipped in December 2002, so that jibes with what I said.  Something
that worked over two years and a couple name changes in the past is a bit far
back for a regression.

This is a byproduct of that bug, and while we could just reopen and mark
depending, there isn't much point leaving two bugs to spam.

Comment 9

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

I'm reopening this bug.
This bug is not dup to bug 221445.
"0x96 0x7C 0x96 0xF3" is encoded to "%96|%96%F3" by Firefox.
It is a bug. It should be "%96%7C%96%F3".

Comment 10

[Jungshik Shin]

Does it work if you drag and drop a file to a running firefox? How about opening
via 'File | Open'? 

Does it make any difference whether firefox is running or not when you
double-click on a file in Windows Explorer? 

If answers to all my questions are yes (which I think is the case), this is
clearly a dupe of bug 278161

For now, I'm making this depend on bug 278161. If it's confirmed that my guess
is right, please, resolve it as a dupe of bug 278161

Comment 11

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

If Bug 278161 is fixed, this bug is fixed too.
When do you fix bug 278161?
If you cannot fix before firefox 1.5 beta, I should fix this before 1.5 beta
release.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

This bug is reproduced when it is booting only.
I.e., On Windows, an user boot firefox by the HTML file double click.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch rv1.0

It fixes the problem. But it is ad hoc.

If '|' is after non-ASCII char, it may be a part of multi byte char. Becuase
'|' is not valid character for path and file name of URI. But it was used for
drive delimiter. If we escape '|' always, it makes another problem.(we cannot
open "file:///C|/foo/bar.txt".) But this patch doesn't make this problem.

However, if bug 278161 is fixed by jshin, this patch is not needed.

Comment 14

[Darin Fisher]

Comment on attachment 190168 [details] [diff] [review]
Patch rv1.0

>Index: xpcom/io/nsEscape.cpp

>+      // And, we should escape the pipe character after non-ascii character.
>+      // The pipe character is not valid character for path and file name.
>+      // But it's used for dive delimiter instead of colon (C|/foo/bar.txt).
>+      // However, the pipe character after non-ascii may be a part of multi
>+      // byte character.

I would change this text to something simpler like this:

	// And, we should escape the '|' character when it occurs after any
	// non-ASCII character as it may be part of a multi-byte character.

I don't think there's any need to mention drive delimiters here.


r+sr=darin

Comment 15

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

Attachment: Patch rv1.1

The risk is low. This is very important intl bug.

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)(away 4/27 - 5/6)]

checked-in.

Comment 17

[Dylan]

Attachment: WIP: Backed out changeset (Bug 288154) for encoding pipe character in URL query parser

Comment 18

[Phabricator Automation]

Comment on attachment 9341048 [details]
WIP: Backed out changeset (Bug 288154) for encoding pipe character in URL query parser

Revision D182064 was moved to bug 1674369. Setting attachment 9341048 [details] to obsolete.

Comment 19

[Phabricator Automation]

Comment on attachment 9341048 [details]
WIP: Backed out changeset (Bug 288154) for encoding pipe character in URL query parser

Revision D182064 was moved to bug 1674369. Setting attachment 9341048 [details] to obsolete.