Closed Bug 1222572 Opened 10 years ago Closed 10 years ago

uriloader/exthandler/mac/nsOSHelperAppService.mm displays as a blank page

Categories

(Webtools Graveyard :: DXR, defect)

Product:
Webtools Graveyard
Component:
DXR
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 affected)

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
firefox45 --- affected

People

(Reporter: Gijs, Assigned: twointofive)

References

Details

https://dxr.mozilla.org/mozilla-central/source/uriloader/exthandler/mac/nsOSHelperAppService.mm It seems to be because there's some bogus bytes in there. It's fine if that breaks syntax highlighting after that point or something, but it's not OK for it to not display at all.
There are several issues here. 1) The file Gijs linked to is not UTF-8 encoded (line 69 has some "extra" characters - chardetect thinks it's ISO-8859-2, my editor thinks it's ISO-8859-15), so core's file_contents fails to decode it as the default utf-8, which causes us to later conclude that it's binary (even though file_contents notes that it's text). It seems to me like we should maybe try a little harder in file_contents. I can see a couple options: a) Treat the 'source_encoding' config as a (space-separated) list instead of a single variable, and attempt each encoding in the list. (Update docs.) b) Just have a fixed list of encodings we attempt, or let something like chardet try to figure it out. I would lean towards a), though that requires more work on the maintainer's part. b) is simpler for the maintainer, but opens up the possibility that someone could have a big repository all in non-utf-8 and never set source_encoding because it just works. How do you feel about those Erik? Or other ideas? *) It would maybe be nice to also have some better UI to indicate the difference between a file we consider binary and a file we consider text but don't know how to decode - that would notify people that they need to add another encoding, or at least explain why they can't view a file. 2) You can't actually browse to the page Gijs linked to since the source file is considered binary - you can only get there by writing out the url. There seem to be several bugs related to this kind of thing. a) As you noted Erik, we raise an exception when we try to return the permalink version of that file. Currently /rev processing only supports line files - the /source page sneaks in a permalink link for nsOSHelperAppService.mm (because we're is_text() and not is_image()? I'd have to check), but the decode fails and we throw an exception. So: *) I guess we shouldn't show a permalink if it's a file we can't decode (or it's binary), since there would be nothing to view at the permalink anyway. (For comparison, binary images don't get a sidebar at all - I think they should since we can (i.e. could) display a past revision via a permalink and the vcs links would still be useful, but that's another bug :-) **) We should give some indication though of what's going on (if we actually want to support going to these pages you can't browse to) - "I think this is a text file, but I can't decode it. See link for adding extra decoding options to DXR." or something. I guess we could add a "Binary file" indication for binary files as well, though I'm not sure that's as needed (currently you just get a page with no content). b) If you go to a /raw link for something that isn't a binary image we also assert. Maybe there are others like that... I would plan on doing 1 (a or b) and 2a) to fix this bug, and leaving 1*), 2a(binary images sidebar) and 2b (maybe closing the assert or adding more support) for other bugs.
Wow, thanks for digging into that in such thoroughness, Tom. Regarding point (1), file_contents() concludes that it's binary, according to my debugger. What did you mean by "which causes us to later conclude that it's binary (even though file_contents notes that it's text)"? This file isn't even *any* encoding, I would argue; in fact, it seems like the point of the \xFF\xFF sequence is to exemplify an undecodeable sequence. I'd be up for doing a better job of mime.is_text(), which has always been cheap. It's amazing we got away with that as long as we did. We'd want to stay relatively quick, though, lest we make indexing take forever. Maybe a good heuristic would be a reasonable pattern of CRs or LFs in the first chunk of the file (sane line lengths, for example). Then, if we pass that test, we give chardet a run at it--chardet is really slow--and record the actual encoding. The encoding stuff could, as you have found, use some love. :-) I'd strongly consider getting rid of the source_encoding config option altogether if we could do a reasonably performant autodetection. That was always a bit of a get-it-working-quick hack for one of abbeyj's trees, so he's a good one to run it by. Regarding (1*), YES. If people are constructing their own URLs (like Gijs is likely using keyword bookmarks to do), let's at least add "(binary file)" or something to the template for when they hit one. That way they don't end up wondering why it's blank. Regarding (2*), I don't have a strong feeling on whether to avoid showing a permalink for binary files. I could see someone wanting to save one in case we someday show something useful on it, like file size or something. But /rev/ shouldn't crash in any case. I think we're agreed: let's see if we can do a better job at encoding detection. But that can be a separate bug if you wish. The minimum fix for this would be to stop /rev/ from crashing and to show "(binary file)" instead of a blank page.
(In reply to Erik Rose [:erik][:erikrose] from comment #4) > Regarding point (1), file_contents() concludes that it's binary, according > to my debugger. What did you mean by "which causes us to later conclude that > it's binary (even though file_contents notes that it's text)"? My way of thinking was that file_contents doesn't itself return binary or not binary, it returns (is_text && can_decode) or not, and this file actually falls in the "is_text but can't decode [because we only try one decode]" hole which eventually becomes is_binary at a later point when we interpret the fact that file_contents returned a str instead of unicode as meaning it's a binary file (core's FileToIndex needles() via FileToSkim.contains_text()). But if we're going to do our very best on each is_text file to decode it, then maybe the distinction I was making goes away, and we really should regard anything we can't decode as binary, even if it returned True on is_text. > This file isn't even *any* encoding, I would argue; in fact, it seems like > the point of the \xFF\xFF sequence is to exemplify an undecodeable sequence. Ah, I missed that point, thanks. But maybe that's a problem then, because chardet says that file is 'ISO-8859-2' (with confidence 0.625247188846775), and decoding the file as that encoding succeeds... Would we require confidence over a certain threshold? > I'd be up for doing a better job of mime.is_text(), which has always been > cheap. It's amazing we got away with that as long as we did. is_text() returns True on this file, which I think is what we want, right? I would be inclined to keep using the current is_text if that's the case, stupid as it is :) > Then, if we pass that > test, we give chardet a run at it--chardet is really slow--and record the > actual encoding. The encoding stuff could, as you have found, use some love. > :-) I'd strongly consider getting rid of the source_encoding config option > altogether if we could do a reasonably performant autodetection. If chardet is really slow, then running chardet on every file that returns True on is_text sounds really slow on a bigger repo like m-c... I can give it a run though to find out how much slower. > I think we're agreed: let's see if we can do a better job at encoding > detection. But that can be a separate bug if you wish. The minimum fix for > this would be to stop /rev/ from crashing and to show "(binary file)" > instead of a blank page. Great, thanks!
Well, I wrote up file_contents to do: if is_text(contents): guess = chardet.detect(contents) # (The 'guess['encoding']' check here shouldn't be necessary, I was just being paranoid) if guess['confidence'] > 0 and guess['encoding']: try: contents = contents.decode(guess['encoding']) except UnicodeDecodeError: pass # Leave contents as str. return contents and it indexed m-c in 5:14:35. On my machine I think my last couple indexings have been closer to 5:05, but I know in the past I've had a range of basically 5:00 to 5:25, so this actually seems perfectly reasonable to me, timewise. And it does decode nsOSHelperAppService.mm. The downsides I can see to switching to chardet detection only are that there may be encodings that str.decode would recognize that chardet does not (the str.decode codec list looks longer than the chardet list, so I assume there are? Maybe not though.), and by chardet's own admission they can't always guess right (i.e. sometimes they guess one encoding and it's actually another), so in some cases we could guess wrong with no user recourse. It's also possible there's already someone out there relying on the source_encoding config for an encoding that chardet wouldn't recognize... Maybe we could switch to making chardet be the default but then use a source_encoding list followed by chardet if the source_encoding list is specified? Also it looks like /xFF is a valid encoded character in ISO-8859-2, so I think we're fine there.
See Also: → 1261789
For reference, if I go through the list of encodings that chardet detects and place a "Y" in front of a corresponding encoding in the python codecs list, the list below is what i get. So it would seem that there are definitely files that chardet wouldn't recognize that str.decode() could, given the right codec (via "source_encoding" or something). Still, I'd be fine with only using chardet, I think that would be the cleanest solution, even if not completely general - just let me know what you'd like to go with and I'll put something together. Y ascii 646, us-ascii English Y big5 big5-tw, csbig5 Traditional Chinese big5hkscs big5-hkscs, hkscs Traditional Chinese cp037 IBM037, IBM039 English cp424 EBCDIC-CP-HE, IBM424 Hebrew cp437 437, IBM437 English cp500 EBCDIC-CP-BE, EBCDIC-CP-CH, IBM500 Western Europe cp720 Arabic cp737 Greek cp775 IBM775 Baltic languages cp850 850, IBM850 Western Europe cp852 852, IBM852 Central and Eastern Europe Y cp855 855, IBM855 Bulgarian, Byelorussian, Macedonian, Russian, Serbian cp856 Hebrew cp857 857, IBM857 Turkish cp858 858, IBM858 Western Europe cp860 860, IBM860 Portuguese cp861 861, CP-IS, IBM861 Icelandic cp862 862, IBM862 Hebrew cp863 863, IBM863 Canadian cp864 IBM864 Arabic cp865 865, IBM865 Danish, Norwegian Y cp866 866, IBM866 Russian cp869 869, CP-GR, IBM869 Greek cp874 Thai cp875 Greek cp932 932, ms932, mskanji, ms-kanji Japanese cp949 949, ms949, uhc Korean cp950 950, ms950 Traditional Chinese cp1006 Urdu cp1026 ibm1026 Turkish cp1140 ibm1140 Western Europe Y cp1250 windows-1250 Central and Eastern Europe Y cp1251 windows-1251 Bulgarian, Byelorussian, Macedonian, Russian, Serbian Y cp1252 windows-1252 Western Europe Y cp1253 windows-1253 Greek cp1254 windows-1254 Turkish Y cp1255 windows-1255 Hebrew cp1256 windows-1256 Arabic cp1257 windows-1257 Baltic languages cp1258 windows-1258 Vietnamese Y euc_jp eucjp, ujis, u-jis Japanese euc_jis_2004 jisx0213, eucjis2004 Japanese euc_jisx0213 eucjisx0213 Japanese Y euc_kr euckr, korean, ksc5601, ks_c-5601, ks_c-5601-1987, ksx1001, ks_x-1001 Korean Y gb2312 chinese, csiso58gb231280, euc- cn, euccn, eucgb2312-cn, gb2312-1980, gb2312-80, iso- ir-58 Simplified Chinese gbk 936, cp936, ms936 Unified Chinese Y gb18030 gb18030-2000 Unified Chinese Y hz hzgb, hz-gb, hz-gb-2312 Simplified Chinese Y iso2022_jp csiso2022jp, iso2022jp, iso-2022-jp Japanese iso2022_jp_1 iso2022jp-1, iso-2022-jp-1 Japanese iso2022_jp_2 iso2022jp-2, iso-2022-jp-2 Japanese, Korean, Simplified Chinese, Western Europe, Greek iso2022_jp_2004 iso2022jp-2004, iso-2022-jp-2004 Japanese iso2022_jp_3 iso2022jp-3, iso-2022-jp-3 Japanese iso2022_jp_ext iso2022jp-ext, iso-2022-jp-ext Japanese Y iso2022_kr csiso2022kr, iso2022kr, iso-2022-kr Korean latin_1 iso-8859-1, iso8859-1, 8859, cp819, latin, latin1, L1 West Europe Y iso8859_2 iso-8859-2, latin2, L2 Central and Eastern Europe iso8859_3 iso-8859-3, latin3, L3 Esperanto, Maltese iso8859_4 iso-8859-4, latin4, L4 Baltic languages Y iso8859_5 iso-8859-5, cyrillic Bulgarian, Byelorussian, Macedonian, Russian, Serbian iso8859_6 iso-8859-6, arabic Arabic Y iso8859_7 iso-8859-7, greek, greek8 Greek Y iso8859_8 iso-8859-8, hebrew Hebrew iso8859_9 iso-8859-9, latin5, L5 Turkish iso8859_10 iso-8859-10, latin6, L6 Nordic languages iso8859_11 iso-8859-11, thai Thai languages iso8859_13 iso-8859-13, latin7, L7 Baltic languages iso8859_14 iso-8859-14, latin8, L8 Celtic languages iso8859_15 iso-8859-15, latin9, L9 Western Europe iso8859_16 iso-8859-16, latin10, L10 South-Eastern Europe johab cp1361, ms1361 Korean Y koi8_r Russian koi8_u Ukrainian Y mac_cyrillic maccyrillic Bulgarian, Byelorussian, Macedonian, Russian, Serbian mac_greek macgreek Greek mac_iceland maciceland Icelandic mac_latin2 maclatin2, maccentraleurope Central and Eastern Europe mac_roman macroman Western Europe mac_turkish macturkish Turkish ptcp154 csptcp154, pt154, cp154, cyrillic-asian Kazakh Y shift_jis csshiftjis, shiftjis, sjis, s_jis Japanese shift_jis_2004 shiftjis2004, sjis_2004, sjis2004 Japanese shift_jisx0213 shiftjisx0213, sjisx0213, s_jisx0213 Japanese Y utf_32 U32, utf32 all languages Y utf_32_be UTF-32BE all languages Y utf_32_le UTF-32LE all languages Y utf_16 U16, utf16 all languages Y utf_16_be UTF-16BE all languages (BMP only) Y utf_16_le UTF-16LE all languages (BMP only) utf_7 U7, unicode-1-1-utf-7 all languages Y utf_8 U8, UTF, utf8 all languages utf_8_sig all languages
Also, if we're going to support more encodings, then I guess the is_text test is going to have to change after all since utf-16 contains lots of '\0's - the utf-16 files from bug 1267189, for example, are currently considered binary by is_text. Any feelings on trying binaryornot? https://pypi.python.org/pypi/binaryornot It at least does the right thing on the files being discussed in this bug... I can do another perf run on m-c to see how it performs.
Assignee: nobody → twointofive
Commit pushed to master at https://github.com/mozilla/dxr https://github.com/mozilla/dxr/commit/7994521f08df419a7e2118c5e14179f379b4f79f Show '(binary file)' for /source/binary, don't assert on /rev/binary. Fixes bug 1222572. Also, in test_binary_files.py, don't allow redirect on the 'path:some_bytes' query since if/when we switch to jumping to a single result, that test would still pass (for the wrong reasons).
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.