Sigil crashes when loading file from the internet

[DiapDealer]

Originally Posted by KevinH

My understanding is that it is the getOpenGpFileNames routine that is somehow crashing before it returns. Is that correct?

I don't think I have enough info to answer that question yet. Without any extra added debug info enabled in the 2.5.2 Windows release, I can say that the crash happens quite a long time after the OK button on getOpenFileNames is clicked. The mouse pointer changes to and from "working" several distinct times. The QTreeView on the left of the SelectFiles.cpp dialog (the one whose "Other Files" buttons call the getOpenFileNames dialog) changes in appearance just before the crash. So I can't be certain if it's getOpenFileNames itself, or if it's QModelIndex data associated with the treeview being handed something (from getOpenFileNames) that it's barfing on. Hoping to do a debug build today or tomorrow to determine where exactly it's dying.

If it is the latter, then clearly the non-native dialog has some sort of extra guardrails that the native dialogs lacks to prevent bad data from being passed on.

[DiapDealer]

I'm a little mistaken on those above details. Give me some time to debug properly.

[DiapDealer]

OK. The getOpenFileNames dialog is not crashing on Windows. It's returning a value for the rest of BookBrowser::AddExisting() to continue processing. There merely seems to be some sort of built-in guardrail in the non-native dialog (and the native one on linux) that returns an empty value when an url is detected.

NOTE: the native getOpenFileNames dialog on my Linux Cinnamon desktop doesn't even give me a field to paste anything into. Just a folder Tree View, a file(s) selection pane, and OK/Cancel buttons.

The "actual" problem area on Windows is line 937 of BookBrowser.cpp

Resource *resource = m_Book->GetFolderKeeper()->AddContentFileToFolder(filepath);

I've added debug output immediately before and after that line, and what comes after never gets output.

Debug: Returned from getOpenFileNames  QList("https://www.adfg.alaska.gov/static/fishing/images/sport/galleries/Sport_Fish.1.jpg")
Debug: Last folder open =  "C:/Program Files/Sigil/https:/www.adfg.alaska.gov/static/fishing/images/sport/galleries"
Debug: File name portion =  "Sport_Fish.1.jpg"
Debug: Preparing to add new resource for:  "https://www.adfg.alaska.gov/static/fishing/images/sport/galleries/Sport_Fish.1.jpg"
Warning: Qt has caught an exception thrown from an event handler. Throwing
exceptions from an event handler is not supported in Qt.
You must not let any exception whatsoever propagate through Qt code.
Warning: Release of profile requested but WebEnginePage still not deleted. Expect troubles !
Warning: Release of profile requested but WebEnginePage still not deleted. Expect troubles !
Warning: Release of profile requested but WebEnginePage still not deleted. Expect troubles !

For what it's worth: I'm not at all opposed to using a non-native dialog on Windows here if it's the easiest fix. I also don't mind digging deeper (or adding a function that checks for urls entered and balks). **shrug**

[DiapDealer]

Why ARE we being given a field to paste a value into on this dialog in the first place? Is there ever going to be a time when a user has an entire valid file path on the clipboard to paste in here and save time browsing to images (or other files) they want to add to the epub?

I've already checked and there doesn't seem to be an option to hide such a field, so my question is pretty pointless, but still...

Line 798 of BookBrowser.cpp is producing a wildly invalid file path that might be an obvious spot to head things off:

 m_LastFolderOpen = QFileInfo(filepaths.first()).absolutePath();

is producing:

C:/Program Files/Sigil/https:/www.adfg.alaska.gov/static/fishing/images/sport/galleries

[KevinH]

That is interesting ... great detective work! So the more correct approach is probably to detect urls being returned on all platforms. Given a colon is not an allowed in a file path in MacOS it would be easy to screen it out on MacOS. But given Windows filepaths use colons after drive letters, and custom url schemes are legal (calibre, sigil, http, https, ftp, file, etc, etc, then a simple screen would probably not be easy. Maybe as BeckyEbook suggested we use Utility::IsFileReadable and see if that returns without crashing, but it may use QFileInfo too.

Which way do you want to go?

[DiapDealer]

I would think there's never going to be a network protocol in the middle of a valid local file path on any platform is there? http:/|https:/|ftp:/, etc...

C:/Program Files/Sigil/https:/www.adfg.alaska.gov/static/fishing/images/sport/galleries

Wouldn't some sort of if path exists suffice?

Let me dig a little deeper into where the crash is happening in Book.cpp

m_Book->GetFolderKeeper()->AddContentFileToFolder(filepath)

[KevinH]

I do think we do want to screen it immediately after the getOpenFileNames call returns and set it to be a null string if any type of url is returned just like the non-native ones do now.

That prevents any rippling effects downstream.

[DiapDealer]

That makes sense to me. Let me ask this: is there ever going to be any circumstance where a path manually entered (either by pasting or typing) into that getOpenFileNames dialog (Add Existing files to epub) might make use of any url whatsoever?

I'm asking in case simply trying to define the returned QString (or in this case loop through all QStrings in the returned QList) as a QUrl and then using QUrl::isValid() to test might be enough?

[KevinH]

Well the KDE Plasma file dialog only ruled out non-file scheme urls so it might let a file: url pass through.

Sigil code was generally not designed to accept a url from open file dialogs. It wants file paths or in this case a list of paths not a list of urls.

So I say we just check if url of any type (but be careful of relative urls) and punt if so.
So maybe a simple file existance or is readable test here is the safest approach.

[mrprobert]

Deleted

[DiapDealer]

Originally Posted by KevinH

Well the KDE Plasma file dialog only ruled out non-file scheme urls so it might let a file: url pass through.

Sigil code was generally not designed to accept a url from open file dialogs. It wants file paths or in this case a list of paths not a list of urls.

So I say we just check if url of any type (but be careful of relative urls) and punt if so.
So maybe a simple file existance or is readable test here is the safest approach.

I agree and in testing it seems to work. BUT... something weird still seems to be happening on Windows. There is a fairly lengthy delay (on a VM, but still...) after submitting the url. During which time the cursor appears and reappears several times. Then the getOpenFileNames dialog simply goes away. There's way more going on than my code that checks for an url being submitted, but I don't know what.

I can easily warn the user with a utility dialog warning about not entering urls, but I'd like to try and find out what is happening that takes so much processing time before the url-checking routine even takes over. Very weird.

[KevinH]

Yes sounds like a plan. But either way we may be safer if a non-native file dialog box is used. But I would still add in a isFileReadable check for all platforms to be even safer

[KevinH]

What is really strange is that QFileDialog getOpenFileName() and getOpenFileNames() should never return a url as there exists getOpenFileUrl() and getOpenFileUrls() in that same QFileDialog class.

My guess is much of the code is shared but that the Name and Names should actively screen out Urls!

So as far as I can tell, this is a Qt bug. We will just have to guard against it.

[DiapDealer]

Yes. I think the non-native dialog just handles this situation better on Windows. It works quicker, and automatically notifies the user that there's a problem. And an isFileReadable check for all platforms for safety makes sense.

[KevinH]

Originally Posted by DiapDealer

Yes. I think the non-native dialog just handles this situation better on Windows. It works quicker, and automatically notifies the user that there's a problem. And an isFileReadable check for all platforms for safety makes sense.

Agreed. Will you push those changes to BookBrowser.cpp or do you want me to?

Either way, we have time as we merged in new strings, so after your change I will update base.ts and we can let the translators get started on them while we wait in case other bugs are reported. And no need to rush out a 2.5.3 since this is not a usual crash, but requires copying and pasting in a url by the user when asked for a file path.