ollm / OpenComic

Comic and Manga reader, written with Node.js and using Electron
GNU General Public License v3.0
867 stars 76 forks source link

[Bug]: Blank pages when opening .cbz archives with very long filenames #241

Open himouto opened 2 months ago

himouto commented 2 months ago

Preflight Checklist

OpenComic Version

1.2.0

Operating System

Windows 11 23H2 build 22631.3767

Steps to reproduce

Open or rename a comics folders with a filename of over 170 characters then convert it to .cbz

Expected Behavior

Comics archive should open normally

Actual Behavior

Bug is similar to #193 When I toggle devtools it shows net::ERR_FILE_NOT_FOUND for each pages.

This error only happens with .cbz archives but doesn't occur with folders when extracted with the exact same long filenames which is weird.

I tested multiple files and found out this error only starts with filenames with over 170 characters (I used an online character counter while testing)

Error message

net::ERR_FILE_NOT_FOUND

Additional Information

No response

Sample file

No response

himouto commented 2 months ago

Just want to clarify the Preflight Checklist and mention of bug #193

The issues were just similar but not the exact same match

ollm commented 2 months ago

It seems to be an electron/chromium bug, I have opened an issue in the electron repository https://github.com/electron/electron/issues/42634

ollm commented 2 months ago

In the end it turns out that it is a problem with long paths in Windows (Equal or greater than 260 characters), the reason it only happens with compressed files is because the final path can be a little longer when extracted to the Tmp folder.

Example, in this case the Tmp path has 40 character more:

C:\Users\Llopart\Documents\Manga\LongFolderName C:\Users\Llopart\AppData\Local\Temp\opencomic\24f634c70f0dce47df2ded2510722c6c7aba675c\LongFolderName

I have decided that in the event that the path is equal or greater than 260 characters, the short path of the file will be generated (Windows 8.3 short filename), NTFS supports these short paths (If it has not been manually disabled), so it should solve this issue in most cases 884bccd153a4cb2775fbfb5f0e7c6575188ec073

Example of the 8.3 short filename, in this case has 19 characters less:

C:\Users\Llopart\AppData\Local\Temp\OPENCO~1\24F634~1\LongFolderName

Build with fix: https://mega.nz/file/SGh1FCgC#rjRqBd1aKsLeuzKKGZ3OsJ5mYJ8PWqtdBVp6u8VtM5c

himouto commented 2 months ago

In the end it turns out that it is a problem with long paths in Windows (Equal or greater than 260 characters), the reason it only happens with compressed files is because the final path can be a little longer when extracted to the Tmp folder.

Example, in this case the Tmp path has 40 character more:

C:\Users\Llopart\Documents\Manga\LongFolderName C:\Users\Llopart\AppData\Local\Temp\opencomic\24f634c70f0dce47df2ded2510722c6c7aba675c\LongFolderName

I have decided that in the event that the path is equal or greater than 260 characters, the short path of the file will be generated (Windows 8.3 short filename), NTFS supports these short paths (If it has not been manually disabled), so it should solve this issue in most cases 884bccd

Example of the 8.3 short filename, in this case has 19 characters less:

C:\Users\Llopart\AppData\Local\Temp\OPENCO~1\24F634~1\LongFolderName

Build with fix: https://mega.nz/file/SGh1FCgC#rjRqBd1aKsLeuzKKGZ3OsJ5mYJ8PWqtdBVp6u8VtM5c

Thanks so much for this

dajotim937 commented 2 weeks ago

I have one of your build after your fix for this issue. image For these files: https://nyaa.si/view/1828761 It's second time I caught this error.

And if I manually extract files into folder and try to open just images, then I get blank pages.

ollm commented 2 weeks ago

It seems to be the same error as https://github.com/ollm/OpenComic/issues/251

I have made these changes to fix it 549b54cf40e38306e4f8eef1b9a269f50ea06c23

Build: https://mega.nz/file/iX4XWbxS#OdWlWLY3-P0FV_wPAe8ypxwgEVrMiPUMPAq27dJJWg0

dajotim937 commented 2 weeks ago

Indeed, problem with cbz. But if I extract files and trying to open folder, not cbz, I'm getting bunch of these (same error like in first comment): 2024-08-22_13-02-24

If it's important, I've already enabled 'LongPathsEnabled' in registry.

ollm commented 2 weeks ago

It seems that the Windows short path solution does not work in this case, probably because the HDD where the images are located does not have short path enabled.

I've made some new changes to fix this 7b1bbdfe3c4a14e0e8987c3d02c4ac40886da010, when a path of less than 260 characters cannot be generated, if sharp interpolation method is enabled, the \\?\ prefix is ​​added to the path (This is enough for sharp), in case interpolation is disabled or you're viewing the image in its original size, I've had to make it copy the image to tmp folder.

Build: https://mega.nz/file/rbZEgbRI#rGbM2her1XlWFlQf9ZbAuJZSt8NOiOo359TQOMr8w-A

dajotim937 commented 2 weeks ago

Yeah it's work now.

I've had to make it copy the image to tmp folder.

Depends on what's under the hood, eventually, it probably would be better to find better way to handle that. I mean sometimes such folder with long names is vol (like in my link above) and copy 300-600 mb to tmp for each folder it's kinda costly, I guess.

But it's just thinking out loud, because it's up to you of course.

ollm commented 2 weeks ago

Depends on what's under the hood

Copied files are handled the same way as files extracted from CBZ, CBR, etc. As long as the time/size set in the app settings are not exceeded for the tmp folder, the files are kept (Most likely it will only be copied once, unless the maximum size of the tmp is small or 0GB), once some of the 2 are exceeded, the files that have not been used for the longest time will be deleted.

I mean sometimes such folder with long names is vol (like in my link above) and copy 300-600 mb to tmp for each folder it's kinda costly, I guess.

Copying is not done with all images at once, when reading only the first 5-10 adjacent images are rendered, and as the reading progresses the rest, also if you have interpolation activated, it is likely that the copying will not be done during the entire reading.

eventually, it probably would be better to find better way to handle that

A possible better option would be to create a Blob object for these cases, which is what is used for sharp interpolated images, I will do some tests to see if this is better option or not.

dajotim937 commented 2 weeks ago

A possible better option would be to create a Blob object for these cases, which is what is used for sharp interpolated images, I will do some tests to see if this is better option or not.

I'm not familiar with electron\js\ts, so is there a reason why it can copy files with long path to tmp but can't display them without short path when path.length>260? If blob object exists only in RAM without copying to tmp that would be a good alternative, I guess. Yeah, depends on performance of course.

ollm commented 1 week ago

I'm not familiar with electron\js\ts, so is there a reason why it can copy files with long path to tmp but can't display them without short path when path.length>260?

This is because electron combines node.js and chromium, the main process, which is where I am doing the copy/reading of the file is handled by node.js, but the rendering part is handled by chromium, node.js handles long paths but chromium does not.

If blob object exists only in RAM without copying to tmp that would be a good alternative, I guess. Yeah, depends on performance of course.

I've been looking and blob files should be kept in memory up to a certain size (2GB - 4GB Chrome's Blob Storage System Design), if it is exceeded then they are also saved as temporary files.